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

packaging: dexc-desktop packaging for Darwin #2333

Merged
merged 4 commits into from Jun 1, 2023

Conversation

ukane-philemon
Copy link
Contributor

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

This builds on top of #1957.

EDIT: Decred DEX Client in the images have been shortened to Decred DEX.
Display:
View in the Applications folder.
Screenshot 2023-05-01 at 9 54 12 AM

DMG installer view.
Screenshot 2023-05-01 at 1 42 01 PM

Dock view.
Screenshot 2023-05-01 at 9 52 20 AM

Screenshot 2023-05-01 at 9 52 00 AM

TODO:

  • We might have to create icons of various sizes:
    16x16 pixels (1x and 2x), 32x32 pixels (1x and 2x), 64x64 pixels (1x and 2x), 128x128 pixels (1x and 2x), 256x256 pixels (1x and 2x), 512x512 pixels (1x and 2x), 1024x1024 pixels (1x only) which will be converted to a single .icns file for MacOs to display the most appropriate icon anywhere.
  • This PR includes a background image to instruct users on how to install the .app folder but we can improve on that too.

@martonp, please I'd appreciate your feedback on this since you also use MacOS.

Comment on lines 99 to 129
# signs the dmg file given to it as an argument.
function sign() {
if [[ -n "${SIGNATURE}" && "${SIGNATURE}" != "-null-" ]]; then
echo "Codesign started"
codesign -s "${SIGNATURE}" "$1"
dmgsignaturecheck="$(codesign --verify --deep --verbose=2 --strict "$1" 2>&1 >/dev/null)"
if [ $? -eq 0 ]; then
echo "The disk image is now codesigned"
else
echo "The signature seems invalid.."
exit 1
fi
fi
}

# notarizes the dmg file given to it as an argument.
function notarize() {
if [[ -n "${NOTARIZE}" && "${NOTARIZE}" != "-null-" ]]; then
echo "Notarization started"
xcrun notarytool submit "$1" --keychain-profile "${NOTARIZE}" --wait
echo "Stapling the notarization ticket"
staple="$(xcrun stapler staple "$1")"
if [ $? -eq 0 ]; then
echo "The disk image is now notarized"
else
echo "$staple"
echo "The notarization failed with error $?"
exit 1
fi
fi
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These aren't been used atm since a $99 dev account is required to get an Apple dev signature for code signing. Notarizing the dexc-desktop dmg is debatable but IMO we might not need it.

@chappjc
Copy link
Member

chappjc commented May 9, 2023

Can rebase with dexc-desktop merged. No rush though.

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.

I was able to build, install, and run it. However when I start the program, I cannot see anything on the markets page and there is an error in the console:

Screenshot 2023-05-10 at 10 54 45 AM Screenshot 2023-05-10 at 10 53 39 AM

@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented May 10, 2023

Uhmm, I'm on the markets page and everything looks fine except I now noticed the order form is a mess, no js error. I will investigate further.
Screenshot 2023-05-10 at 4 00 43 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.

The open logs folder selector is not working.

Why don't we just include the create-dmg script in the repo and have pkg-darwin.sh call the create-dmg script? Then we can just grab any updates they make.

client/cmd/dexc-desktop/pkg/pkg-darwin.sh Outdated Show resolved Hide resolved
@chappjc
Copy link
Member

chappjc commented May 13, 2023

The open logs folder selector is not working.

Did you guys try converting spaces as per my code comment:

// url.Parse can be touchy, so consider replacing only spaces, manually:
// path = strings.ReplaceAll(path, " ", "%20")
return fileURL.String(), nil

My Mac VM is pretty much broken because it refuses to display graphics in a lot of apps, so can't test easily.

@ukane-philemon
Copy link
Contributor Author

Did you guys try converting spaces as per my code comment:

There was no space in the resulting folder url.

@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented May 13, 2023

Why don't we just include the create-dmg script in the repo and have pkg-darwin.sh call the create-dmg script? Then we can just grab any updates they make.

IMO, we can do with less code independently. Updates can be added if need be.

@martonp
Copy link
Contributor

martonp commented May 13, 2023

IMO, we can do with less code independently. Updates can be added if need be.

Why would it be more code to use their script? It seems like most of this is copy-pasted from there.

@ukane-philemon
Copy link
Contributor Author

Why would it be more code to use their script? It seems like most of this is copy-pasted from there.

Yes, I had to use a chunk of their code since it solves the issue at hand. I thought we'd prefer a custom build flow but @chappjc can decide which is better.

@ukane-philemon
Copy link
Contributor Author

Encountered these display issues and added commits to resolve them in this PR. @martonp please confirm if you can reproduce the weird display without these changes and if these changes fix them on your device.

Screenshot 2023-05-13 at 10 56 39 AM Screenshot 2023-05-13 at 10 56 15 AM Screenshot 2023-05-13 at 10 58 24 AM Screenshot 2023-05-13 at 6 49 05 AM Screenshot 2023-05-13 at 10 58 01 AM

@ukane-philemon
Copy link
Contributor Author

When I resize the web view window on my Mac with the dev console open, it chops off the top of the page by an amount equal to the console's minimum height. This issue persists even after refreshing the page, but clicking on the resize icon at the top right corner when the dev console is closed resolves it. I haven't observed this problem on Safari or other browsers.

However, this behaviour doesn't occur when resizing the window with the dev console detached. It may be considered a non-issue since we don't anticipate users resizing the web view window while the dev console is open.

webview-window.mp4

@chappjc
Copy link
Member

chappjc commented May 13, 2023

Why would it be more code to use their script? It seems like most of this is copy-pasted from there.

Yes, I had to use a chunk of their code since it solves the issue at hand. I thought we'd prefer a custom build flow but @chappjc can decide which is better.

I have no opinion on the issue. It's not clear to me what it would look like doing it the way @martonp suggests. Is it this? https://github.com/create-dmg/create-dmg
It's up to you guys.

@ukane-philemon
Copy link
Contributor Author

Is it this? https://github.com/create-dmg/create-dmg

Yes, it is.

@chappjc
Copy link
Member

chappjc commented May 16, 2023

I'm fixing up the "Open logs folder" functionality in #2351.
It shouldn't be trying to open a folder in a webview window. It should use the OS/window manager to do it, which is what the https://github.com/pkg/browser/blob/master/browser_darwin.go package does, although it's named in such a way to suggest it just opens browsers, when really it uses os-specific opening functions.

@ukane-philemon
Copy link
Contributor Author

I'm fixing up the "Open logs folder" functionality in #2351.

Nice, tested well on MacOS.

@ukane-philemon
Copy link
Contributor Author

Why don't we just include the create-dmg script in the repo and have pkg-darwin.sh call the create-dmg script? Then we can just grab any updates they make.

I agree this can eliminate additional complexity and maintenance overhead, regardless of the amount of code involved. We can have it if the others agree on this @martonp.

@chappjc
Copy link
Member

chappjc commented May 19, 2023

I think you should use grab the create-dmg in full, and make the adjustments in our separate script, add @martonp says. Does that work out?

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.

The UI is looking good now, however I see some other issues.

On the tray icon, when I click "Open" it is always opening a new window even if one is already open. Also if another App is in the foreground and I click the "Open", it will just create a new window behind the app that is in the foreground.

I actually think on Mac the tray icon is not the way to go. When there is an app running on mac, even if there is not a window open, it will still be displayed on the dock at the bottom of the screen. When you right click the icon at the bottom of the screen, there are usually some options. The "Open logs folder" option can go over there. It feels strange that the app is running but I see no icon on the bottom dock. Every other app is there.

client/cmd/dexc-desktop/pkg/pkg-darwin.sh Outdated Show resolved Hide resolved
@chappjc
Copy link
Member

chappjc commented May 21, 2023

On the tray icon, when I click "Open" it is always opening a new window even if one is already open. Also if another App is in the foreground and I click the "Open", it will just create a new window behind the app that is in the foreground.

I was surprised by this too, but @buck54321 makes the point that this is like making multiple tabs, which webview does not support. EDIT: Note that this is not Mac-specific behavior.

I actually think on Mac the tray icon is not the way to go. When there is an app running on mac, even if there is not a window open, it will still be displayed on the dock at the bottom of the screen. When you right click the icon at the bottom of the screen, there are usually some options.

On all systems, I made my case in chat that the tray icon is pointless except when all windows are closed and it's just waiting for orders to settle or something. On this note, I still don't like that closing the "last window" should terminate the singular dex process running Core. I think that the sole utility of a tray icon is so that you can close all the windows and let it continue running, where the tray provides a way to resurrect a window.

On the Mac specific points, even if we adjust things as per my previous paragraph, it's debatable if the tray icon is desirable or not. Only the main dexc process that has the running Core serves this tray. Only that process would be capable of providing the same menu items but on the dock as you suggest, not the dummy webview processes.
Also jrick made the point in chat that it's still nice to have some visual other than the subtle black dots in the Dock.

I don't use Mac enough to have an informed opinion, but I do think the functionality of the tray icon on all systems is flawed (the "last window" kills the Core process design).

@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented May 21, 2023

On the tray icon, when I click "Open" it is always opening a new window even if one is already open. Also if another App is in the foreground and I click the "Open", it will just create a new window behind the app that is in the foreground

I understand your concerns. We had some discussion around this on matrix, and as @chappjc has said, creating sub webview window is flawed. Also, creating a new webview window will make another icon appear in the dock and it was unpleasant to look at, so I had to add a .plist item to make it behave like a background app so users can create multiple windows without displaying unnecessary icons in the dock. Our use of a separate dexc process to create a new window allow for multiple webview window on Mac and I don't know if there's a workaround.

but I do think the functionality of the tray icon on all systems is flawed (the "last window" kills the Core process design).

I agree. We have the systray, whether or not an order is running I think we should remove this behaviour. Some applications behave this way though, but it's up to us and what's best in this case for users.

@martonp
Copy link
Contributor

martonp commented May 22, 2023

Why does anyone need to have multiple dexc windows open.

I don't see any subtle black dots. I see this:

Screenshot 2023-05-22 at 4 22 49 AM

On Mac, all the applications that are running, whether or not there is a window open, are displayed on the bottom dock like this, with a dot below them.

Screenshot 2023-05-22 at 4 23 40 AM

Even if I close all the windows, the app will still stay running. To actually terminate the program, you have to right click the icon and select "Quit." I think the ideal behavior would be that if there are no active trades it just quits. If there are active trades it should give a confirmation alert first.

I definitely don't think this should be a background app. I think there should just be one window allowed, get rid of the dock on top, and just have the bottom dock icon behavior as I wrote in the previous paragraph.

@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented May 22, 2023

Doing it the "Mac way" using webview is a can of worms.

@chappjc
Copy link
Member

chappjc commented May 22, 2023

Why does anyone need to have multiple dexc windows open.

I tend to agree. I was confused.

I don't see any subtle black dots. I see this:

Screenshot 2023-05-22 at 4 22 49 AM

On Mac, all the applications that are running, whether or not there is a window open, are displayed on the bottom dock like this, with a dot below them.

Screenshot 2023-05-22 at 4 23 40 AM

Those are the two options, yes. The black dots option I'm talking about is actually those gray dots there. If that's how mac users work, ok.

Even if I close all the windows, the app will still stay running. To actually terminate the program, you have to right click the icon and select "Quit." I think the ideal behavior would be that if there are no active trades it just quits. If there are active trades it should give a confirmation alert first.

I haven't tested this PR, but dexc-desktop right now kills the app when all the windows are closed, unless there are trades running.

I definitely don't think this should be a background app. I think there should just be one window allowed, get rid of the dock on top, and just have the bottom dock icon behavior as I wrote in the previous paragraph.

What is wrong with having it as a background app? You don't need the window when you have placed all your orders and just want to let it work. We don't need the systray bits to make that work on Mac because of how the dock works, I acknowledge you points, but I still think it's desirable. Are you saying that on mac it's just as good to minimize the window to achieve this?

The "Mac way" I have observed is that closing all the windows does not kill the app. You have to go to the menu and hit "Quit"

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.

DMG stuff LGTM. I think it may be possible to have the application be hidden instead of closed when the window closes by calling some objective-c code in the cgo section, but I haven't figured out exactly how yet.

I remove the <key>LSUIElement</key><true/> element and also created the webview in the original process instead of using the subprocess. Then if I right click the icon and select "hide", that is the exact behavior we want when the red button is clicked.

@buck54321
Copy link
Member

buck54321 commented May 24, 2023

Just that there will be no little dot below the icon in the dock. Users will have to use the systray to manage dexc-desktop

Are you saying when running in the background, they can't open a window by clicking on the app's icon in the dock?

@buck54321
Copy link
Member

  1. (this didn't look right so i added <key>LSUIElement</key><true/> to make all the dock icons disappear even when there are multiple windows open)

Hmm. Do you mean that you can't pin an icon to the dock anymore?

Sorry for the questions. I'm trying to get a mac right now. Hopefully I'll be more helpful soon.

@ukane-philemon
Copy link
Contributor Author

I remove the LSUIElement element and also created the webview in the original process instead of using the subprocess. Then if I right click the icon and select "hide", that is the exact behavior we want when the red button is clicked.

But sadly we don't have control over the red btn yet when running webview in the original process.

@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented May 24, 2023

Are you saying when running in the background, they can't open a window by clicking on the app's icon in the dock?

Yep. But the systray will be visible and users can open new windows from there.

Hmm. Do you mean that you can't pin an icon to the dock anymore?

You can, for easy access.

Sorry for the questions. I'm trying to get a mac right now. Hopefully I'll be more helpful soon.

np.

@martonp
Copy link
Contributor

martonp commented May 25, 2023

If this returned 0 instead of 1, I think we'd be good...

https://github.com/webview/webview/blob/8387ff8945fc010e7c4203c021943ce4ca12a276/webview.h#LL832C27-L832C74

@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented May 25, 2023

If this returned 0 instead of 1, I think we'd be good...

https://github.com/webview/webview/blob/8387ff8945fc010e7c4203c021943ce4ca12a276/webview.h#LL832C27-L832C74

This will cause dexc-desktop to continue running (without a window), but we won't be able to "resurrect" the window if the user clicks on the dock icon again.

It seems the trend is to clone webview and add the desired feature as other forks have so I did yesterday but I think we need total control over webview windows.

@martonp
Copy link
Contributor

martonp commented May 25, 2023

Would there be a way to add an option on the icon menu to create a new window?

@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented May 25, 2023

Would there be a way to add an option on the icon menu to create a new window?

Other apps have this. But I don't know how we can achieve this with webview.

@chappjc
Copy link
Member

chappjc commented May 26, 2023

@ukane-philemon Would you mind rebasing and squashing down with a new commit message? Thanks.

@buck54321
Copy link
Member

Are you saying when running in the background, they can't open a window by clicking on the app's icon in the dock?

Yep. But the systray will be visible and users can open new windows from there.

This is going to cause a lot of confusion. There's got to be some signal we can work with to catch them clicking the launcher icon, no? Or does Mac just completely ignore it because of the systray entry?

@chappjc
Copy link
Member

chappjc commented May 26, 2023

Are you saying when running in the background, they can't open a window by clicking on the app's icon in the dock?

Yep. But the systray will be visible and users can open new windows from there.

This is going to cause a lot of confusion. There's got to be some signal we can work with to catch them clicking the launcher icon, no? Or does Mac just completely ignore it because of the systray entry?

I think @ukane-philemon is properly addressing this from #2372. This PR is primarily dealing with the dmg packaging.

Comment on lines +15 to +17
"scss/at-extend-no-missing-placeholder": null,
"declaration-block-no-redundant-longhand-properties": null,
"value-no-vendor-prefix": null
Copy link
Member

Choose a reason for hiding this comment

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

Why these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To allow for changes fixing #2333 (comment)

@@ -238,7 +238,10 @@ button.form-button {
@extend .flex-center;

position: fixed;
inset: 0;
Copy link
Member

Choose a reason for hiding this comment

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

Why undoing this? A linter update recently made us do this, and I don't think we should disable that linter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not supported on webkit. #2333 (comment)

Copy link
Member

@chappjc chappjc May 31, 2023

Choose a reason for hiding this comment

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

Oh, right. That's kinda messed up that webkit doesn't support inset. It is supported in every major browser, including Safari since v14.1: https://caniuse.com/mdn-css_properties_inset
What do we know about the webkit version provided by macdrive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m running Safari ver 15.6.1 but know nothing about the WebKit version provided by Macdrive. I think it will use whatever webkit version it finds.

To get Webkit Version on macOS run: defaults read /System/Library/Frameworks/WebKit.framework/Resources/Info.plist CFBundleShortVersionString I got 15609 which corresponds with Safari ver 15.6.1 according to the comment here

@@ -35,12 +35,12 @@
{{end}}
<div class="apply-bttn d-hide"><button class="bg2 demi">[[[apply]]]</button></div>
</div>
<div id="exportOrders" class="export-archived-records-bttn mt-3">
<div id="exportOrders" class="export-archived-records-bttn mt-3 pb-2">
Copy link
Member

Choose a reason for hiding this comment

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

Will you please comment on all these frontend changes? I didn't notice them in prior review, and I don't think they are relevant to the DMG package additions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The display didn't look right on webkit. See #2333 (comment).

fix macOS multiple dock icons

rebase changes

build webpack bundle always

fix webkit issue with unsupported inset property

fix webkit backdrop-filter on login form

fix css variable bug

fix webkit rendering of market list, wallet select btns, order btns, and notification tabs

remove redundant css class

fix minor css style padding and bump cache

martonp review changes

dexc-desktop: catch os.Interrupt signal and shutdown gracefully

Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented May 27, 2023

Installer view updated in aab9446:

EDIT: After the icon update in c6c1d5c
Screenshot 2023-06-01 at 2 55 34 PM

Screenshot 2023-05-31 at 10 55 53 PM Screenshot 2023-06-01 at 3 02 04 PM

Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
@buck54321
Copy link
Member

Notably the window is stuck on top like a modal. But I'm guessing #2372 will fix that.

@ukane-philemon
Copy link
Contributor Author

Notably the window is stuck on top like a modal.

Stuck? Can I see a video of what you mean?

@buck54321
Copy link
Member

Video

@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented Jun 1, 2023

Video

Seen and was able to reproduce with our current use of webview but this is resolved in #2372 since we are using native macOS APIs directly.

@buck54321 buck54321 merged commit 05ad13a into decred:master Jun 1, 2023
5 checks passed
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.

None yet

4 participants