Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dexc-desktop: Use macdrive for Darwin #2372

Merged
merged 17 commits into from
Jul 17, 2023
Merged

Conversation

ukane-philemon
Copy link
Contributor

@ukane-philemon ukane-philemon commented May 25, 2023

Builds on #2333.

I researched other options and MacDrive seem to be a reasonable option. It allows using supported native Mac APIs, which covers our use case.

I was able to implement some "native" macOS app behaviour and features; keeping the app running without windows, creating new windows, and having a dock icon menu.

Screenshot 2023-05-25 at 8 35 26 PM

I'm putting this up so we can have discussions around it and clarify what feature is missing (menu items etc)

EDIT: If we want to keep the New Window menu as @martonp mentioned here #2333 (comment), we'd have to allow more than one window on macOS.

@ukane-philemon
Copy link
Contributor Author

Changes included in 638752b

Support for multiple windows (max: 5. this is not really a requirement just keeping things sane?)
Screenshot 2023-05-26 at 10 32 31 PM

Systray display.
Screenshot 2023-05-26 at 10 32 58 PM

Copy link
Contributor

@martonp martonp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good.. personally I don't see any reason for multiple windows, but it's fine.

The scrollbar issue that was happening earlier is back again:
Screenshot 2023-05-28 at 6 12 17 AM

client/cmd/dexc-desktop/app_darwin.go Outdated Show resolved Hide resolved
client/cmd/dexc-desktop/app_darwin.go Outdated Show resolved Hide resolved
client/cmd/dexc-desktop/app_darwin.go Show resolved Hide resolved
client/cmd/dexc-desktop/app_darwin.go Outdated Show resolved Hide resolved
@martonp
Copy link
Contributor

martonp commented May 28, 2023

The top-left menu should have the same options as the dock icon menu:
Screenshot 2023-05-28 at 6 25 13 AM

@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented May 28, 2023

The scrollbar issue that was happening earlier is back again:

When? I know some CSS styling is not supported on Webkit. Even some "default" styling in other browsers could also be missing.

EDIT: I'm unable to reproduce, tried building the CSS files again? :

scroll.mp4

EDIT 2: I notice the white bg on hover. @martonp what did you expect?

@ukane-philemon
Copy link
Contributor Author

The top-left menu should have the same options as the dock icon menu: Screenshot 2023-05-28 at 6 25 13 AM

Getting this to work as expected with native APIs was a PITA but it works.

Screenshot 2023-05-31 at 3 38 31 PM Screenshot 2023-05-31 at 3 38 11 PM Screenshot 2023-05-31 at 3 38 24 PM

As a bonus, I was able to get this working too (the nice check and window icon):
Screenshot 2023-05-31 at 3 37 45 PM

@ukane-philemon
Copy link
Contributor Author

Rebased..

@martonp
Copy link
Contributor

martonp commented Jun 3, 2023

The scrollbar issue that was happening earlier is back again:

When? I know some CSS styling is not supported on Webkit. Even some "default" styling in other browsers could also be missing.

EDIT: I'm unable to reproduce, tried building the CSS files again? :

scroll.mp4
EDIT 2: I notice the white bg on hover. @martonp what did you expect?

Seems OK now.. maybe it's an intermittent issue or I didn't build it properly. But the build script builds the UI every time..

@ukane-philemon
Copy link
Contributor Author

But the build script builds the UI every time..

Yup, that was why I did this #2333 (comment) earlier since I'd have to build several times to test everything was fine when working on that PR.

Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We never shut down on Mac?


Window is not opening for me after installation. Logs show that dexc is shutting down right away.

...
2023-06-09 22:18:47.663 [INF] CORE: Connected to 1 of 1 DEX servers
2023-06-09 22:18:47.663 [INF] CORE: Loaded btc wallet configuration.
2023-06-09 22:18:47.663 [INF] CORE: Loaded dcr wallet configuration.
2023-06-09 22:18:47.663 [INF] CORE: Loaded 0 active orders.
2023-06-09 22:18:47.663 [INF] CORE: Loaded 0 active match orders
2023-06-09 22:18:47.663 [DBG] CORE: starting fiat rate fetching
2023-06-09 22:18:47.663 [DBG] WEB: Using embedded site resources.
2023-06-09 22:18:47.663 [INF] WEB: Using language en-US
2023-06-09 22:18:47.663 [INF] WEB: Using embedded HTML templates
2023-06-09 22:18:47.679 [DBG] APP: Exiting dexc main.

client/cmd/dexc-desktop/app_darwin.go Outdated Show resolved Hide resolved
client/cmd/dexc-desktop/app_darwin.go Outdated Show resolved Hide resolved
client/cmd/dexc-desktop/app_darwin.go Outdated Show resolved Hide resolved
client/cmd/dexc-desktop/app_darwin.go Outdated Show resolved Hide resolved
client/cmd/dexc-desktop/app_darwin.go Outdated Show resolved Hide resolved
client/cmd/dexc-desktop/app_darwin.go Outdated Show resolved Hide resolved
client/cmd/dexc-desktop/app_darwin.go Outdated Show resolved Hide resolved
client/cmd/dexc-desktop/app_darwin.go Outdated Show resolved Hide resolved
client/cmd/dexc-desktop/app_darwin.go Show resolved Hide resolved
client/cmd/dexc-desktop/app.go Outdated Show resolved Hide resolved
@ukane-philemon
Copy link
Contributor Author

