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

Check if OS notifications work correctly on Windows #1411

Closed
HenrikJannsen opened this issue Nov 24, 2023 · 14 comments · Fixed by #1573
Closed

Check if OS notifications work correctly on Windows #1411

HenrikJannsen opened this issue Nov 24, 2023 · 14 comments · Fixed by #1573
Assignees
Labels
Milestone

Comments

@HenrikJannsen
Copy link
Contributor

On Windows we use AwtNotifications. We should figure out if there is a native solution which works well on all commonly used Windows versions. If not feasible we need to check if AwtNotifications works sufficiently well.

@HenrikJannsen HenrikJannsen added this to the 2.0.0 milestone Nov 24, 2023
@mrblue313
Copy link
Contributor

I can look into this.

@HenrikJannsen
Copy link
Contributor Author

Depends on #1383

@HenrikJannsen
Copy link
Contributor Author

@mrblue313 Any update on that task?

@mrblue313
Copy link
Contributor

I am working on it. Will give an update in the following days.

@mrblue313
Copy link
Contributor

Giving an update on the current status of this.

Testing the current solution

I have been testing using Clear-net and Windows 11.

The following matrix was considered:

  • Private/public channels
  • @mentions
  • User being online/offline
  • Edit/delete messages
  • Notification options (all/@mention/off)
  • Windows notification system

These were my findings:

  • Clicking on the notification only dismisses it. We could potentially pass an action here and allow navigation. Alternatively, we could improve the navigation path. For instance, when receiving a message in "Conferences", it could say "Events > Conferences". At least when testing it doesn't feel straightforward where the message comes from.

image

For instance, in the case of the offerbook, this is what it displays (not very indicative):

image

  • "Java(TM) Platform SE binary" appears as the title, since it is the default description of the Java application process running the code. I would like to test this with the binary as well.

image

Same when you click on the three dots icon:

image

And in the system settings:

image

  • The body/message gets truncated very early. Perhaps an improvement could be to ensure those are shorter for fixed messages like publishing offers.

image

  • Notifications do show in the Windows notification panel:

image

... and even the notification bell lights up!

image

Other things I have noticed (outside the notification framework layer, but still find worth mentioning):

  • When a message is edited, users get notified about it again. Is this by design?
  • If you choose "Ignore user", all their messages disappear everywhere. Seems like this option should be given to the user a bit more carefully, possibly involving two clicks and explaining what this means and how to undo. While testing, I was just expecting not to see this user's notifications.

To compare, this is an example of how notifications show in Element. They have the user image, longer text, etc.

image

Investigation

Suggested next steps

I suggest I test again when the binary is ready to be generated and revaluate the situation. But if we are able to set the app name correctly + image, and with some enhancements on the title/message that we pass adhering to the constraints seen, I would continue using AWT.

Another thing I am concerned about is that Windows Toast Notifications is only available from Windows 10, so if we want to ensure compatibility with any Windows version, that would still not be a good solution.

@HenrikJannsen
Copy link
Contributor Author

Events > Conferences

Yes agree.

I would like to test this with the binary as well.

It should work now to build a binary for windows.
./gradlew :desktop:desktop-app-launcher:generateInstallers
Then the Bisq icon and "Bisq" should be shown.

The body/message gets truncated very early.

Yes we should increase maxLenght

When a message is edited, users get notified about it again. Is this by design?

Not by design. Probably better to not send a notification in that case, but low prio to fix IMO.

If you choose "Ignore user", all their messages disappear everywhere. Seems like this option should be given to the user a bit more carefully, possibly involving two clicks and explaining what this means and how to undo. While testing, I was just expecting not to see this user's notifications.

We could show a popup (with dont show again) with more info. When un-ignore all msg are shown again. Its just a local filtering.

..user image...

That would be nice but I think thats a challenging task to get on all OS working. You could add an issue as a future feature request.

Thanks for all the testing. Can you try if you can get a windows binary running to see if icon / name works as expected?
And can you make a PR for the minor changes with lenngth and nav path improvements? A util for getting a display string from the nav path would be good to be reused in other app-scope areas (and supported by translations). We have all the nav paths, so its just to combine those where we want to show the parent path.

@mrblue313
Copy link
Contributor

Sure, I'll get back with this.

@mrblue313
Copy link
Contributor

mrblue313 commented Dec 21, 2023

I have tested with the binary. Presenting the findings.

After generating the binary, I am taking the installer from:
bisq2\apps\desktop\desktop-app-launcher\build\packaging\jpackage\temp_exe\images

It looks like the content in the folders inside images is swapped? in the win-exe.image is the .msi and in win-msi.image is the .exe.

I have tried both of them. The Bisq 2.exe seems to be a portable version. However, non of them show the image correctly in the notification. Besides that, as application name when receiving the notification, description is being picked up instead of name:

"--description", "A decentralized bitcoin exchange network.",

This only happens in the notification panel, the rest of places get "Bisq2".

image

I have also noticed that we are using a different (old) version of the logo in the task bar. This looks inconsistent with the one appearing in the process window, which is the newer one. I can update those icons in a PR soon.

image

Not sure if related to our *.msi in particular, but the installation using the Windows installer works really bad. I am not able to close the window, it seems to be hung up and need to kill the process once the app has been installed.

One more thing to consider, and that I have noticed, (independent from the build itself) is the flushing of messages when the user connects/starts up the app. At the moment, the user needs to wait/dismiss for every single notification. If the messages are more than 3 o 4, that might not feel like a good experience. I have created an issue for this #1546.

@HenrikJannsen
Copy link
Contributor Author

@mrblue313
I assume you have build the installer with ./gradlew :desktop:desktop-app-launcher:generateInstallers, right?

Can you try to fix the issues with the image and title? If no solution found @alvasw might have an idea.

@mrblue313
Copy link
Contributor

I assume you have build the installer with ./gradlew :desktop:desktop-app-launcher:generateInstallers, right?

Yes, that's what I used.

Can you try to fix the issues with the image and title?

Sure, I will continue looking into it.

@alvasw
Copy link
Contributor

alvasw commented Dec 29, 2023

Do you know when did it worked last (with the installed binary)? That would help to identify the commit that broke the notifications.

@mrblue313
Copy link
Contributor

Do you know when did it worked last (with the installed binary)? That would help to identify the commit that broke the notifications.

I have never seen it working before. The first time I tested it was around the 16th of Dec.

@alvasw
Copy link
Contributor

alvasw commented Dec 31, 2023

Do you know when did it worked last (with the installed binary)? That would help to identify the commit that broke the notifications.

I have never seen it working before. The first time I tested it was around the 16th of Dec.

Oh my bad, I thought it worked before we updated the packaging module. @HenrikJannsen Have the notifications ever worked on Windows? I haven't read or worked on notification code before.

@HenrikJannsen
Copy link
Contributor Author

I never managed to build a binary for Windows. So I could not test it with a binary to see if the icon and app name is correct.
When running from source the notifications itself worked but showed the java icon.

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

Successfully merging a pull request may close this issue.

3 participants