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

Layer shell support #53

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

Conversation

DemiMarie
Copy link
Contributor

@DemiMarie DemiMarie commented Dec 1, 2024

This is almost completely working under Wayland. To test, use a compositor that supports Layer Shell, such as KWin. KWin can be spawned nested inside an X11 window, such as what Qubes OS provides.

Issues:

  1. The window is too short: fixed by explicitly setting the menu size every time the window is opened.
  2. The configuration options that control where the app menu appears did not work: fixed by explicitly checking for them and anchoring the window to the correct corner
  3. Tests code was included in the production script: I moved the test code to my own local branch.
  4. mouse mode is interpreted as bottom-left (KDE) or top-left (otherwise): will be fixed by providing mouse information via IPC.

@DemiMarie DemiMarie force-pushed the layer-shell branch 3 times, most recently from 56c9ea8 to 0384b93 Compare December 1, 2024 07:36
@marmarta
Copy link
Member

marmarta commented Dec 1, 2024

Yay, this looks great! I see you already know about the config options for menu location, so I won't do a review with that :D
BTW, by default, the menu appears not exactly in top-left corner, but next to mouse.

@DemiMarie
Copy link
Contributor Author

Yay, this looks great! I see you already know about the config options for menu location, so I won't do a review with that :D BTW, by default, the menu appears not exactly in top-left corner, but next to mouse.

With this patch, the menu will appear in exactly the corner specified by configuration. Is that okay?

@marmarek
Copy link
Member

marmarek commented Dec 2, 2024

pylint complains, the stuff related to mockups (protected access, wrong import place) you can ignore with a comment, but too long lines want fixing

qubes_menu/appmenu.py Outdated Show resolved Hide resolved
qubes_menu/appmenu.py Outdated Show resolved Hide resolved
@marmarek
Copy link
Member

marmarek commented Dec 2, 2024

openQA says https://openqa.qubes-os.org/tests/120587#step/simple_gui_apps/6
good: no more border
bad: in the center of the screen, very small

@marmarta
Copy link
Member

marmarta commented Dec 2, 2024

looks like some minimal size, the size-setting seems to have failed

@DemiMarie
Copy link
Contributor Author

As per discussion with @marmarta in Matrix, the current plan is:

  • Initially, or if no position information is provided, launch the application in a corner. The default will be bottom-left under KDE and top-left otherwise.
  • In a future PR, the platform-specific plugin will provide the needed information.

@qubesos-bot
Copy link

qubesos-bot commented Dec 2, 2024

OpenQA test summary

Complete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2024121602-4.3&flavor=pull-requests

Test run included the following:

New failures, excluding unstable

Compared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2024111705-4.3&flavor=update

  • system_tests_basic_vm_qrexec_gui

    • TC_30_Gui_daemon: test_002_clipboard_200k (failure)
      AssertionError: '' != 'test123ab\ntest123ab\ntest123ab\ntest123a[21...
  • system_tests_extra

    • TC_00_QVCTest_debian-12-xfce: test_010_screenshare (failure)
      AssertionError: 7.933165427175505 not less than 2.0
  • system_tests_kde_gui_interactive

    • gui_keyboard_layout: wait_serial (wait serial expected)
      # wait_serial expected: "echo -e '[Layout]\nLayoutList=us,de' | sud...
  • system_tests_guivm_vnc_gui_interactive

    • guivm_manager: unnamed test (unknown)
    • guivm_manager: Failed (test died)
      # Test died: no candidate needle with tag(s) 'manager-vm-settings' ...

Failed tests

6 failures
  • system_tests_basic_vm_qrexec_gui

    • TC_30_Gui_daemon: test_002_clipboard_200k (failure)
      AssertionError: '' != 'test123ab\ntest123ab\ntest123ab\ntest123a[21...
  • system_tests_extra

    • TC_00_QVCTest_debian-12-xfce: test_010_screenshare (failure)
      AssertionError: 7.933165427175505 not less than 2.0
  • system_tests_kde_gui_interactive

    • gui_keyboard_layout: wait_serial (wait serial expected)
      # wait_serial expected: "echo -e '[Layout]\nLayoutList=us,de' | sud...

    • gui_keyboard_layout: Failed (test died)
      # Test died: command 'test "$(cd ~user;ls e1*)" = "$(qvm-run -p wor...

  • system_tests_guivm_vnc_gui_interactive

    • guivm_manager: unnamed test (unknown)
    • guivm_manager: Failed (test died)
      # Test died: no candidate needle with tag(s) 'manager-vm-settings' ...

