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

Set WM_CLASS to org-jabref-JabRefMain #3273

Merged
merged 1 commit into from
Oct 9, 2017
Merged

Set WM_CLASS to org-jabref-JabRefMain #3273

merged 1 commit into from
Oct 9, 2017

Conversation

michaellass
Copy link
Contributor

WM_CLASS is used by certain Un*x window managers, such as GNOME, to map windows
to the corresponding application. Currently WM_CLASS is set to java-lang-Thread
which is very generic and not unique to JabRef. Instead, use reflection to set
it to org-jabref-JabRefMain which resembles the main class name.

The implementation in this PR is based on this blog post:
http://elliotth.blogspot.de/2007/02/fixing-wmclass-for-your-java.html
By now you find similar solutions in several Java applications. I'm not aware of a more convenient way to manually set WM_CLASS.

I tested the change on a Linux system running GNOME 3 and also on macOS to rule out any cross-platform issues. Also I chose the WM_CLASS name that matches the StartupWMClass in buildres/snapcraft/jabref.desktop, so no changes are necessary in there.

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

@Siedlerchr
Copy link
Member

Hm, I appreciate this 👍 , but I will think it will backfire to us with java 9 or later , because we are effectively accessing internal Apis (for sure jdeps will complain), but as we still have do to this for the accent problem on linux I think it's worth. And I see no alternative.

Another thing I found is the following: I do not fully understand the discussion and the resulting code change. But what I understand is that they added a property`/method to the c++ code which sets this gtk stuff in javafx.
Maybe someone with more insight can help to clarify this

https://bugs.openjdk.java.net/browse/JDK-8097949

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 6, 2017
lenhard
lenhard suggested changes Oct 7, 2017
View reviewed changes
src/main/java/org/jabref/gui/GUIGlobals.java Outdated
@@ -156,6 +157,16 @@ public static void init() {
GUIGlobals.currentFont = new Font(Globals.prefs.get(JabRefPreferences.FONT_FAMILY),
Globals.prefs.getInt(JabRefPreferences.FONT_STYLE), Globals.prefs.getInt(JabRefPreferences.FONT_SIZE));

// Set WM_CLASS using reflection for certain Un*x window managers
try {
Copy link
Member

Choose a reason for hiding this comment

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

I guess we should only execute this code, if we are actually running on a unix system, right? That means it should all be wrapped in an if (OS.LINUX)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would also exclude Unix systems like FreeBSD, wouldn't it?

Copy link
Member

Choose a reason for hiding this comment

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

In all my time in the JabRef team, I've never heard of someone running it on FreeBSD. Anyway, the alternative solution is !OS.WINDOWS && !OS.OSX :)

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 added that alternative variant now. Thanks for your review!

@tobiasdiez
Copy link
Member

tobiasdiez commented Oct 7, 2017 •
edited
Loading

According to the above link, for JavaFX it should suffice to call setApplicationName http://hg.openjdk.java.net/openjfx/8u60/rt/file/89345a49e71f/modules/graphics/src/main/java/com/sun/javafx/application/PlatformImpl.java#l109. But I have no idea how to do this...(and no IDE at hand to check further)

@michaellass
Copy link
Contributor Author

According to the above link, for JavaFX it should suffice to call setApplicationName

It seems to me like this should happen automatically when the application is launched. But for some reason this is either not happening or the name is set to java-lang-Thread instead. I will try to find out what is going on here...

@michaellass
Copy link
Contributor Author

Here are some new results:

  • LauncherImpl calls PlatformImpl.setApplicationName() during application startup
  • this in turn calls setName() on the Application object
  • both methods get the correct name ("org-jabref-JabRefMain") and this is successfully stored in the name attribute of the Application object
  • the value of the name attribute does not change afterwards
  • getName() of the Application object is never called

This leaves the big question: Why does my window manager still show "java-lang-Thread" as WM_CLASS? In case this is only an issue on specific Linux systems, here are information about mine:

  • Desktop / window manager: GNOME 3.26.1
  • GTK: 2.24.31
  • JRE: openjdk 8u144
  • JavaFX: openjfx 8u121
  • GNOME runs on wayland but jabref is started as an X11 application using xwayland

@Siedlerchr
Copy link
Member

That is indeed mysterious: I can confirm this with Xfce, however in the Task Manager under Xfce it's correctly displayed

libgtk-3-0:amd 3.18.9-1ubun amd64       
libgtk2.0-0:am 2.24.30-1ubu amd64 
xprop|grep WM_CLASS
WM_CLASS(STRING) = "sun-awt-X11-XFramePeer", "java-lang-Thread"

http://i.imgur.com/jfwCfuR.png

@tobiasdiez
Copy link
Member

Thanks for investigating this. I suppose the problem is again that we are mixing JavaFX and Swing. I'm fine with the awt-solution if it works as expected. Please add the if check @lenhard proposed and this is ready for merge.

@michaellass
Set WM_CLASS to org-jabref-JabRefMain
e89f949 
WM_CLASS is used by certain Un*x window managers, such as GNOME, to map windows
to the corresponding application. Currently WM_CLASS is set to java-lang-Thread
which is very generic and not unique to JabRef. Instead, use reflection to set
it to org-jabref-JabRefMain which resembles the main class name.
@tobiasdiez tobiasdiez merged commit 1b030b4 into JabRef:master Oct 9, 2017
Siedlerchr added a commit that referenced this pull request Oct 19, 2017
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into exportPdf
edec73d 
* upstream/master:
  Set WM_CLASS to org-jabref-JabRefMain (#3273)
  Terminate JavaFX thread when running with "nogui" option (#3274)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lenhard lenhard lenhard requested changes

@Siedlerchr Siedlerchr Siedlerchr approved these changes

Assignees
No one assigned
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
Development

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