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

Show notification after running macro #3269

Merged
merged 11 commits into from Nov 13, 2023
Merged

Conversation

FliegendeWurst
Copy link
Member

@FliegendeWurst FliegendeWurst commented Sep 3, 2023

This PR adds a notification that will show up after a macro is done. Depending on the settings it will always / when the KeY window is not focused / never show up.

OS (Desktop Environment)
Windows screenshot-notification-windows
MacOS screenshot-notification-macos (claims it comes from the app "Script Editor")
Linux (KDE) image
Linux (Gnome) screenshot-notification-gnome
Linux (XFCE) screenshot-notification-xfce
Linux (Cinnamon) screenshot-notification-cinnamon

The view settings panel has also been restructured.

Screenshot_20230922_090859

@FliegendeWurst FliegendeWurst added GUI Feature New feature or request Review Request Waiting for review labels Sep 3, 2023
@codecov
Copy link

codecov bot commented Sep 3, 2023

Codecov Report

Merging #3269 (b7c549d) into main (ae6efac) will decrease coverage by 0.01%.
The diff coverage is 15.78%.

@@             Coverage Diff              @@
##               main    #3269      +/-   ##
============================================
- Coverage     37.84%   37.84%   -0.01%     
  Complexity    16895    16895              
============================================
  Files          2054     2054              
  Lines        125490   125494       +4     
  Branches      21227    21228       +1     
============================================
+ Hits          47495    47496       +1     
- Misses        72142    72145       +3     
  Partials       5853     5853              
Files Coverage Δ
...ain/java/de/uka/ilkd/key/macros/TryCloseMacro.java 11.11% <ø> (ø)
.../ilkd/key/prover/impl/DefaultTaskFinishedInfo.java 26.66% <ø> (ø)
...a/de/uka/ilkd/key/macros/SequentialProofMacro.java 6.25% <0.00%> (ø)
...in/java/de/uka/ilkd/key/settings/ViewSettings.java 37.58% <25.00%> (-0.76%) ⬇️
...de/uka/ilkd/key/macros/ProofMacroFinishedInfo.java 14.28% <10.00%> (-0.81%) ⬇️

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@github-actions
Copy link

github-actions bot commented Sep 3, 2023

Thank you for your contribution.

The test artifacts are available on Artiweb.
The newest artifact is here.

@FliegendeWurst FliegendeWurst removed the Review Request Waiting for review label Sep 4, 2023
@FliegendeWurst
Copy link
Member Author

FliegendeWurst commented Sep 14, 2023

I just tried this PR on Windows 10. For some reason the notification only appears in the notification center, not on the screen. Making this whole feature almost completely useless...

@WolframPfeifer
Copy link
Member

WolframPfeifer commented Sep 20, 2023

I just tried this PR on Windows. For some reason the notification only appears in the notification center, not on the screen. Making this whole feature almost completely useless...

On my Windows 11 machine, it looks fine:
screenshot-notification-windows
Two notes though:

  • It would be better to show "KeY" instead of "OpenJDK Platform binary", but I think it is impossible with this API.
  • The notification is shown in the notification center as long as it is displayed, afterwards is disappears from the notification center. This seems to be a recent bug in Windows (https://bugs.openjdk.org/browse/JDK-8315647).

@FliegendeWurst
Copy link
Member Author

It would be better to show "KeY" instead of "OpenJDK Platform binary", but I think it is impossible with this API.

I have checked this out and it appears the only way is to remove that text completely. That is what I've done in my latest commit. It should still show the icon though.

Implement a MacOS version of the notification (maybe Richard @unp1 can test it).

The MacOS version is ready for testing now.

@WolframPfeifer
Copy link
Member

I did some quick tests with different desktop environments. Seems to look fine everywhere. I added the screenshots in the PR description.

@WolframPfeifer WolframPfeifer added the Reviewer Feedback Feedback from the review needs to be addressed label Sep 21, 2023
@FliegendeWurst FliegendeWurst added this to the v2.14.0 milestone Sep 22, 2023
@FliegendeWurst FliegendeWurst added Review Request Waiting for review and removed Reviewer Feedback Feedback from the review needs to be addressed labels Sep 23, 2023
@WolframPfeifer
Copy link
Member

@unp1 Could you please check that displaying a notification works on macOS? I think that nobody else has a Mac and can test it.

@unp1
Copy link
Member

unp1 commented Nov 10, 2023

@unp1 Could you please check that displaying a notification works on macOS? I think that nobody else has a Mac and can test it.

Just checked it. The setting is on always, but notifications do show up. We can look at it together in/after the KeY meeting.

@unp1
Copy link
Member

unp1 commented Nov 10, 2023

@unp1 Could you please check that displaying a notification works on macOS? I think that nobody else has a Mac and can test it.

Just checked it. The setting is on always, but notifications do show up. We can look at it together in/after the KeY meeting.

OK, first good news it works (it does not show a notification if the macro managed to close the proof, which makes sense).

One problem is that macOS thinks the notification comes from its ScriptEditor. This has two minor issues (1) the user thinks it comes from a different program and (2) if the ScriptEditor is not allowed to send notification, the user will not see them for KeY either.

In principle it seems to be possible (according to apples documentation) to pass an icon to osascript, but as there are several discussions that say it is not possible, I assume it has been added lately. I suggest we live with the Script Editor icon for the moment. Alternatively, the provided Java only alternative works as well with the following drawbacks:

  1. it seems as one can only allow all or no Java application to send notification
  2. the icon used is a rather ugly terminal/java icon.

@FliegendeWurst
Copy link
Member Author

(1) the user thinks it comes from a different program

Almost the same problem applies to the Windows notification, which comes from the "OpenJDK Platform Binary". At least we can set the icon on Windows.

I have changed the default to "when KeY is not focused" as discussed.

@unp1 unp1 added this pull request to the merge queue Nov 13, 2023
Merged via the queue into KeYProject:main with commit e978aed Nov 13, 2023
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature or request GUI Review Request Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants