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

Remove dependency to jgoodies-looks #3458

Merged
merged 8 commits into from
Jan 2, 2018
Merged

Remove dependency to jgoodies-looks #3458

merged 8 commits into from
Jan 2, 2018

Conversation

lenhard
Copy link
Member

@lenhard lenhard commented Nov 25, 2017 •
edited
Loading

Jgoodies-looks uses internal APIs (see #2594 (comment) ), which means that it causes problems with Java 9. This PR removes this dependency. The biggest change is that two Look and Feels go and that particularily affects Linux users.

I have used the Nimbus Look and Feel as the new default for Linux, but we can change that or even search for better Java 9-compatible Look and Feels. I have also added a preferences migration in case a user has set one of the incompatible Look and Feels.

I would suggest that we wait with merging this after we have release 4.1. Java 9 does not yet matter for 4.1 and we should give the Linux users a stable JabRef that still has the old Look and Feels before moving on.

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

@lenhard lenhard added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Nov 25, 2017
@lenhard lenhard added this to the v4.2 milestone Nov 25, 2017
Siedlerchr
Siedlerchr reviewed Nov 27, 2017
View reviewed changes
src/main/java/org/jabref/JabRefGUI.java Outdated
MetalLookAndFeel.setCurrentTheme(new SkyBluer());
com.jgoodies.looks.Options.setPopupDropShadowEnabled(true);
UIManager.setLookAndFeel(lnf);
UIManager.setLookAndFeel(new NimbusLookAndFeel());
Copy link
Member

Choose a reason for hiding this comment

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

you should set the LookAndFeel based on className, as recommend by the jdk api:
UIManager.setLookAndFeel("javax.swing.plaf.metal.MetalLookAndFeel");

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing out! Changed it.

tobiasdiez
tobiasdiez approved these changes Nov 27, 2017
View reviewed changes
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

The code looks good to me. I have one suggestion that you might want to consider.

src/main/java/org/jabref/gui/preftabs/AppearancePrefsTab.java Outdated
@@ -60,8 +60,6 @@
public static Set<String> getAvailableLookAndFeels() {

Set<String> lookAndFeels = new HashSet<>();
lookAndFeels.add("com.jgoodies.looks.plastic.Plastic3DLookAndFeel");
lookAndFeels.add("com.jgoodies.looks.windows.WindowsLookAndFeel");

for (LookAndFeelInfo info : UIManager.getInstalledLookAndFeels()) {
Copy link
Member

Choose a reason for hiding this comment

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

I think the rest of the method can now be replaced by the one-liner return getInstalledLookAndFeels().stream().map(LookAndFeelInfo:getClassName).collect(toSet) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not exactly (it's and array), but almost: Arrays.stream(UIManager.getInstalledLookAndFeels()).map(LookAndFeelInfo::getClassName).collect(Collectors.toSet())

@koppor
Copy link
Member

koppor commented Nov 30, 2017

Can't incorporating this new L&F in 4.1 give us a huge user-base to test? 4.1 will be a release with christmas colors and then we will release 4.2 somewhere around 6th january with the default colors again. - Hoping that this works with the new color configuration ^^.

@koppor
Copy link
Member

koppor commented Nov 30, 2017

This is how JabRef looks after upgrading:

grafik

@LinusDietz could you provide a Linux screenshot?

@tobiasdiez
Copy link
Member

To be honest, I wouldn't spend to much time on swing look and feels. They disappear as soon as we finish migrating (most) of the ui to JavaFX.

koppor
koppor approved these changes Nov 30, 2017
View reviewed changes
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Yeah, no issues on Windows. Seems we have to wait for a Screenshot of a Linux person and then we can merge it!

src/main/java/org/jabref/preferences/JabRefPreferences.java
@@ -466,12 +466,12 @@ private JabRefPreferences() {
defaults.put(WIN_LOOK_AND_FEEL, UIManager.getSystemLookAndFeelClassName());
defaults.put(EMACS_PATH, "emacsclient");
} else if (OS.WINDOWS) {
defaults.put(WIN_LOOK_AND_FEEL, "com.jgoodies.looks.windows.WindowsLookAndFeel");
defaults.put(WIN_LOOK_AND_FEEL, "com.sun.java.swing.plaf.windows.WindowsLookAndFeel");
Copy link
Member

Choose a reason for hiding this comment

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

According to https://bugs.openjdk.java.net/browse/JDK-8136366, this is a non-public L&F. But reading along, it seems, we are "correctly" using it...

@lenhard
Copy link
Member Author

lenhard commented Nov 30, 2017

@koppor: For me JabRef looked like this all the time ;-) I have been using the windows landf all along. The big difference in look might rather be on Linux.

@Siedlerchr
Copy link
Member

Okay, here are some linux screenshots:
Xubuntu 16.10, Theme: Greybird
Plastic Look and Feel - jgoodies (seems like the same as metal?)
linuxplasticxubuntu

Nimbus L&F
linuxnimbusxubuntu

Metal L&F
linuxmetalxubuntu

GTK L&F (broken)
linuxgtkxubuntu

@koppor koppor added status: devcall and removed status: devcall status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Dec 5, 2017
@lenhard
Copy link
Member Author

lenhard commented Jan 2, 2018

Since 4.1 is released, we can merge this as agreed.

@lenhard lenhard merged commit f0aad59 into master Jan 2, 2018
@lenhard lenhard deleted the remove-jgoodies-looks branch January 2, 2018 12:47
Siedlerchr added a commit that referenced this pull request Jan 2, 2018
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into selectFilesDlg
d4aed9b 
* upstream/master:
  Remove dependency to jgoodies-looks (#3458)
  Add simple gui test (#3399)
  Library properties: no change but.. (issue #3562) (#3579)
  Initialize previe before MainPanel to prevent NPEs on delete Fixes #3584
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Siedlerchr Siedlerchr Siedlerchr left review comments

@tobiasdiez tobiasdiez tobiasdiez approved these changes

@koppor koppor koppor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v4.2
Development

Successfully merging this pull request may close these issues.
4 participants
@lenhard @koppor @tobiasdiez @Siedlerchr