Window is not opening for me after installation. Logs show that dexc is shutting down right away.

Weird. I wonder what could be terminating dexc-desktop instance on your machine.

@ukane-philemon
Copy link
Contributor Author

We never shut down on Mac?

If you mean automatically(i.e. after all windows have been closed and no active order), no. The user will have to shut down dexc-desktop on macOS using the Quit menu item or use Command+Q.

Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working well. Almost ready, imo.


External links are not functional for me.

image

client/cmd/dexc-desktop/app_darwin.go Outdated Show resolved Hide resolved
client/cmd/dexc-desktop/app_darwin.go Outdated Show resolved Hide resolved
client/cmd/dexc-desktop/app_darwin.go Show resolved Hide resolved
client/cmd/dexc-desktop/app_darwin.go Show resolved Hide resolved
client/cmd/dexc-desktop/app_darwin.go Outdated Show resolved Hide resolved
@ukane-philemon
Copy link
Contributor Author

Working well. Almost ready, imo.

I'm glad, you were able to test this PR and it's running fine on your end. I will look into the issue with the links.

@martonp
Copy link
Contributor

martonp commented Jun 24, 2023

In the "Window" section of the top toolbar, there are options for tabs, i.e. "Show Tab Bar", "Show Previous Tab". Is it possible to get rid of those?

@ukane-philemon
Copy link
Contributor Author

In the "Window" section of the top toolbar, there are options for tabs, i.e. "Show Tab Bar", "Show Previous Tab". Is it possible to get rid of those?

Yeah, it is possible. I looked into it over the weekend.

Signed-off-by: Philemon Ukane <[email protected]>
- Add support for upto 5 windows for macOS.
- Add icon to system status bar.
- Add more webview config preferences.
- Refactor.

Signed-off-by: Philemon Ukane <[email protected]>
- Handle defer statements properly for darwin.
- Remove syncserver for darwin
- Refactor
- Prevent multiple dexc-desktop instance.
Signed-off-by: Philemon Ukane <[email protected]>
@ukane-philemon
Copy link
Contributor Author

Rebased for 36a9a1a.

@chappjc
Copy link
Member

chappjc commented Jul 10, 2023

This looks A-OK to me. I'm unlikely to get a Mac to test anytime soon, so please decide when it's good to merge @buck54321 and pull the trigger when you like.

@ukane-philemon
Copy link
Contributor Author

This looks A-OK to me. I'm unlikely to get a Mac to test anytime soon, so please decide when it's good to merge @buck54321 and pull the trigger when you like.

I am yet to push changes relating to the recent reviews but the major changes I'd like to add here are:

  1. Getting external links to open in the user's default browser.
  2. Enabling file uploads. (Figured out this wasn’t working while testing but has been a real PITA. I've implemented the required handlers/methods but still getting panics): Related documentations:
    Doc 1, Doc 2 and Docs 3
  3. Refactoring mainCore because it now looks too stuffy and long.

@chappjc
Copy link
Member

chappjc commented Jul 10, 2023

2. Enabling file uploads. (Figured out this wasn’t working while testing but has been a real PITA. I've implemented the required handlers/methods but still getting panics)

Uploads? For the wallet config file and dex cert file selections? Do they not work?

@ukane-philemon
Copy link
Contributor Author

Uploads? For the wallet config file and dex cert file selections? Do they not work?

Yes, they won't work unless we implement a special method for that.

@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented Jul 15, 2023

@chappjc, @buck54321, @martonp, blockers mentioned in #2372 (comment) have been addressed in my recent commits(i.e 3f1052d).

- Implement self-signed cert challenge handler for MacOS app

Signed-off-by: Philemon Ukane <[email protected]>
Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @ukane-philemon. I've confirmed that links and file uploads work.

I noticed that when I open the app, the window opens but doesn't take focus. Anybody else seeing this? I thought it was working before.

@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented Jul 17, 2023

I noticed that when I open the app, the window opens but doesn't take focus. Anybody else seeing this? I thought it was working before.

I've just looked into this and I don't know how we can make the login form take focus automatically in NSWindow. We already use makeKeyAndOrderFront:, and I think that's all we need. Users might need to interact with the form before entering their password.

@buck54321 buck54321 merged commit 3dd6adf into decred:master Jul 17, 2023
5 checks passed
@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented Jul 17, 2023

After poking around I found out we can achieve that behaviour with [[NSRunningApplication currentApplication] activateWithOptions:(NSApplicationActivateAllWindows | NSApplicationActivateIgnoringOtherApps)]; but it is discouraged in the Apple docs: You should rarely pass this flag because stealing key focus produces a poor user experience. See: https://developer.apple.com/documentation/appkit/nsapplicationactivationoptions/nsapplicationactivateignoringotherapps?language=objc

But IMO, I think we should enable this behaviour. Saves users the extra burden of interacting with the app. After all, they opened it and intend to use it. What do you think @buck54321?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants