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

improve wayland compatibility #1548

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

softmix
Copy link

@softmix softmix commented Sep 9, 2022

to avoid outright crashes on native wayland, this removes the x11 entanglements of goldedict at the expense of losing hotkey and scan popup functionality

this branch can help users who wish to run goldendict on native wayland (rather than xwayland) do their dictionary lookups as before, although the x11 desktop integration is removed from it. it is a stopgap measure before wayland protocols are stabilized and can be relied on for providing desktop integration again. it is possible that these functionalities will not be implemented in any protocols, however

this removes the x11 entanglements of goldedict at the expense of losing hotkey and scan popup functionality
Copy link
Member

@vedgy vedgy left a comment

Choose a reason for hiding this comment

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

  1. What are the downsides of using GoldenDict with XWayland?
  2. Can the X11-specific code be disabled conditionally on native Wayland? Some of the functionality removed in this commit is nonessential. For instance, the users should be able to emulate the hotkeys by adding system keyboard (application) shortcuts. This emulation might actually turn out better, because currently not all key combinations work as hotkeys in practice. It may be possible to make the native-Wayland GD version work pretty well, in which case the changes could be merged into master.
  3. Does selection buffer work on Wayland? If not, this could be the most significant and difficult to overcome issue. What else doesn't work?

p.scanPopupModifiers += ui.leftCtrl->isChecked() ? KeyboardState::LeftCtrl: 0;
p.scanPopupModifiers += ui.rightCtrl->isChecked() ? KeyboardState::RightCtrl: 0;
p.scanPopupModifiers += ui.leftShift->isChecked() ? KeyboardState::LeftShift: 0;
p.scanPopupModifiers += ui.rightShift->isChecked() ? KeyboardState::RightShift: 0;
Copy link
Member

Choose a reason for hiding this comment

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

This should be implementable on Wayland.

@softmix
Copy link
Author

softmix commented Sep 11, 2022

What are the downsides of using GoldenDict with XWayland?

having to enable and allow xwayland which is an entire x11 server. the goal of wayland is to finally replace that and the existence of xwayland is to support transition for legacy applications until they are replaced or adapted. qt applications that have kept up with the development of the toolkit itself are already over that hill since the toolkit has first-class support for wayland. in case of goldendict, the x11 entanglements really are hacks that go around the toolkit and that's why they clashed with native wayland execution in the first place. from a design standpoint it is less-than-good practice, even if functionally necessary, to take these alternative routes

Can the X11-specific code be disabled conditionally on native Wayland?

yes, it could be placed in #ifdef HAVE_X11 blocks as some non-x11 "unix" (macos) support is already there and uses such blocks and their counterparts to exclude other x11 code. it should have been unnecessary since qt is already (intended to be) cross-platform. the hacks were required specifically for defeating qt's model of doing things and as such had to be implemented three times in different ways for the desktop platforms goldendict runs on (x11, win32, macos). touching the code further would require closer familiarity with the code base than i have. what's more, wayland protocols are not stable yet. there are three iterations of input method protocol which are implemented by different compositors and at least two iterations of text input protocol. it will still take some time before the canonical way of doing these things, or in some cases the logistical possibility and desirability of them, becomes clear. some functionalities of x11 depend on the (lack of) security model which allowed every x11 application visibility into contents and received events of any other x11 application. there were prospective security extensions to x window system regulating that issue but they stayed nascent in the most common x window system implementation and didn't take off. wayland specifically avoids that problem by completely isolating its clients from each other but since the visibility in question is integral to some applications, such as screen readers or dictionary software that can read text from other applications or screenshot applications that can get images from other applications' windows, it is possible that a compositor-gated protocol and implemented mechanism for going around the gui isolation principles conditional on user consent is developed later. it is possible that gnome already has such functionality but i'm unaware of its specifics or how broadly it is implemented in other comopsitors, such as wlroots-based ones

Does selection buffer work on Wayland? If not, this could be the most significant and difficult to overcome issue. What else doesn't work?

there is a clipboard mechanism in wayland implemented in the more popular compositors. there is no equivalent to (primary and secondary) selection buffers and it is specifically undesired as it is considered a grave security risk. central to wayland's design is that any inter-process communication (side-)channels opened by the gui should be regulated by the compositor so that gui isolation can be achieved if desired by the user (or broader system designers)

@vedgy
Copy link
Member

vedgy commented Sep 11, 2022

Does the X11-specific workaround in MainWindow::toggleMainWindow() work on native Wayland? I would be surprised if it did, but this PR doesn't disable it...

This workaround in MainWindow::toggleMainWindow() is the root cause of #781. I have tried to #ifdef the workaround away as detailed in my comments under that issue. But the workaround turned out to be essential on Plasma. Replacing it with a non-workaround implementation proved to be more difficult and time-consuming than expected, which lead me to give up. Still, this should be a relatively easy task for a developer with startup notifications experience. Pull requests are welcome.

@softmix
Copy link
Author

softmix commented Sep 12, 2022

Does the X11-specific workaround in MainWindow::toggleMainWindow() work on native Wayland? I would be surprised if it did, but this PR doesn't disable it...

in goldendict.pro line 152 HAVE_X11 is removed from definitions so any sections enclosed in that are automatically removed by the preprocessor. there is no need to directly modify those sections of mainwindow.cc. what the other changes on this branch do is removing the parts that are x11 entanglements not marked as they should have been

achieving a "toggle window by global hotkey" function on native wayland is possible since swhkd exists already (https://github.com/waycrate/swhkd) and the toggle window action via tray icon works for both goldendict and keepassxc (both qt applications) so chaining these together one could get a hotkey translated to a regular window show/hide

@vedgy
Copy link
Member

vedgy commented Sep 12, 2022

in goldendict.pro line 152 HAVE_X11 is removed from definitions so any sections enclosed in that are automatically removed by the preprocessor. there is no need to directly modify those sections of mainwindow.cc.

Haven't noticed this. I don't think removing this definition from goldendict.pro is a good idea. There is lots of code under that preprocessor variable that should work on native Wayland. I think HAVE_X11 has been used instead of Q_OS_LINUX in order to enable the code sections on other freedesktop systems, such as BSD.

Haven't found any mention of startup notifications in the swhkd repository. Either activating a window doesn't work on Plasma with default focus stealing prevention level or swhkd defeats them in a different way.

@dontdieych
Copy link

to avoid outright crashes on native wayland, this removes the x11 entanglements of goldendict at the expense of losing hotkey and scan popup functionality

this branch can help users who wish to run goldendict on native wayland (rather than xwayland) do their dictionary lookups as before, although the x11 desktop integration is removed from it. it is a stopgap measure before wayland protocols are stabilized and can be relied on for providing desktop integration again. it is possible that these functionalities will not be implemented in any protocols, however

@softmix , I wanna try this build for not stretched window (blurry text) in hidpi setup.

One question, do you have any workaround for scan popup and activate goldendict main window hotkeys? Don't need to be full explanation. Any clue appreciated.

@softmix
Copy link
Author

softmix commented Oct 14, 2022

One question, do you have any workaround for scan popup and activate goldendict main window hotkeys? Don't need to be full explanation. Any clue appreciated.

@dontdieych, no. unfortunately not. i didn't use those features on x11 either

@dontdieych
Copy link

dontdieych commented Oct 14, 2022 via email

@ceed0
Copy link

ceed0 commented Jan 7, 2023

activate goldendict main window hotkeys

With wayland this should be handled on compositor side, because goldendict wouldn't have access to global input. Maybe scan popup could be implemented too with a few cli options.

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.

None yet

4 participants