Skip to content

Set WM_CLASS to org-jabref-JabRefMain#3273

Merged
tobiasdiez merged 1 commit into
JabRef:masterfrom
michaellass:set_wm_class
Oct 9, 2017
Merged

Set WM_CLASS to org-jabref-JabRefMain#3273
tobiasdiez merged 1 commit into
JabRef:masterfrom
michaellass:set_wm_class

Conversation

@michaellass

@michaellass michaellass commented Oct 6, 2017

Copy link
Copy Markdown
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

Siedlerchr commented Oct 6, 2017

Copy link
Copy Markdown
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
Comment thread src/main/java/org/jabref/gui/GUIGlobals.java Outdated
Globals.prefs.getInt(JabRefPreferences.FONT_STYLE), Globals.prefs.getInt(JabRefPreferences.FONT_SIZE));

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

@lenhard lenhard Oct 7, 2017

Copy link
Copy Markdown
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)

@michaellass michaellass Oct 7, 2017

Copy link
Copy Markdown
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?

@lenhard lenhard Oct 7, 2017

Copy link
Copy Markdown
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 :)

@michaellass michaellass Oct 8, 2017

Copy link
Copy Markdown
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

tobiasdiez commented Oct 7, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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

michaellass commented Oct 7, 2017

Copy link
Copy Markdown
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

michaellass commented Oct 7, 2017

Copy link
Copy Markdown
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

Siedlerchr commented Oct 7, 2017

Copy link
Copy Markdown
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

tobiasdiez commented Oct 8, 2017

Copy link
Copy Markdown
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

@Siedlerchr Siedlerchr Siedlerchr approved these changes

+1 more reviewer

@lenhard lenhard lenhard requested changes

Reviewers whose approvals may not affect merge requirements

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