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: set Wayland application ID #34855

Merged
merged 2 commits into from Jul 11, 2022

Conversation

vially
Copy link
Contributor

@vially vially commented Jul 8, 2022

Description of Change

Electron versions prior to 18.0.0 (<18.0.0) were setting the Wayland application ID to the same value as the X11 WM_CLASS property. But in recent versions of Electron the Wayland's application ID property is empty (#33578).

Most likely this was caused by this upstream Chromium change which introduced the wayland_app_id property.

This pull-request makes use of the wayland_app_id property by setting its value to the basename of the application's .desktop file.

Fixes #33578
Closes #34852

Related issues

Checklist

Release Notes

Notes: Fix empty app_id when running under wayland

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jul 8, 2022
Copy link
Member

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

Could you add Fixes: https://github.com/electron/electron/issues/33578 to the PR description, so that the issue gets closed automatically when your change gets merged? Might also want to add a release note because it fixes an issue that's relevant to app developers.

Could you also add a test? Maybe something like parsing the output of the logs mentioned in #33578 (comment) if there's no neater way?

Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing this issue! Also reminded me that wm_class_class should also be fixed for wayland microsoft/vscode#129953 (comment)

@deepak1556 deepak1556 added semver/patch backwards-compatible bug fixes target/18-x-y labels Jul 11, 2022
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jul 11, 2022
@MarshallOfSound MarshallOfSound merged commit f63bba8 into electron:main Jul 11, 2022
@release-clerk
Copy link

release-clerk bot commented Jul 11, 2022

Release Notes Persisted

Fix empty app_id when running under wayland

@trop
Copy link
Contributor

trop bot commented Jul 11, 2022

I have automatically backported this PR to "18-x-y", please check out #34877

@trop
Copy link
Contributor

trop bot commented Jul 11, 2022

I have automatically backported this PR to "19-x-y", please check out #34878

@trop
Copy link
Contributor

trop bot commented Jul 11, 2022

I have automatically backported this PR to "20-x-y", please check out #34879

@vially vially deleted the fix-wayland-app-id branch July 12, 2022 08:19
@vially
Copy link
Contributor Author

vially commented Jul 12, 2022

Could you also add a test? Maybe something like parsing the output of the logs mentioned in #33578 (comment) if there's no neater way?

@RaisinTen I was considering adding some basic Wayland tests but I encountered some issues and I have the feeling there are some things that would need to be taken care of before being able to do that.

Part of the challenge here is that, as far as I'm aware, the CI is currently configured to run all Linux tests under X11 only. On top of that, some of the existing tests are currently failing when run under Wayland.

So, before getting started with writing some Wayland specific tests, these are a couple of things that would need to be fixed first:

  • make a Wayland environment available for running the tests in CI
  • identify and either fix or disable the tests that are currently failing under Wayland
  • decide on a strategy for running the Linux tests (e.g.: should all Linux tests be run twice: once under X11 and once under Wayland?)

I could try to help with some of these issues, but I would need some pointers from someone who is 1) familiar with these systems and 2) is able to provide guidance and feedback along the way.

Also reminded me that wm_class_class should also be fixed for wayland

@deepak1556 My impression was that wm_class_class should not be used at all when Electron is running as a native Wayland client (e.g.: --ozone-platform=wayland). Or do you mean that it should be fixed when running Electron under XWayland? If that's what you mean, what should it be changed into?

X11 WM_CLASS vs Wayland app_id

I've only looked briefly into it, but according to GNOME's recommendation, the app should "[...] set the WM_CLASS X window property to be the same as your application's .desktop file name, without the .desktop extension". In other words, it should be the same as Wayland's app_id.

But, as you've mentioned (microsoft/vscode#129953 (comment)), that's not currently the case.

So, if backwards compatibility was not a concern, my suggestion would be to make both X11's WM_CLASS and Wayland's app_id to be the same (e.g.: the app's .desktop file name without the extension). It might also be worth making the setDesktopName() API public (Linux only), or improve the documentation to make it slightly easier to find a way to set it:

// Set application's desktop name.
if (packageJson.desktopName != null) {
app.setDesktopName(packageJson.desktopName);
} else {
app.setDesktopName(`${app.name}.desktop`);
}

@RaisinTen
Copy link
Member

So, before getting started with writing some Wayland specific tests, these are a couple of things that would need to be fixed first:

  • make a Wayland environment available for running the tests in CI
  • identify and either fix or disable the tests that are currently failing under Wayland
  • decide on a strategy for running the Linux tests (e.g.: should all Linux tests be run twice: once under X11 and once under Wayland?)

I could try to help with some of these issues, but I would need some pointers from someone who is 1) familiar with these systems and 2) is able to provide guidance and feedback along the way.

To my understanding, making the Wayland environment should be done in https://github.com/electron/build-images. I'm not sure who the right person is for answering your questions, maybe @MarshallOfSound and @jkleinsc because y'all have the highest number of commits on that repo?

@ReillyBrogan
Copy link

I think there's an issue in this in the case where an application has multiple .desktop files. For instance consider the following two desktop files:

code-oss.desktop

[Desktop Entry]
Name=Visual Studio Code
Comment=Visual Studio Code
GenericName=Text Editor
Exec=/usr/bin/code-oss --ozone-platform-hint=auto --disable-crash-reporter %U
Icon=com.visualstudio.code.oss
Type=Application
StartupNotify=false
StartupWMClass=code-oss
Categories=TextEditor;Development;IDE;
MimeType=text/plain;application/x-code-oss-workspace;
Terminal=false
Actions=new-empty-window;
Keywords=vscode;

[Desktop Action new-empty-window]
Name=New Empty Window
Exec=/usr/bin/code-oss --ozone-platform-hint=auto --disable-crash-reporter --new-window %F
Icon=com.visualstudio.code.oss

code-oss-url-handler.desktop

[Desktop Entry]
Name=Visual Studio Code URL Handler
Comment=Code Editing. Redefined.
GenericName=Text Editor
Exec=/usr/bin/code-oss --ozone-platform-hint=auto --disable-crash-reporter --open-url %U
Icon=com.visualstudio.code.oss
Type=Application
NoDisplay=true
StartupNotify=true
Categories=Utility;TextEditor;Development;IDE;
MimeType=x-scheme-handler/code-oss;
Keywords=vscode;

(this is a real example where a separate desktop file exists in order to register a URL handler, tested by switching vscode 1.69.2 to Electron 19.0.10)

In this case Electron when launched via the code-oss.desktop desktop fill will apparently set the app_id to code-oss-url-handler which obviously does not match what it should be in the code-oss.desktop file, thus triggering the WM to believe that it's a separate application and show the generic icon instead.

I'm not sure what the correct logic should be, but perhaps desktop files with NoDisplay=true should be ignored during the check?

@zcbenz
Copy link
Member

zcbenz commented Aug 2, 2022

@ReillyBrogan Apps should have specified its desktop filename with app.setDesktopName, so I think multiple desktop files should be fine?

@ReillyBrogan
Copy link

ReillyBrogan commented Aug 2, 2022

@ReillyBrogan Apps should have specified its desktop filename with app.setDesktopName, so I think multiple desktop files should be fine?

I'm unsure of what you mean (I'm not generally familiar with the Electron codebase, just trying to report this issue after testing the new version of Electron with a few apps) . Are you saying that that's how they should do it when the electron app is installed at the user-level? What about the case where an Electron app is packaged as a system package? The user running the app would typically not run it as a root-level process and it would not be able to modify its .desktop file in that case.

For instance take a look at the Arch Linux package for code-oss, it ships two desktop files in /usr/share/applications and would almost certainly run into the same issue I described after updating to Electron 19.0.10, or any other version with the fix in this PR.

@ReillyBrogan
Copy link

In the system package case the packager usually decides the name of the desktop file, though they may decide to use the default names provided by the application.

@zcbenz
Copy link
Member

zcbenz commented Aug 2, 2022

Electron has an API (app.setDesktopName) to set which desktop file to use, and the wayland_app_id set in this PR uses the result of the API. So if apps have multiple desktop files, they can use the API to specify the name of desktop instead of letting the desktop manager to guess one for them.

@vially
Copy link
Contributor Author

vially commented Aug 2, 2022

[...] when launched via the code-oss.desktop desktop fill will apparently set the app_id to code-oss-url-handler

For what it's worth, VSCode explicitly sets the desktopName to the URL handler desktop file. Without looking into it, this does seem a bit strange, but I suppose there must be a good reason for it.

But even so, my interpretation of the specs1 is that as long as the deskop file (code-oss-url-handler.desktop in this case) contains an Icon field, I think the window manager/compositor should be able to pick it up.

Footnotes

    1. Applications in GNOME 3
    2. Desktop Entry Specification

@ReillyBrogan
Copy link

But even so, my interpretation of the specs1 is that as long as the deskop file (code-oss-url-handler.desktop in this case) contains an Icon field, I think the window manager/compositor should be able to pick it up.

Yes, the icon will be picked up correctly but if the user has some kind of taskbar that they've pinned the "main" desktop file to the end result will be that windows created by clicking that desktop file will not be correctly associated with the main launcher and would appear as a new entry. For instance, I'm using Plasma right now where the taskbar by default has similar behavior to the Windows 7/10/11 taskbar in that users are able to pin applications to it and running instances of those applications are automatically grouped with the pinned icon.

It does seem though like this is definitely an issue on the VS Code side, so I'll bring up the issue on their tracker so they can correct the incorrect desktopName behavior. Thank you so much for your assistance!

schetle pushed a commit to schetle/electron that referenced this pull request Nov 3, 2022
* refactor: extract XDG app ID logic into a method

* fix: set application ID on Wayland
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
* refactor: extract XDG app ID logic into a method

* fix: set application ID on Wayland
@pdf pdf mentioned this pull request Jan 22, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
7 participants