dexc-desktop: prepare installable desktop app #1957
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Cool. One of the reasons I had closed the PoC webview PR (#1452) was because webview had appeared to be dead with about a year of no commits, but it seems to have been brought back to life earlier this year. We should request a stable release tag if we wanna commit to it as a framework for a dex desktop app. |
chappjc
reviewed
Nov 12, 2022
I'm not commited to WebView at all. Honestly, if I didn't have anyone to answer to, I would just include a chromium binary with the distro and run it with |
If licensing permits, I wouldn't object to that given we can sandbox the thing so it doesn't fiddle with the user's chromium settings. |
I'm pretty sure we'd have to have the app behave like a "downloader" to fetch the browser because third party binaries with ours is what would be problematic in terms of licensing. |
buck54321
force-pushed
the
desktop-app
branch
2 times, most recently
from
12a618e to
b2473d0
Compare
|
Still Webview, just a separate process for windows. |
buck54321
force-pushed
the
desktop-app
branch
from
cbecf72 to
bb03538
Compare
JoeGruffins
approved these changes
Dec 5, 2022
Comment on lines
68
to
82
| if resp.StatusCode == http.StatusOK { | ||
| log.Errorf("Unexpected response code from kill signal send: %d (%s)", resp.StatusCode, http.StatusText(resp.StatusCode)) | ||
| } |
| case s.openC <- struct{}{}: | ||
| log.Info("Window reopened") | ||
| default: | ||
| log.Infof("Ignored a window reopen request from another instance") |
There was a problem hiding this comment.
nit: Nothing to format, here and in a few more places.
chappjc
reviewed
Dec 5, 2022
There was a problem hiding this comment.
When this goes in we can squash with you as author. After squash, git commit --amend --author="buck54321 <[email protected]>", and ideally tweaked commit message like
add cmd/dexc-desktop
Introduce a new cmd, dexc-desktop. This coordinates a WebView
window with our normal dexc stack.
Prepare Debian archive for installation, including control files,
Desktop Entry specificiation and icons. User can install with
their preferred package manager.
Moves some common utilities to a new package client/app.
Co-authored-by: Jonathan Chappelow <[email protected]>
The pkg-debian.sh addition would ideally be a second commit since dexc-desktop is mostly unrelated to Debian.
client/cmd/dexc-desktop/main.go
Outdated
| m.Unlock() | ||
| defer closeWindow(windowID) | ||
|
|
||
| cmd.Run() |
client/cmd/dexc-desktop/main.go
Outdated
Comment on lines
533
to
512
| // TODO: Enable toggling automatic startup on boot. This would be part of a | ||
| // larger effort aimed at securing refunds through system restarts. | ||
| // https://github.com/decred/dcrdex/pull/1957#discussion_r1020780014 |
There was a problem hiding this comment.
This seems firmly in the realm of user/os setup. At best an installer. Do you mean this app itself that is running dex core would make changes the the user/os env to make auto-startup?
My view is that users start programs that they want to run. We can facilitate system service setup with scripts, systemd configs, installer, etc., but I don't think this app code should be concerned with that.
norwnd
reviewed
Dec 27, 2022
norwnd
reviewed
Dec 27, 2022
buck54321
changed the title
chappjc
reviewed
Apr 17, 2023
chappjc
reviewed
Apr 20, 2023
chappjc
reviewed
Apr 20, 2023
chappjc
reviewed
Apr 20, 2023
chappjc
reviewed
Apr 20, 2023
buck54321
force-pushed
the
desktop-app
branch
3 times, most recently
from
06a8919 to
dfdfd39
Compare
|
Ran the dexc-desktop on my Mac and hit this panic immediately. Panic logs |
|
@ukane-philemon Would you try a fresh project with just this to see if it works on your mac? package main
import "github.com/webview/webview"
func main() {
w := webview.New(false)
defer w.Destroy()
w.SetTitle("Example")
w.SetSize(480, 320, webview.HintNone)
w.Navigate("http://127.0.0.1:8080")
w.Run()
} |
|
The issue for MacOS is executing |
Nice. I think I recall that issue when working on dexc/main_tray.go |
chappjc
reviewed
Apr 29, 2023
client/cmd/dexc-desktop/main.go
Outdated
Comment on lines
320
to
323
| windowloop: | ||
| for { | ||
| var backgroundNoteSent bool | ||
| select { |
There was a problem hiding this comment.
Looks like main needs a refactor so that this is in a goroutine and main is just blocking on systray.Run.
Yup, no panic with your suggestion. #1957 (comment) |
|
While testing #2333, I noticed that the |
|
@buck54321, I have some code suggestions that might interest you here e2a3f17. |
I added a debug-level log for the the output of |
chappjc
reviewed
May 4, 2023
@ukane-philemon maybe try with the |
chappjc
reviewed
May 4, 2023
Okay. |
Comment on lines
+404
to
+414
| log.Infof("Closing window. %d windows remain open.", remain) | ||
| cmd.Process.Kill() |
There was a problem hiding this comment.
If I shut down before a webview is opened, I see a panic since cmd.Process will still be nil but have been added to m.windows giving another process an opportunity to access cmd.Process.
ukane-philemon
suggested changes
May 4, 2023
client/cmd/dexc-desktop/main.go
Outdated
Comment on lines
421
to
432
| w.SetTitle("Decred DEX Client") | ||
| w.SetSize(int(C.display_width()), int(C.display_height()), webview.HintNone) |
There was a problem hiding this comment.
We should also set a min width here. w.SetSize(600, 600, webview.HintMin)(min width & height can be anything > 100). The webview almost disappears when I click on minimise and it doesn't look good if users can minimise the window all the way.
client/cmd/dexc-desktop/main.go
Outdated
Comment on lines
361
to
365
| systray.Run(func() { | ||
| systrayOnReady(appCtx, filepath.Dir(cfg.LogPath), openC, killChan) | ||
| }, func() { | ||
| wg.Done() | ||
| }) |
There was a problem hiding this comment.
should remove wg.Done(), the last func argument can just be nil.
client/cmd/dexc-desktop/main.go
Outdated
Comment on lines
312
to
313
| defer wg.Done() | ||
|
|
There was a problem hiding this comment.
We should also defer cancel() here since we will no longer be able to create new windows if this goroutine is exited.
|
Made some conflicts for you. Getting to final review though |
chappjc
reviewed
May 8, 2023
buck54321
commented
May 9, 2023
package a desktop version for Debian Introduce a new cmd, dexc-desktop. This coordinates a WebView window with our normal dexc stack. Moves some common utilities to a new package client/app. Co-authored-by: Jonathan Chappelow <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prepare Debian archive for installation, including control files, Desktop Entry specificiation and icons. User can install with their preferred package manager.Debian packaging moved to separate branch.