jellyfin-media-player: fix StartupWMClass/icon

[bjornfor]

Things done

Fix the icon in the dash/dock.

Upstream PR: jellyfin/jellyfin-media-player#771

Tested in NixSO 24.11.

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[paumr]

Hi, thanks for your PR!

I had a quick look into this issue, I'll outline my findings here. Please let me know if I misunderstood/misinterpreted something.

Findings:

• lots of tools / ui-frameworks seem to use StartupWMClass differently on wayland than on X11, however they seem to adapt "wayland's style"
• it does not seem like the StartupWMClass specification changed:
  https://specifications.freedesktop.org/desktop-entry-spec/latest/recognized-keys.html
• this PR caused your issue: Fix icon for Wayland (#440 #462) jellyfin/jellyfin-media-player#515
  • does this set the wmClass appropriately? If not it will get fixed in an upcoming release:
    • https://bugreports.qt.io/browse/QTBUG-117772
    • change in question: https://codereview.qt-project.org/c/qt/qtbase/+/509724/3/src/plugins/platforms/xcb/qxcbwindow.cpp
• your mentioned upstream PR was created in October 2024, it was not merged: Fix desktop icon jellyfin/jellyfin-media-player#771
• this worked before on X11 since it relied on the (unspecified) default behavior

Further notes:

• The set wmClass / app_id can be debugged via:
  • wayland: https://bugs.kde.org/show_bug.cgi?id=409967#c1
  • X11: xprop | grep -i WM_CLASS

First of all I have to say was surprised to see that the projects I looked into QT/GTK adapted this change without any change in the specification.
But since they did, and since I assume this PR breaks the icons for wayland, I don't see a point in trying to restore the prior behavior.
Even more so since it would distance us from the upstream changes.

What would make sense however would be to look into why setDesktopFileName does not have the desired effect on X11 systems.
https://doc.qt.io/qt-6/qguiapplication.html#desktopFileName-prop
Maybe some other attribute has to be set? If this can be figured out/fixed I'll happily approve this PR.

@bjornfor can you confirm that my findings are correct? Did you test this change on a wayland system?

[bjornfor]

@bjornfor can you confirm that my findings are correct?

Not enough time/skill on my part.

Did you test this change on a wayland system?

Yes. I run NixOS with GNOME (navigating to "Settings -> About -> System Details" shows "Wayland") and verified this PR there. I'm pretty sure the Jellyfin icon worked fine before, and it wasn't until the upstream change that it broke for me (and a few others).

[bjornfor]

Thanks for digging into the issue!

[paumr]

You are welcome!

To Document this issue:
Can you execute WAYLAND_DEBUG=1 jellyfinmediaplayer |& grep set_app_id and paste its output here?
(using the upstream version, not the PR one)

Question regarding this PR:
You substitute the entry of the .desktop file on linux and darwin. Does the file exist in darwin's $out directory?
Does MacOS use .desktop files?

General Note:
I'm not sure if using substituteInPlace here is the best approach.
Personally I'd go for reverting the (presumably breaking) PR and adding this as a patch.
However, we would have to check first if this works as expected on newer qt/wayland/x11 versions.

[bjornfor]

You are welcome!

To Document this issue: Can you execute WAYLAND_DEBUG=1 jellyfinmediaplayer |& grep set_app_id and paste its output here? (using the upstream version, not the PR one)

On NixOS 24.11 at commit e24b4c0, with GNOME (Wayland) desktop:

$ WAYLAND_DEBUG=1 jellyfinmediaplayer |& grep set_app_id
(no output)

Question regarding this PR: You substitute the entry of the .desktop file on linux and darwin. Does the file exist in darwin's $out directory? Does MacOS use .desktop files?

Oops! I think not. Will fix.

General Note: I'm not sure if using substituteInPlace here is the best approach. Personally I'd go for reverting the (presumably breaking) PR and adding this as a patch.

• Do you mean to create the revert patch locally and copy it into nixpkgs? That I find more expensive for little added value for this one-line change. substituteInPlace --replace-fail fails loudly when it doesn't match, so I don't really see any benefit in using real patches.
• Or do you mean to fetchpatch the upstream PR? I guess I can, but I'm worried about a commit reference in an unmerged PR going stale.
• Maybe there's a way to reverse-apply a patch directly? But I don't know how. (patch --reverse?)

[bjornfor]

Maybe you're right about substituteInPlace vs patch, because now that I think of it, I don't know what other, if any, system uses the desktop files. So would the substituteInPlace condition be !isDarwin or something else? Or check if the file exists at build time? But that leads to silent failures if the file is renamed...

But, let's find out what the StartupWMClass= value should be first, then I can update the implementation.

[bjornfor]

Oh, and a side note about the isDarwin condition; I think the correct check is hostPlatform.isDarwin, not stdenv.isDarwin.

[bjornfor]

Made a discovery; jellyfin-media-player runs under xwayland on my NixOS GNOME system!

$ xlsclients
bf-laptop  keepassxc
bf-laptop  gsd-xsettings
bf-laptop  jellyfinmediaplayer         <----- It's a xwayland client, not native wayland
bf-laptop  mutter-x11-frames

So that should explain some things... but why isn't it using native wayland?

[bjornfor]

So that should explain some things... but why isn't it using native wayland?

Now I noticed this startup message:

$ ./result/bin/jellyfinmediaplayer 
Warning: Ignoring XDG_SESSION_TYPE=wayland on Gnome. Use QT_QPA_PLATFORM=wayland to run on Wayland anyway.
libpng warning: iCCP: known incorrect sRGB profile

And surely, running like this fixes the icon: QT_QPA_PLATFORM=wayland ./result/bin/jellyfinmediaplayer.

But why is it ignoring wayland to begin with?

[bjornfor]

$ WAYLAND_DEBUG=1 jellyfinmediaplayer |& grep set_app_id
(no output)

But forcing wayland, it the app id gets set:

$ WAYLAND_DEBUG=1 QT_QPA_PLATFORM=wayland ./result/bin/jellyfinmediaplayer |& grep app_id
[1569261.892] {Default Queue}  -> xdg_toplevel#25.set_app_id("com.github.iwalton3.jellyfin-media-player")

(When run like this the icon is correct in the dash/dock.)