Fixed failures

Compared to: https://openqa.qubes-os.org/tests/119126#dependencies

3 fixed
  • system_tests_extra

    • TC_00_QVCTest_whonix-gateway-17: test_010_screenshare (failure)
      ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^... AssertionError: 0 == 0
  • system_tests_basic_vm_qrexec_gui_zfs

    • switch_pool: Failed (test died)
      # Test died: command 'dnf install -y ./zfs-release.rpm' failed at /...
  • system_tests_audio@hw1

Unstable tests

  • system_tests_audio

    TC_20_AudioVM_PipeWire_fedora-40-xfce/test_260_audio_mic_enabled_switch_audiovm (1/5 times with errors)
    • job 117586 AssertionError: too short audio, expected 10s, got 0.00013605442176...
  • system_tests_audio@hw1

    TC_20_AudioVM_PipeWire_fedora-40-xfce/test_260_audio_mic_enabled_switch_audiovm (1/5 times with errors)
    • job 117586 AssertionError: too short audio, expected 10s, got 0.00013605442176...

@marmarek
Copy link
Member

marmarek commented Dec 3, 2024

Now it opens in the correct corner (at least in KDE), but it's still small: https://openqa.qubes-os.org/tests/120796#step/simple_gui_apps/6

@DemiMarie
Copy link
Contributor Author

Now it opens in the correct corner (at least in KDE), but it's still small: https://openqa.qubes-os.org/tests/120796#step/simple_gui_apps/6

The problem is that when running as a daemon, self.main_window.get_allocated_width() and self.main_window.get_allocated_height() both return 1. Patch coming.

@marmarek
Copy link
Member

marmarek commented Dec 6, 2024

Now it opens in a correct place: https://openqa.qubes-os.org/tests/121008#step/simple_gui_apps/4 :)
(it looks to overlap the panel a bit, but it's only because the mouse is in the right bottom corner, which is a gesture that among other things moves the panel up a bit)

@marmarek
Copy link
Member

marmarek commented Dec 6, 2024

But mypy complains, about None not handled in few places. If it's false positive, AFAIR adding assert ... is not None should do the trick.

Copy link

codecov bot commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 34.00000% with 33 lines in your changes missing coverage. Please review.

Project coverage is 81.37%. Comparing base (8c75256) to head (8d2ebd7).

Files with missing lines Patch % Lines
qubes_menu/appmenu.py 34.00% 33 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #53      +/-   ##
==========================================
- Coverage   82.19%   81.37%   -0.83%     
==========================================
  Files          22       22              
  Lines        2342     2378      +36     
==========================================
+ Hits         1925     1935      +10     
- Misses        417      443      +26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

qubes_menu/appmenu.py Outdated Show resolved Hide resolved
This adds Wayland support on compositors that support the
wlr-layer-shell protocol, which includes KWin, Sway, COSMIC, niri, Mir,
GameScope, and Jay.  The only major compositors without support for
wlr-layer-shell are Mutter, which is generally only used by GNOME, and
Weston, which is not a general-purpose desktop compositor.
@marmarta
Copy link
Member

visually this looks good, can we get an OpenQA run? @marmarek

@marmarta
Copy link
Member

Ok, it looks fine now. Does clicking away from the menu work? (as in, clicking elsewhere - anywhere, hopefully, but I assume wayland has bugs here too)

@DemiMarie
Copy link
Contributor Author

Ok, it looks fine now. Does clicking away from the menu work? (as in, clicking elsewhere - anywhere, hopefully, but I assume wayland has bugs here too)

As discussed on the call: Yes, except for clicking on unused areas of the taskbar, just like the X11 menu.

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.

4 participants