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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9e02f6b
fix: Wraps alert display in Platform.runlater()
Feb 20, 2022
a2a94e7
fix: Adds an icon that reflects the available update status to the he…
Feb 20, 2022
9ce3afb
Improves UX with better menu item text
Feb 26, 2022
c7fe0da
Merge branch 'master' into issue-489
Feb 26, 2022
2b94b2f
Removing hi-res icons as they mess up the menu on Mac
Feb 26, 2022
4d14d8a
Removes unused icon.
paul-court Feb 27, 2022
ee65d35
Sets the OK image and message by default and only updates the menu it…
paul-court Feb 27, 2022
d3202ec
Removes unused import.
paul-court Feb 27, 2022
1fa4a33
Merge branch 'master' into issue-489
paul-court Apr 2, 2022
aed8b86
Replaces stack trace outputs with log messages.
paul-court May 8, 2022
91c5919
Alternate menu item label when there are no updates.
paul-court May 8, 2022
4e86eb4
Merge branch 'master' into issue-489
paul-court May 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions app/src/main/java/com/oracle/javafx/scenebuilder/app/SceneBuilderApp.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,6 @@ public boolean canPerformControlAction(ApplicationControlAction a, DocumentWindo
switch (a) {
case ABOUT:
case REGISTER:
case CHECK_UPDATES:
case NEW_FILE:
case NEW_TEMPLATE:
case OPEN_FILE:
Expand All @@ -235,6 +234,10 @@ public boolean canPerformControlAction(ApplicationControlAction a, DocumentWindo
result = true;
break;

case CHECK_UPDATES:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

result = AppSettings.isUpdateAvailable();
break;

case CLOSE_FRONT_WINDOW:
result = windowList.isEmpty() == false;
break;
Expand Down Expand Up @@ -993,11 +996,13 @@ private void checkUpdates(DocumentWindowController source) {
dialog.showAndWait();
});
} else {
SBAlert alert = new SBAlert(Alert.AlertType.INFORMATION, getFrontDocumentWindow().getStage());
alert.setTitle(I18N.getString("check_for_updates.alert.up_to_date.title"));
alert.setHeaderText(I18N.getString("check_for_updates.alert.headertext"));
alert.setContentText(I18N.getString("check_for_updates.alert.up_to_date.message"));
alert.showAndWait();
Platform.runLater(()->{
SBAlert alert = new SBAlert(Alert.AlertType.INFORMATION, getFrontDocumentWindow().getStage());
alert.setTitle(I18N.getString("check_for_updates.alert.up_to_date.title"));
alert.setHeaderText(I18N.getString("check_for_updates.alert.headertext"));
alert.setContentText(I18N.getString("check_for_updates.alert.up_to_date.message"));
alert.showAndWait();
});
}
} catch (NumberFormatException ex) {
Platform.runLater(() -> showVersionNumberFormatError(source));
Expand Down
23 changes: 23 additions & 0 deletions app/src/main/java/com/oracle/javafx/scenebuilder/app/menubar/MenuBarController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import com.oracle.javafx.scenebuilder.app.i18n.I18N;
import com.oracle.javafx.scenebuilder.app.preferences.PreferencesController;
import com.oracle.javafx.scenebuilder.app.preferences.PreferencesRecordGlobal;
import com.oracle.javafx.scenebuilder.app.util.AppSettings;
import com.oracle.javafx.scenebuilder.kit.editor.EditorController;
import com.oracle.javafx.scenebuilder.kit.editor.EditorController.ControlAction;
import com.oracle.javafx.scenebuilder.kit.editor.EditorController.EditAction;
Expand All @@ -56,6 +57,7 @@

import java.io.File;
import java.io.IOException;
import java.io.InputStream;
AlmasB marked this conversation as resolved.
Show resolved Hide resolved
import java.net.URL;
import java.util.ArrayList;
import java.util.Collections;
Expand All @@ -66,6 +68,7 @@
import java.util.TreeMap;
import java.util.TreeSet;

import javafx.application.Platform;
import javafx.beans.value.ChangeListener;
import javafx.collections.ObservableList;
import javafx.event.ActionEvent;
Expand All @@ -81,6 +84,8 @@
import javafx.scene.control.RadioMenuItem;
import javafx.scene.control.SeparatorMenuItem;
import javafx.scene.effect.Effect;
import javafx.scene.image.Image;
import javafx.scene.image.ImageView;
import javafx.scene.input.KeyCharacterCombination;
import javafx.scene.input.KeyCode;
import javafx.scene.input.KeyCodeCombination;
Expand Down Expand Up @@ -1171,6 +1176,24 @@ public String getTitle() {
insertMenu.setOnMenuValidation(onCustomPartOfInsertMenuValidationHandler);

windowMenu.setOnMenuValidation(onWindowMenuValidationHandler);

// Modify the Check Updates menu item if there is an update available.
// Icons by Font Awesome (https://fontawesome.com/license/free) under CC BY 4.0 License
AppSettings.getLatestVersion(latestVersion -> {
var updateAvailable = AppSettings.isUpdateAvailable();
Image icon = new Image(MenuBarController.class
.getResource(updateAvailable ? "download_icon.png" : "check_icon.png")
Oliver-Loeffler marked this conversation as resolved.
Show resolved Hide resolved
.toExternalForm());
ImageView iconView = new ImageView(icon);

Platform.runLater(() -> {
checkUpdatesMenuItem.setGraphic(iconView);
checkUpdatesMenuItem.disableProperty().setValue(!updateAvailable);
Oliver-Loeffler marked this conversation as resolved.
Show resolved Hide resolved
checkUpdatesMenuItem.setText(updateAvailable
? I18N.getString("menu.title.check.updates.available")
: I18N.getString("menu.title.check.updates.ok"));
});
});
}

private void addSwatchGraphic(RadioMenuItem swatchMenuItem) {
Expand Down
13 changes: 12 additions & 1 deletion app/src/main/java/com/oracle/javafx/scenebuilder/app/util/AppSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,9 @@ public class AppSettings {

private static String sceneBuilderVersion;
private static String latestVersion;

private static String latestVersionText;
private static String latestVersionAnnouncementURL;
private static boolean isUpdateAvailable = false;

private static final JsonReaderFactory readerFactory = Json.createReaderFactory(null);

Expand Down Expand Up @@ -114,6 +114,10 @@ public static boolean isCurrentVersionLowerThan(String version) {
return false;
}

public static boolean isUpdateAvailable() {
return isUpdateAvailable;
}

public static void getLatestVersion(Consumer<String> consumer) {

if (latestVersion == null) {
Expand All @@ -136,6 +140,13 @@ public static void getLatestVersion(Consumer<String> consumer) {
ex.printStackTrace();
}
latestVersion = onlineVersionNumber;

try {
isUpdateAvailable = isCurrentVersionLowerThan(latestVersion);
} catch (NumberFormatException e) {
e.printStackTrace();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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 was just following what the rest of the code in the block was doing. e.printStackTrace() seems to be used quite a lot.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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 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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

}

consumer.accept(latestVersion);
}, "GetLatestVersion").start();
} else {
Expand Down
5 changes: 4 additions & 1 deletion app/src/main/resources/com/oracle/javafx/scenebuilder/app/i18n/SceneBuilderApp.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,10 @@ 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 = Checking for updates...
menu.title.check.updates.available = Update available
menu.title.check.updates.ok = All up-to-date!
Copy link
Collaborator

Choose a reason for hiding this comment

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

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:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about "No updates available"?

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


menu.title.about = About Scene Builder
menu.title.register = Register...
menu.title.help.javafx=JavaFX
Expand Down
Binary file added app/src/main/resources/com/oracle/javafx/scenebuilder/app/menubar/check_icon.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...src/main/resources/com/oracle/javafx/scenebuilder/app/menubar/download_icon.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added app/src/main/resources/com/oracle/javafx/scenebuilder/app/menubar/warn_icon.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.