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

Fix Look and Feel related issues #4002

Merged
merged 9 commits into from
May 4, 2018
Merged

Fix Look and Feel related issues #4002

merged 9 commits into from
May 4, 2018

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented May 1, 2018

Enabled the change of the L&F for mac.
Remove GTK L&F
Fix migration of old jgoodies l&f

Fixes #3986
Fixes #3995


  • Change in CHANGELOG.md described
  • Tests created for changes
  • Manually tested changed features in running JabRef
  • Screenshots added in PR description (for bigger UI changes)
  • Ensured that the git commit message is a good one
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

remove gtk l&f from style selection
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 1, 2018
@Siedlerchr Siedlerchr changed the title Remove gtk Remove gtk L&F from settings options May 1, 2018
@Siedlerchr Siedlerchr changed the title Remove gtk L&F from settings options Remove gtk L&F from settings options and fix font setting May 1, 2018
@Siedlerchr Siedlerchr changed the title Remove gtk L&F from settings options and fix font setting Fix Lool and Feel related issues May 1, 2018
@Siedlerchr Siedlerchr changed the title Fix Lool and Feel related issues Fix Look and Feel related issues May 1, 2018
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 changes look good to me, except one if statement that I don't understand.

Could you please also remove all L&F related code on the maintable-branch. Since now the main display uses JavaFX, there is no need to make the look of the remaining swing dialogs customizable. Especially, if we sometimes run in problems. Just choose a suitable default (Nimbus & plaf.Win ?).

&& !System.getProperty("java.runtime.name").contains("OpenJDK")) {
// try to avoid ending up with the ugly Metal L&F
UIManager.setLookAndFeel("javax.swing.plaf.nimbus.NimbusLookAndFeel");
if (UIManager.getCrossPlatformLookAndFeelClassName().equals(lookFeel) && !GTK_LF_CLASSNAME.equals(lookFeel)) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the second test. I thought we want to prevent the GTK L&F by all means....

Copy link
Member Author

Choose a reason for hiding this comment

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

You missed the not. We only go with the System look and feel if it's not GTk

Furthermore the migration replaces the stored (if gtk) look and feel.
So we are on the safe due

Copy link
Member

Choose a reason for hiding this comment

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

ok now I don't understand the complete if statement (and think we can simply delete it).

cases:

  • system: gtk -> lookFeel = Nimbus, if = false
  • system: win -> lookFeel = win, if = true (why do we set the l&f to nimbus?!)
  • system: win, WIN_LOOK_AND_FEEL: win -> lookFeel = win, if = true (again wrong)
  • system: gtk, WIN_LOOK_AND_FEEL: gtk -> lookFeel = gtk, if = false (but we actually wanted to prevent setting it to gtk?!)
  • system: win, WIN_LOOK_AND_FEEL: gtk -> lookFeel = gtk, if = false (again, we don't want gtk)

If I understand you correctly, WIN_LOOK_AND_FEEL = gtk can never happen because of the migration, right? In this case we can ignore the last two cases, but even in the first three cases the behavior is odd.

Copy link
Member Author

Choose a reason for hiding this comment

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

I simplified the condition. Now the cases should be more clear.

* upstream/master:
  Fix statement in changelog
  [WIP] Add Text File Export for "Find Unlinked Files" (#3979)
} else {
lookFeel = systemLookFeel;
}
} else {
lookFeel = Globals.prefs.get(JabRefPreferences.WIN_LOOK_AND_FEEL);
}

if (UIManager.getCrossPlatformLookAndFeelClassName().equals(lookFeel) && !GTK_LF_CLASSNAME.equals(lookFeel)) {
//Prevent metal l&f
Copy link
Member Author

Choose a reason for hiding this comment

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

use Default L&F:

  1. System L&F = gtk => Nimbus
  2. System L&F = win => win
  3. System L&F = metal look and feel

Check cross platform

  1. Metal look and feel => Nimbus

@Siedlerchr
Copy link
Member Author

I merge this now, and I will create a PR to remove that L&F stuff completely in maintable-beta

@Siedlerchr Siedlerchr merged commit d650b26 into master May 4, 2018
@Siedlerchr Siedlerchr deleted the removeGTK branch May 4, 2018 16:20
Siedlerchr added a commit that referenced this pull request May 4, 2018
* upstream/master:
  New translations JabRef_en.properties (French) (#4009)
  Fix Look and Feel related issues (#4002)
  Fix statement in changelog
  [WIP] Add Text File Export for "Find Unlinked Files" (#3979)

# Conflicts:
#	src/main/java/org/jabref/JabRefGUI.java
#	src/main/java/org/jabref/gui/preftabs/AppearancePrefsTab.java
#	src/main/java/org/jabref/migrations/PreferencesMigrations.java
Siedlerchr added a commit that referenced this pull request May 5, 2018
* upstream/maintable-beta:
  set look and feel to windows, aqua or nimbus for swing in case
  fix import
  remove look and feel
  New translations JabRef_en.properties (French) (#4009)
  Fix Look and Feel related issues (#4002)
  Fix statement in changelog
  [WIP] Add Text File Export for "Find Unlinked Files" (#3979)
Siedlerchr added a commit that referenced this pull request May 5, 2018
…rsectionnew

* upstream/maintable-beta: (88 commits)
  set look and feel to windows, aqua or nimbus for swing in case
  fix import
  remove look and feel
  New translations JabRef_en.properties (French) (#4009)
  Fix Look and Feel related issues (#4002)
  Fix statement in changelog
  [WIP] Add Text File Export for "Find Unlinked Files" (#3979)
  Fix IEEE preview does not display month  (#3983)
  Activate context menu on key press (#4004)
  Improve code layout
  Fix concurrent modification exception when adding entries to groups
  Fix build
  Typo
  Add fix
  Rename test
  Fix #3994 Duplicate unmodifiable list for sorting (#3996)
  Remove deprecated and unused method (#3993)
  Improvements around external file types (#3887)
  Migrate to native gradle test task (#3987)
  Do not run checkstyle as part of the gradle check task (#3985)
  ...
Siedlerchr added a commit that referenced this pull request May 10, 2018
…drop

* upstream/maintable-beta: (174 commits)
  Fix ACM fetcher import of entries (#4019)
  Reimplement tooltips for file and identifier columns (#4011)
  Add button-icon for union/intersection in the groups side panel (#3954)
  Update Dependencies (#4012)
  set look and feel to windows, aqua or nimbus for swing in case
  fix import
  remove look and feel
  New translations JabRef_en.properties (French) (#4009)
  Fix Look and Feel related issues (#4002)
  Fix statement in changelog
  [WIP] Add Text File Export for "Find Unlinked Files" (#3979)
  Fix IEEE preview does not display month  (#3983)
  Activate context menu on key press (#4004)
  Improve code layout
  Fix concurrent modification exception when adding entries to groups
  Fix build
  Typo
  Add fix
  Rename test
  Fix #3994 Duplicate unmodifiable list for sorting (#3996)
  ...

# Conflicts:
#	src/main/java/org/jabref/gui/actions/CleanupAction.java
#	src/main/java/org/jabref/gui/maintable/MainTable.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os: linux os: macOS status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants