feat: Added menu item icon to show whether updates are available #519

paul-court · 2022-02-20T16:25:56Z

Issue

Fixes #489

Adds an icon to the "Help -> Check for Update..." menu and displays in one of the following states:-

All OK and up-to-date:-

Update is available:-

Exception thrown in the existing version checking code:-

Progress

Change must not contain extraneous whitespace
License header year is updated, if required
The PR name must follow the pre-defined format
Verify the contributor has signed Gluon Individual Contributor License Agreement (CLA)

…lp menu

AlmasB · 2022-02-20T16:45:53Z

Looks great!

It seems there are 3 cases: OK, update available, exception. I think we can simplify this for the user:

case exception: (the user doesn't need to know there is an error, so just fall through to case OK)
case OK: we should show OK icon and say "All up-to-date!" - also disable this button since no need to click.

case update available: we should show download icon and say "Update available..."

Is there any way we can do this in FXML? Perhaps add both of these buttons and then make them visible / invisible as needed from code.

paul-court · 2022-02-20T18:04:00Z

I'm not sure about doing it all from the FXML, I might need some help with that.

It looks like the existing logic handles this with the SceneBuilderApp::canPerformControlAction() method. By getting the switch to return false for CHECK_UPDATES , the menu item becomes disabled:-

If I were to change this one to being two menu items that are shown/hidden, it would make it the odd one out.

I can easily add the same if check for the version to this switch statement, but I'll have a stab at refactoring a little bit (rule of thirds and all that).

AlmasB · 2022-02-21T08:27:12Z

You could go with "All OK" in FXML with the appropriate icon. Then, in code, check for updates and overwrite if an update is available.

Let me know if you need help with the FXML part of it.

jperedadnr · 2022-02-25T13:10:24Z

Doing a quick test on Mac:

Seems like iconView.setFitHeight, iconView.setFitWidth don't have any effect, and imageView uses the size of the icon (which works fine if the menubar is not system menu).

paul-court · 2022-02-25T13:28:54Z

Oh, thanks @jperedadnr. I'll have a play round.

jperedadnr · 2022-02-25T14:13:37Z

Seems that GlassSystemMenu::getPixels doesn't check other than the url of the image to get the pixels down to native.
So you'll need to provide a smaller size for the icons?

AlmasB · 2022-02-25T16:21:42Z

My original proposal was to use something like new red Circle(), which is visible when there is an update, otherwise not. Both for the update item and the "Help" menu title. I wonder if that would simplify this issue.

jperedadnr · 2022-02-25T16:25:40Z

That won't work for Mac, see https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/GlassSystemMenu.java#L293: only ImageView...

AlmasB · 2022-02-25T16:31:18Z

In which case, for this PR, we could ditch the visual indicator and do:

If update available, the menu item text is "Update available..."
Else, the menu item text is "Up to date" and greyed out (disabled).

Later, once we have a notification service of some sort like a non-modal pop-up in the top-right corner or something, we can make use of that to notify the update is available.

paul-court · 2022-02-26T13:50:46Z

Thanks for all the feedback.

@AlmasB I agree with the simpler UX for the user, so I removed the exception state, and the default if anything goes wrong is to show the OK message, as you suggested.

However, when simulating a delay in the thread that goes off and grabs the version number from the web, I felt some kind of user feedback that something was happening in the background would be better. So I added an initial state of "Checking for update...". This is what it looks likes with a forced 10s delay (sorry about the washed out gif colours):-

Since there is now no image for the default state, I've not added anything else to the FXML. If you still want to experiment with that, then I'll have to take you up on your offer for some FXML assistance because I couldn't figure that bit out.

@jperedadnr I found an article from Dirk Lemmermann and updated the images and code accordingly. However the same oversized icon was rendered, so this is deffo a bug in the Mac code as it correctly identified a high-res system and loaded [email protected] but then still rendered it at double the size. So I have just removed the high-res versions for now.

This is how it looks on Mac:-

And on Linux:-

And on Linux when there's an update available:-

paul-court · 2022-02-27T11:53:25Z

FYI: I think I fixed the icon rendering issue in openjfx:- openjdk/jfx#743. So hopefully we can put some high res icons back in play at some point in the future (if/when that PR gets merged).

AlmasB · 2022-02-27T12:42:41Z

app/src/main/java/com/oracle/javafx/scenebuilder/app/SceneBuilderApp.java

@@ -235,6 +234,10 @@ public boolean canPerformControlAction(ApplicationControlAction a, DocumentWindo
                result = true;
                break;

+            case CHECK_UPDATES:


Do we need to make this change at all? If the update is not available, the item is disabled, so not clickable. I think the existing behaviour is fine here.

Yes. The checks are performed every time you open the menu. Even with disabling the menu item in the FXML file, if we leave this hard coded to a true response, it will re-enable the item when you open the menu.

app/src/main/java/com/oracle/javafx/scenebuilder/app/menubar/MenuBarController.java

…em when there is an available update.

app/src/main/java/com/oracle/javafx/scenebuilder/app/menubar/MenuBarController.java

AlmasB

Looks good, thanks.

@jperedadnr did you want to have another look at this? If not, we are good to go.

jperedadnr · 2022-02-28T09:19:26Z

app/src/main/resources/com/oracle/javafx/scenebuilder/app/i18n/SceneBuilderApp.properties

@@ -191,7 +191,9 @@ menu.title.no.window = No Window
 # Help menu items
 menu.title.scene.builder.help = Scene Builder Help
 menu.title.show.welcome = Show Welcome Page
-menu.title.check.updates = Check for Update...
+menu.title.check.updates.available = Update available
+menu.title.check.updates.ok = All up-to-date!


This text out of context can mean many things. We need a better wording like "Updates: Scene Builder is up-to-date", which is longer, but I see that the menu is already wider than expected:

How about "No updates available"?

Also, menu doesn't have that extra space on Linux 🤷

jperedadnr · 2022-02-28T09:23:29Z

app/src/main/java/com/oracle/javafx/scenebuilder/app/menubar/MenuBarController.java

+
+                Platform.runLater(() -> {
+                    checkUpdatesMenuItem.setGraphic(iconView);
+                    checkUpdatesMenuItem.disableProperty().setValue(false);


Does it make sense to disable this menu option?

In case the user really wanted to be sure, clicking on it should at least bring a dialog saying that the current version XXX is already up-to-date, which is what we already do now?

jperedadnr · 2022-02-28T09:24:48Z

app/src/main/java/com/oracle/javafx/scenebuilder/app/util/AppSettings.java

+                try {
+                    isUpdateAvailable = isCurrentVersionLowerThan(latestVersion);
+                } catch (NumberFormatException e) {
+                    e.printStackTrace();


Move this to Logger.logSevere ? We shouldn't print stacktraces to console as they get lost unless you run from command line.

I was just following what the rest of the code in the block was doing. e.printStackTrace() seems to be used quite a lot.

I understand, but we have logs for a reason, and that's how new PRs should be tackling exceptions.

I did a search of how Logging is being done elsewhere. Seems we have two patterns:-

Logger.getLogger(AppSettings.class.getName()).log(Level.SEVERE, "An exception was thrown:", ex);

or

Logger.getLogger(PreferencesRecordDocument.class.getName()).log(Level.SEVERE, null, ex);

With the second one seeming to be a bit more common. Any recommentation on which to use?

I'm assuming from the output that it will be clear its an exception, making the "An exception was thrown:" text redundant?

The String message should normally include some useful information. So in the above case it might say: "Cannot parse version string" or something similar.

jperedadnr · 2022-02-28T09:27:43Z

@AlmasB I've added some comments.
@Gargoyle Looks good, it's also great that you created the OpenJFX PR.

Oliver-Loeffler · 2024-08-26T19:37:33Z

@Gargoyle Would you mind to have a look at the conflicts? May be we can get this one eventually merged. It technically looks fine to me.

paul-court · 2024-09-27T20:14:15Z

I had a quick look this evening and got myself into a rebase nightmare. 🙄

I've hardly looked at SceneBuilder (or even done much JavaFX) in the last two years, so what will probably be best is that I'll see if I can find some time in the next week or so to get back into it a bit, and just re-implement this on a fresh branch cut from the latest main/master.

My fix for MacOS icon rendering did make it into JavaFX 20, so if SceneBuilder is targeting that or later as a min requirement, I can redo the high res versions of the icons too?

Will need to pair up with someone though, as I don't even have my old Mac to check even basic stuff anymore.

Oliver-Loeffler · 2024-09-27T21:23:21Z

Hi Paul, I still have my old MacBook so I can help out there. Very cool that you'checked in Here. Thanks!

Paul Court added 2 commits February 20, 2022 11:21

fix: Wraps alert display in Platform.runlater()

9e02f6b

fix: Adds an icon that reflects the available update status to the he…

a2a94e7

…lp menu

Paul Court added 3 commits February 26, 2022 12:12

Improves UX with better menu item text

9ce3afb

Merge branch 'master' into issue-489

c7fe0da

Removing hi-res icons as they mess up the menu on Mac

2b94b2f

AlmasB reviewed Feb 27, 2022

View reviewed changes

paul-court added 2 commits February 27, 2022 13:40

Removes unused icon.

4d14d8a

Sets the OK image and message by default and only updates the menu it…

ee65d35

…em when there is an available update.

AlmasB reviewed Feb 27, 2022

View reviewed changes

app/src/main/java/com/oracle/javafx/scenebuilder/app/menubar/MenuBarController.java Outdated Show resolved Hide resolved

Removes unused import.

d3202ec

AlmasB previously approved these changes Feb 27, 2022

View reviewed changes

AlmasB changed the title ~~fix: Adds status icon to the Check for Updates menu item. Fixes #489~~ feat: Added menu item icon to show whether updates are available Feb 27, 2022

jperedadnr reviewed Feb 28, 2022

View reviewed changes

paul-court added 3 commits April 2, 2022 11:43

Merge branch 'master' into issue-489

1fa4a33

Replaces stack trace outputs with log messages.

aed8b86

Alternate menu item label when there are no updates.

91c5919

paul-court dismissed AlmasB’s stale review via 91c5919 May 8, 2022 06:18

Merge branch 'master' into issue-489

4e86eb4

Oliver-Loeffler added the enhancement New feature or request label Aug 26, 2024

Oliver-Loeffler added this to the 23 milestone Aug 26, 2024

Oliver-Loeffler requested review from AlmasB and jperedadnr August 26, 2024 19:38

Oliver-Loeffler modified the milestones: 23, 24 Sep 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Added menu item icon to show whether updates are available #519

feat: Added menu item icon to show whether updates are available #519

paul-court commented Feb 20, 2022 •

edited

Loading

AlmasB commented Feb 20, 2022 •

edited

Loading

paul-court commented Feb 20, 2022

AlmasB commented Feb 21, 2022

jperedadnr commented Feb 25, 2022 •

edited

Loading

paul-court commented Feb 25, 2022

jperedadnr commented Feb 25, 2022

AlmasB commented Feb 25, 2022

jperedadnr commented Feb 25, 2022

AlmasB commented Feb 25, 2022

paul-court commented Feb 26, 2022

paul-court commented Feb 27, 2022

AlmasB Feb 27, 2022

paul-court Feb 27, 2022

AlmasB left a comment

jperedadnr Feb 28, 2022

paul-court May 8, 2022

jperedadnr Feb 28, 2022

jperedadnr Feb 28, 2022

paul-court Feb 28, 2022

jperedadnr Feb 28, 2022

paul-court Apr 2, 2022

AlmasB Apr 3, 2022

jperedadnr commented Feb 28, 2022

Oliver-Loeffler commented Aug 26, 2024

paul-court commented Sep 27, 2024

Oliver-Loeffler commented Sep 27, 2024 via email •

edited

Loading

feat: Added menu item icon to show whether updates are available #519

Are you sure you want to change the base?

feat: Added menu item icon to show whether updates are available #519

Conversation

paul-court commented Feb 20, 2022 • edited Loading

Issue

Progress

AlmasB commented Feb 20, 2022 • edited Loading

paul-court commented Feb 20, 2022

AlmasB commented Feb 21, 2022

jperedadnr commented Feb 25, 2022 • edited Loading

paul-court commented Feb 25, 2022

jperedadnr commented Feb 25, 2022

AlmasB commented Feb 25, 2022

jperedadnr commented Feb 25, 2022

AlmasB commented Feb 25, 2022

paul-court commented Feb 26, 2022

paul-court commented Feb 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlmasB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jperedadnr commented Feb 28, 2022

Oliver-Loeffler commented Aug 26, 2024

paul-court commented Sep 27, 2024

Oliver-Loeffler commented Sep 27, 2024 via email • edited Loading

paul-court commented Feb 20, 2022 •

edited

Loading

AlmasB commented Feb 20, 2022 •

edited

Loading

jperedadnr commented Feb 25, 2022 •

edited

Loading

Oliver-Loeffler commented Sep 27, 2024 via email •

edited

Loading