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 generic icon when using Wayland #1923

Merged
merged 2 commits into from
Dec 20, 2023
Merged

Fix generic icon when using Wayland #1923

merged 2 commits into from
Dec 20, 2023

Conversation

FireSpike0
Copy link

When using melonDS on Wayland (Plasma in my case) the program icon was not set correctly such that the generic Wayland icon was used instead of melonDS own icon. The reason for that is that the used method setWindowIcon does not work on Wayland anymore for setting app icons. Instead applications should make use of setDesktopFileName which provides the graphical shell the name of the associated desktop file from which the referenced icon is taken for the taskbar and the window border.

I've tested the commit on Linux using Plasma on Wayland and Plasma on X11; in either case both icons (window border / window decorations and taskbar) were set correctly.

Regarding the used flags I'm unsure if there exists a better fitting flag (like e.g. __UNIX__), but I think it should cover the relevant platforms.

What's worth noting is, that despite the icon shouldn't work at all without my commit on Wayland (with Plasma) in the taskbar and the window border, the icon was only broken in the window border and not in the taskbar. Other applications with the same issue (using only setWindowIcon) had broken icons in the taskbar and the window border.

The newly added method eventually raises the required version of Qt5 to version 5.7 in which the method setDesktopFileName was added.

@nadiaholmquist
Copy link
Collaborator

This seems good to me.

Is the ifdef actually necessary? The documentation would imply to me that the property exists on all platforms but only does something on freedesktop systems.

@FireSpike0
Copy link
Author

I'm not sure, whether it's necessary or not. I added the ifdef just for safety reasons to avoid breaking anything on platforms I couldn't test / I won't target with this fix.

Just a disclaimer for the following comment: I'm neither experienced in C++ nor in Qt, I just researched around the issue and found a solution.
A quick superficial look to the Qt docs also didn't mention, whether the ifdef around the preceeding setWindowIcon (which was my reference point for my addition) is necessary for Apple (-> if Apple must be excluded) / has an effect for Windows (-> if Windows must be included). Eventually these lines should be changed as well or maybe just my ifdef is not necessary at the moment.

@nadiaholmquist
Copy link
Collaborator

nadiaholmquist commented Dec 20, 2023

The ifdef for macOS is there because macOS determines the app icon based on the app bundle the program was launched from. macOS also allows setting custom app icons by the user, so we don't want to override the icon the system selected for the app if the user wants to use some different one.

I actually wonder if we even need it for Windows - or whether it will pick up the default icon from the .exe otherwise...

@FireSpike0
Copy link
Author

After thinking about the flags for a while I think we should at least leave them in or add a better fitting flag. The reason for that is, that at the moment the behavior of the method is unspecified for platforms not conforming to / not using the freedesktop desktop entry spec. If we enable that method call on all platforms, I think it could probably break in the future (or is broken at the moment; I cannot test it on macOS or Windows) if the methods non-freedesktop-platform behavior is changed. Additionally, the method isn't supposed to do / fix anything on other platforms (Windows an macOS). The unspecified behavior in the documentation holds for Qt 5 [1] as well as for Qt 6 [2].

I'm not sure which platforms are targetted by melonDS at the moment, but probably the flags should be changed so, that it's only included on platforms using the freedesktop desktop entry specification.

As I mentioned before I'm not an experienced C++ developer and as such I can't judge other criterias / aspects of using compilation flags in that case.

If the flag should be removed if possible for any reasons, I would appreciate some help on testing the code on Windows and especially on macOS.

[1] https://doc.qt.io/qt-5/qguiapplication.html#desktopFileName-prop
[2] https://doc.qt.io/qt-6/qguiapplication.html#desktopFileName-prop

@nadiaholmquist nadiaholmquist merged commit fd3c349 into melonDS-emu:master Dec 20, 2023
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants