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

wip: add/use wlr_xwayland_surface_offer_focus() #1142

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jlindgren90
Copy link
Contributor

@jlindgren90 jlindgren90 commented Oct 6, 2023

For testing/discussion. Depends on a wlroots change that I haven't submitted yet submitted here:
https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4384

Offer focus by sending WM_TAKE_FOCUS to a client window supporting it. The client may accept or ignore the offer. If it accepts, the surface will emit a focus_in signal notifying the compositor that it has received focus. The compositor should then call wlr_xwayland_surface_activate(surface, true).

This is a more compatible method of giving focus to windows using the Globally Active input model (see wlr_xwayland_icccm_input_model()) than calling wlr_xwayland_surface_activate() unconditionally, since there is no reliable way to know in advance whether these windows want to be focused.

Previous experimental wlroots branch, with lots of extra debugging output for testing purposes:
jlindgren90/wlroots@offer-focus

@jlindgren90
Copy link
Contributor Author

This fixes an issue with qmpanel (probably lxqt-panel as well) running in XWayland mode, extremely similar to the recent waybar issue (#1131). The taskbar wants to raise/focus a window if it's not already active, otherwise minimize it. The logic breaks down if the panel itself gets focus.

I'd be interested in how this affects other XWayland applications: good, bad, indifferent? Particularly if @kyak would be gracious enough to test it with MATLAB and log the debug output, that would be helpful.

@Consolatis
Copy link
Member

Consolatis commented Oct 6, 2023

This will likely fail to build on systems with a system wide installation of wlroots 0.16.
Suggest changing the wlroots dep in meson.build to version: ['>=0.17.0', '<0.18.0'].

Edit: Woops, looked at the wrong branch, sorry for the noise.

@jlindgren90
Copy link
Contributor Author

The wlroots branch is 0.16.2 + one commit, not 0.17 though.
Using meson setup --force-fallback-for=wlroots build should work if there is already a system-wide wlroots 0.16.2.

@Consolatis
Copy link
Member

Yep, just recognized. Disregard my comment :)

@stefonarch
Copy link
Contributor

stefonarch commented Oct 6, 2023

This fixes an issue with qmpanel (probably lxqt-panel as well) running in XWayland mode,

To be honest i never even tried to use those panels in XWayland mode. I see

  • popups, calendar and menus not closing if focus changes
  • taskbars only show xwayland windows so basically still useless if based on kwindowsystem
  • window rules not working (toggleAlwaysOnBottom) → not sticky

@kyak
Copy link
Contributor

kyak commented Oct 6, 2023

@jlindgren90 I get this error during build:

Looking for a fallback subproject for the dependency wlroots because:
Use of fallback dependencies is forced.

labwc/meson.build:51:10: ERROR: Automatic wrap-based subproject downloading is disabled

@jlindgren90
Copy link
Contributor Author

@kyak That's odd, I haven't seen that error before. You could also install that branch of wlroots separately.

@jlindgren90
Copy link
Contributor Author

Menus/popups not closing automatically is an issue with XWayland views in general. They expect to be able to observe clicks elsewhere, which only works if you click on another XWayland view -- not a native Wayland view or the desktop. I have some ideas how to solve that, nothing definite yet though.

@stefonarch
Copy link
Contributor

Menus/popups not closing automatically is an issue with XWayland views in general.

The only xwayland app I have to use is element-desktop and menus from menu bars are closing fine here. Also falkon in xwayland mode closes fine its settings I see, so there is some other factor involved probably.

@jlindgren90
Copy link
Contributor Author

I have seen both GTK and Qt apps affected. Haven't tested either of the ones you mentioned.

@jlindgren90
Copy link
Contributor Author

At the moment, I run just about everything in XWayland due to basic features like window positioning not working under native Wayland (support is missing from xdg-shell protocol entirely and you have to use layer-shell, but many apps don't support layer-shell well or at all).

@Consolatis
Copy link
Member

Consolatis commented Oct 6, 2023

labwc/meson.build:51:10: ERROR: Automatic wrap-based subproject downloading is disabled

That sounds like a setting / meson command arg from the PKGBUILD (or your distro's variant of a build script).
Compiling it manually should not show that.

@kyak
Copy link
Contributor

kyak commented Oct 6, 2023

labwc/meson.build:51:10: ERROR: Automatic wrap-based subproject downloading is disabled

That sounds like a setting / meson command arg from the PKGBUILD (or your distro's variant of a build script). Compiling it manually should not show that.

Yep, that's from arch-meson, thanks!

@Consolatis
Copy link
Member

Regarding the PR relying on a change to wlroots 0.16: I don't think a API change like this (even if its just an addition) will be allowed into the 0.16 branch of wlroots.

I think we could theoretically copy over xwayland/xwm.c from wlroots and configure our own X11 window manager. That would allow a bit more flexibility here. But that also means that we stop benefiting from upstream changes there and might need to adjust based on further xwayland changes. So maybe this is something that should target wlroots 0.17 instead.

@jlindgren90
Copy link
Contributor Author

I agree, the wlroots change would go into 0.17, not 0.16. The experimental wlroots branch was based on 0.16 just for the sake of testing (to avoid dealing with other 0.17 changes/issues). The actual change cherry-picks fine to 0.17 without conflicts.

@kyak
Copy link
Contributor

kyak commented Oct 7, 2023

@jlindgren90 here is the log: labwc.log

I'd say, it works somewhat better. For example, the popup menu doesn't disappear now. Although I can't click anything in the menu - it's like this popup never gets focus.

@jlindgren90
Copy link
Contributor Author

Thanks for testing! An unfocusable popup might be an issue with how we handle "unmanaged" (override-redirect) XWayland windows. I'm not actually sure why this change would have helped at all with popups, but it's good to hear that it at least didn't make things worse.

There are a couple of interesting things in the log. Not necessarily anything indicating an issue on the labwc/wlroots side, but merely things that caught my attention:

00:01:31.528 [ERROR] [../labwc/src/desktop.c:91] not focusing view [DefaultOverlayManager.JWindow]
00:01:39.412 [ERROR] [../labwc/src/desktop.c:91] not focusing view [DefaultOverlayManager.JWindow]
00:01:40.412 [ERROR] [../labwc/src/desktop.c:91] not focusing view [DefaultOverlayManager.JWindow]
00:01:43.441 [ERROR] [../labwc/src/desktop.c:91] not focusing view [DefaultOverlayManager.JWindow]

I'm not sure what this "DefaultOverlayManager.JWindow" is. It appears to have neither WM_HINTS.focus nor WM_TAKE_FOCUS set, making it truly a "No Input" window, thus the technically correct behavior is not to focus it. Hopefully this isn't the popup that never received focus.

00:01:01.858 [ERROR] [../labwc/src/desktop.c:78] setting focus to view [kyak@laptop:~]
00:01:01.858 [ERROR] [xwayland/xwm.c:314] setting focus to none
00:01:04.193 [ERROR] [../labwc/src/desktop.c:86] offering focus to view [MATLAB R2023b - trial use]
00:01:04.193 [ERROR] [xwayland/xwm.c:360] offering focus to surface [MATLAB R2023b - trial use]
00:01:04.194 [ERROR] [xwayland/xwm.c:1538] rejected focus_in for surface [Simulink Start Page]
00:01:04.194 [ERROR] [xwayland/xwm.c:314] setting focus to none
00:01:04.195 [ERROR] [xwayland/xwm.c:1538] rejected focus_in for surface [Simulink Start Page]
00:01:04.195 [ERROR] [xwayland/xwm.c:314] setting focus to none

This looks like we tried to focus MATLAB and it rejected the focus, passing it to Simulink instead. wlroots prevented this (as it was unexpected) and it looks like the terminal window (Wayland-native?) kept the focus.

00:01:12.455 [ERROR] [../labwc/src/desktop.c:78] setting focus to view [Simulink Start Page]
00:01:12.455 [ERROR] [xwayland/xwm.c:332] setting focus to surface [Simulink Start Page]
00:01:12.456 [ERROR] [xwayland/xwm.c:1533] accepted focus_in for surface [Simulink Start Page]
00:01:12.456 [ERROR] [xwayland/xwm.c:305] surface is already focused
00:01:12.456 [ERROR] [../labwc/src/xwayland.c:407] received focus_in for view [Simulink Start Page]
00:01:17.802 [ERROR] [xwayland/xwm.c:1538] rejected focus_in for surface [MATLAB R2023b - trial use]

This looks like we focused Simulink and then MATLAB tried to "steal" the focus back. wlroots prevented this, and Simulink kept the focus. I guess MATLAB/Simulink are making some attempts to automatically pass focus back and forth, but this isn't allowed in labwc (by wlroots policy).

@kyak
Copy link
Contributor

kyak commented Oct 18, 2023

@jlindgren90 i just tried this branch in its current state.

Popups do show up (as opposed to current master), but they are not clickable. When i try to click an item in the popup menu, it behaves as if I've "clicked through" the popup and clicked the window behind the popup menu.

But this is already a great progress!

@jlindgren90
Copy link
Contributor Author

Popups do show up (as opposed to current master), but they are not clickable. When i try to click an item in the popup menu, it behaves as if I've "clicked through" the popup and clicked the window behind the popup menu.

That's very interesting, and unexpected to me. Not sure what would be causing that. Thanks for testing, regardless!

@jlindgren90
Copy link
Contributor Author

Now that I think about it, I have noticed some other popups (xfce4-notifyd notifications for example) being unclickable. Maybe related, maybe not.

Clicks passing through sounds like maybe XWayland's idea of the window stacking order is out-of-sync with labwc's.

@kyak
Copy link
Contributor

kyak commented Nov 24, 2023

@jlindgren90 seems that wlroots 0.17 has been released, but the MR (https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4384) wasn't merged.

I've branched off your branch (https://github.com/kyak/labwc/tree/offer-focus) and been rebasing this on top of master for the last month (so that not to bother you with rebases), and it seems to work for me.

However, I'm not very fond of doing this for indefinite time :) Do you have an idea what can we do?

@jlindgren90
Copy link
Contributor Author

I guess we'll just have to wait for the next wlroots release.

@Consolatis
Copy link
Member

There were talks by the wlroots devs doing more releases, once a year at the current stage of development indeed seems a bit long. I also didn't manage to get the expose-window-manager-xcb-connection feature into the release (although that is completely on me because I didn't actually send a MR for that and then was surprised by the release).

@jlindgren90
Copy link
Contributor Author

Rebased onto master and switched over to a 0.17-backports branch of wlroots.

@kyak
Copy link
Contributor

kyak commented Feb 4, 2024

@jlindgren90 can you please rebase your branch on top of the latest labwc/master? There are rebase conflicts since maybe 1 week ago and it would be better if you looked into it.

@jlindgren90
Copy link
Contributor Author

Yes, I'll try to get to it soon. I've been a bit busy lately, sorry this has gotten out of date.

@jlindgren90
Copy link
Contributor Author

Updated to include all the fixes from #1533, plus one more fix (support for keyboard grabs). The wlroots branch also has some new changes.

@kyak
Copy link
Contributor

kyak commented Feb 21, 2024

Updated to include all the fixes from #1533, plus one more fix (support for keyboard grabs). The wlroots branch also has some new changes.

I've been running this branch for a couple of days now, all looks good!

jlindgren90 added a commit to jlindgren90/labwc that referenced this pull request Mar 2, 2024
Offer focus by sending WM_TAKE_FOCUS to a client window supporting it.
The client may accept or ignore the offer. If it accepts, the surface will
emit a focus_in signal notifying the compositor that it has received focus.
The compositor should then call wlr_xwayland_surface_activate(surface, true).

This is a more compatible method of giving focus to windows using the
Globally Active input model (see wlr_xwayland_icccm_input_model()) than
calling wlr_xwayland_surface_activate() unconditionally, since there is no
reliable way to know in advance whether these windows want to be focused.

labwc#1142
jlindgren90 added a commit to jlindgren90/labwc that referenced this pull request Mar 2, 2024
Use the new grab_focus signal as a more reliable way to tell when an
unmanaged (override-redirect) surface wants focus.

labwc#1142
jlindgren90 added a commit to jlindgren90/labwc that referenced this pull request Mar 4, 2024
Offer focus by sending WM_TAKE_FOCUS to a client window supporting it.
The client may accept or ignore the offer. If it accepts, the surface will
emit a focus_in signal notifying the compositor that it has received focus.
The compositor should then call wlr_xwayland_surface_activate(surface, true).

This is a more compatible method of giving focus to windows using the
Globally Active input model (see wlr_xwayland_icccm_input_model()) than
calling wlr_xwayland_surface_activate() unconditionally, since there is no
reliable way to know in advance whether these windows want to be focused.

labwc#1142
jlindgren90 added a commit to jlindgren90/labwc that referenced this pull request Mar 4, 2024
Use the new grab_focus signal as a more reliable way to tell when an
unmanaged (override-redirect) surface wants focus.

labwc#1142
Offer focus by sending WM_TAKE_FOCUS to a client window supporting it.
The client may accept or ignore the offer. If it accepts, the surface will
emit a focus_in signal notifying the compositor that it has received focus.
The compositor should then call wlr_xwayland_surface_activate(surface, true).

This is a more compatible method of giving focus to windows using the
Globally Active input model (see wlr_xwayland_icccm_input_model()) than
calling wlr_xwayland_surface_activate() unconditionally, since there is no
reliable way to know in advance whether these windows want to be focused.
Use the new grab_focus signal as a more reliable way to tell when an
unmanaged (override-redirect) surface wants focus.
jlindgren90 added a commit to jlindgren90/labwc that referenced this pull request Mar 17, 2024
Offer focus by sending WM_TAKE_FOCUS to a client window supporting it.
The client may accept or ignore the offer. If it accepts, the surface will
emit a focus_in signal notifying the compositor that it has received focus.
The compositor should then call wlr_xwayland_surface_activate(surface, true).

This is a more compatible method of giving focus to windows using the
Globally Active input model (see wlr_xwayland_icccm_input_model()) than
calling wlr_xwayland_surface_activate() unconditionally, since there is no
reliable way to know in advance whether these windows want to be focused.

labwc#1142
jlindgren90 added a commit to jlindgren90/labwc that referenced this pull request Mar 17, 2024
Use the new grab_focus signal as a more reliable way to tell when an
unmanaged (override-redirect) surface wants focus.

labwc#1142
jlindgren90 added a commit to jlindgren90/labwc that referenced this pull request Apr 8, 2024
Offer focus by sending WM_TAKE_FOCUS to a client window supporting it.
The client may accept or ignore the offer. If it accepts, the surface will
emit a focus_in signal notifying the compositor that it has received focus.
The compositor should then call wlr_xwayland_surface_activate(surface, true).

This is a more compatible method of giving focus to windows using the
Globally Active input model (see wlr_xwayland_icccm_input_model()) than
calling wlr_xwayland_surface_activate() unconditionally, since there is no
reliable way to know in advance whether these windows want to be focused.

labwc#1142
jlindgren90 added a commit to jlindgren90/labwc that referenced this pull request Apr 8, 2024
Use the new grab_focus signal as a more reliable way to tell when an
unmanaged (override-redirect) surface wants focus.

labwc#1142
jlindgren90 added a commit to jlindgren90/labwc that referenced this pull request Apr 19, 2024
Offer focus by sending WM_TAKE_FOCUS to a client window supporting it.
The client may accept or ignore the offer. If it accepts, the surface will
emit a focus_in signal notifying the compositor that it has received focus.
The compositor should then call wlr_xwayland_surface_activate(surface, true).

This is a more compatible method of giving focus to windows using the
Globally Active input model (see wlr_xwayland_icccm_input_model()) than
calling wlr_xwayland_surface_activate() unconditionally, since there is no
reliable way to know in advance whether these windows want to be focused.

labwc#1142
jlindgren90 added a commit to jlindgren90/labwc that referenced this pull request Apr 19, 2024
Use the new grab_focus signal as a more reliable way to tell when an
unmanaged (override-redirect) surface wants focus.

labwc#1142
jlindgren90 added a commit to jlindgren90/labwc that referenced this pull request May 11, 2024
Offer focus by sending WM_TAKE_FOCUS to a client window supporting it.
The client may accept or ignore the offer. If it accepts, the surface will
emit a focus_in signal notifying the compositor that it has received focus.
The compositor should then call wlr_xwayland_surface_activate(surface, true).

This is a more compatible method of giving focus to windows using the
Globally Active input model (see wlr_xwayland_icccm_input_model()) than
calling wlr_xwayland_surface_activate() unconditionally, since there is no
reliable way to know in advance whether these windows want to be focused.

labwc#1142
jlindgren90 added a commit to jlindgren90/labwc that referenced this pull request May 11, 2024
Use the new grab_focus signal as a more reliable way to tell when an
unmanaged (override-redirect) surface wants focus.

labwc#1142
@kyak
Copy link
Contributor

kyak commented Jun 8, 2024

@jlindgren90 as usual, I was rebasing your branch on top of labwc/master. This time there's a build error:

[238/474] Compiling C object subprojects/wlroots/libwlroots.a.p/backend_drm_libliftoff.c.o
FAILED: subprojects/wlroots/libwlroots.a.p/backend_drm_libliftoff.c.o
cc -Isubprojects/wlroots/libwlroots.a.p -Isubprojects/wlroots -I../labwc/subprojects/wlroots -Isubprojects/wlroots/include -I../labwc/subprojects/wlroots/include -Isubprojects/wlroots/protocol -Isubprojects/wlroots/render/gles2/shaders -Isubprojects/wlroots/render/vulkan/shaders -Isubprojects/wlroots/backend/drm -I/usr/include/libdrm -I/usr/include/pixman-1 -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -Werror -std=c11 -O0 -g -DWLR_USE_UNSTABLE -DWLR_LITTLE_ENDIAN=1 -DWLR_BIG_ENDIAN=0 -Wundef -Wlogical-op -Wmissing-include-dirs -Wold-style-definition -Wpointer-arith -Winit-self -Wstrict-prototypes -Wimplicit-fallthrough=2 -Wendif-labels -Wstrict-aliasing=2 -Woverflow -Wmissing-prototypes -Walloca -Wno-missing-braces -Wno-missing-field-initializers -Wno-unused-parameter -fmacro-prefix-map=../labwc/subprojects/wlroots/= -march=x86-64 -mtune=generic -O2 -pipe -fno-plt -fexceptions -Wp,-D_FORTIFY_SOURCE=3 -Wformat -Werror=format-security -fstack-clash-protection -fcf-protection -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -g -ffile-prefix-map=/home/peselnikmg/build/labwc-git/src=/usr/src/debug/labwc-git -flto=auto -fPIC -MD -MQ subprojects/wlroots/libwlroots.a.p/backend_drm_libliftoff.c.o -MF subprojects/wlroots/libwlroots.a.p/backend_drm_libliftoff.c.o.d -o subprojects/wlroots/libwlroots.a.p/backend_drm_libliftoff.c.o -c ../labwc/subprojects/wlroots/backend/drm/libliftoff.c
../labwc/subprojects/wlroots/backend/drm/libliftoff.c: In function ‘crtc_commit’:
../labwc/subprojects/wlroots/backend/drm/libliftoff.c:439:19: error: too few arguments to function ‘liftoff_output_apply’
  439 |         int ret = liftoff_output_apply(crtc->liftoff, req, flags);
      |                   ^~~~~~~~~~~~~~~~~~~~
In file included from ../labwc/subprojects/wlroots/backend/drm/libliftoff.c:3:
/usr/include/libliftoff.h:85:1: note: declared here
   85 | liftoff_output_apply(struct liftoff_output *output, drmModeAtomicReq *req,
      | ^~~~~~~~~~~~~~~~~~~~
[251/474] Compiling C object subprojects/wlroots/libwlroots.a.p/backend_drm_drm.c.o
ninja: build stopped: subcommand failed.

Can you please have a look?

@jlindgren90
Copy link
Contributor Author

@kyak I am not sure as I am not using libliftoff, but it looks like a new version was released recently and broke backwards compatibility. You may need to cherry-pick https://gitlab.freedesktop.org/wlroots/wlroots/-/commit/1bfd0613ca4865c73250488375310fdd6a4c1b5d.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants