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

Add mouse events & friends for Wayland #1672

Closed
wants to merge 38 commits into from
Closed

Add mouse events & friends for Wayland #1672

wants to merge 38 commits into from

Conversation

Caellian
Copy link
Collaborator

@Caellian Caellian commented Nov 5, 2023

Description

This PR is a supplement to #955 (X11 mouse events) - it adds support for the mouse event hook to Wayland.

Progress of this PR:

  • Fix mouse events referencing X11 code
  • Attach event listeners to Wayland
  • Fill in missing data and pass all events to Lua
  • Test mouse hook for all event types
  • Fix broken X11 code
  • Clean up code
    • Debug propagation focus flickering
  • Test again
  • Update documentation in doc/

Optional:

Compatibility

Leave surface event doesn't contain cursor position

Can be handled by statically storing the last known position from the move event in the window. I won't store it in window struct to avoid its layout changing based on build flags but instead create a new static storage in mouse-events.cc.

Keyboard modifier keys aren't provided with button press/release events

I'm not providing modifier keys in Wayland (the table will be empty). I looked into it and supporting that requires me to add dependency on XKB, map scan codes through it to keys, maybe even support keyboard layouts, and storing the state of all tracked modifiers statically.
I feel this functionality should be provided through an additional feature for keyboard support (e.g. BUILD_KEYBOARD_EVENTS) because it basically adds everything needed for that. As such, it might be better to implement that in a separate PR.

  • That PR should probably disable mods on X11 as well if the flag isn't enabled and handle modifiers better (e.g. left vs. right Shift key).
  • Keyboard support requires special handling and isn't supported the best by some compositors (Hyprland: setting interactivity to on_demand completely steals keyboard focus until the layer closes)

Mouse button event produces platform dependent event codes instead of a semantic value

This is explained in detail in the next section:

Breaking changes

  • event.button previously mapped to integers representing mouse buttons because it was passing those values from X11 directly, now event.button is a string value (e.g. left, right, middle) representing a semantic name of a button.

    • left, right, middle correspond to previous 1, 2 and 3 values.
    • event.button will be nil when no semantic name is hardcoded in conky.
    • event.button_code is now provided and contains raw event codes
      • value will always be present for all pointer buttons, but event.button is less platform dependent so event.button_code should only be used as fallback.
  • event.mods no longer reports buttons/keys that aren't modifiers.

  • renamed mod1 to super.

While the previous values could've been kept, I feel this new format will stand the test of time much better and the scripts will be more stable/portable.

Choices

I'm not processing pointer events on Wayland frames because it would make the implementation much more complicated while providing no benefit - just like last_known_positions is static, a frame queue would have to be as well. We don't need complex axis event handling so we can greedily process all events.

  • This might cause larger overhead for movement events due to higher number of Lua dispatches, but I don't think optimizing those warrants the additional processing complexity.

Out of scope

Errors are now handled better in llua.cc. i.e. lua_L will no longer be initialized if lua_load property points to a file that doesn't exist which will make many of the functions in the file return early because they're referencing a blank lua context anyway (and would've failed at a later point).

Bumped toluapp minimum cmake version so CMake stops complaining about the old one no longer being supported.

Allowed Lua libraries to be included in build on Wayland.

Added a macro for ConkyBuildOptions that lets the user running cmake know which of the selected options were disabled by the script and why.

Disabled requirement for BUILD_ARGB feature on Wayland as it's supported out of the box (by almost all compositors I can think of). Even OWN_WINDOW should work as Wayland allows surfaces to be transparent.

X11 Fixes

Enabled propagation of additional events to desktop window such as mouse movement and expose event. Provided a function which handled propagation for most core X11 events properly.

Added optional Xinput2 dependency that enables better (though more CPU intensive) tracking for whether cursor is over conky by capturing global movement events.

  • It's double optional as it falls back to old behavior if not present at runtime. 😄

X11 Error handling

Removed abort() call when X11 errors are received.

Fixing current cursor area detection bug where mouse leave wasn't being detected if a window was overlapping conky proved to be a nuisance - XQueryPointer didn't return correct window under the cursor so I implemented a manual lookup function that checks window properties though XGetWindowAttributes. Call to that function was failing when it was being executed on windows querried directly before by XQueryTree.

Conky should handle X11 errors without closing as they can fail for various reasons which don't mean the application is in invalid state. Instead of calling abort() from x11_error_handler, calls that are crucial for conky to function properly should check for Success at call site and invoke CRIT_ERR with an appropriate message. Documentation also notes that:

when a request terminates with an error, the request has no side effects

I added detection for xcb-util-errors which now enables printing more descriptive error messages (name of failed object (error_code) & request (major, minor)) and added a fallback for error_code when xcb-util-errors isn't installed on a machine.

License

All added code is licensed under GPLv3.

Signed-off-by: Tin Svagelj <[email protected]>
…nky into feature/wayland-mouse-events

Signed-off-by: Tin <[email protected]>
Set initial window scale to 0 because wayland freezes the process when it's not >0.
Provide more information from `llua_mouse_hook` about why the callback is failing.
Prevent call to `llua_init` when provided lua_load script doesn't exist.
- This caused me a headache and now lua_L won't be initialized if the
  script doesn't exist. This prevents a missing file from causing later
  issues which might be harder to catch.
Feature gated X11 in mouse-events.h/.cc files.

Signed-off-by: Tin <[email protected]>
Copy link

netlify bot commented Nov 5, 2023

Deploy Preview for conkyweb canceled.

Name Link
🔨 Latest commit bbd18f5
🔍 Latest deploy log https://app.netlify.com/sites/conkyweb/deploys/655a5bfed045120008722cef

@github-actions github-actions bot added the sources PR modifies project sources label Nov 5, 2023
src/display-wayland.cc Outdated Show resolved Hide resolved
@Caellian
Copy link
Collaborator Author

Caellian commented Nov 7, 2023

Did testing with a modified script from last PR (to account for new data):

preview

All events except mouse_enter and mouse_leave seem to work.

  • Didn't call llua_mouse_hook(event); in those events 🤦🏻

Remove some code added by accident; reduce change list.
Fix mouse_enter and mouse_leave events.

Signed-off-by: Tin <[email protected]>
Fix most issues with X11 event propagation.
Remove X11 code from mouse-events.cc/.h completely.

CMake:
Update toluapp cmake_minimum_required to stop it complaining during build.
Fix ConkyPlatformCheck.cmake not providing access to cairo, imlib2 and rsvg to Wayland.
Move BUILD_MOUSE_EVENTS includes under BUILD_GUI so they're not duplicated.

Signed-off-by: Tin <[email protected]>
@github-actions github-actions bot added the 3rdparty Issue or PR that suggests changes to 3rd party dependencies label Nov 8, 2023
@@ -227,6 +230,29 @@ bool display_output_x11::shutdown() {
return true;
}

inline void propagate_unconsumed_event(XEvent &ev, bool is_consumed) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function was tested thoroughly on KDE plasma with background widgets and such.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This code portion is now additionally invoked on cursor movement so that e.g. desktop shortcuts get proper on-hover highlighting.

Signed-off-by: Tin <[email protected]>
@Caellian Caellian changed the title [WIP] Add mouse events for Wayland Add mouse events for Wayland Nov 8, 2023
Update /doc

Signed-off-by: Tin <[email protected]>
@github-actions github-actions bot added the documentation Issue or PR that suggests documentation improvements label Nov 8, 2023
src/display-x11.cc Outdated Show resolved Hide resolved
@Caellian Caellian changed the title Add mouse events for Wayland [WIP] Add mouse events for Wayland Nov 8, 2023
@Caellian
Copy link
Collaborator Author

Caellian commented Nov 9, 2023

X11 errors now give human readable error messages when compiled with xcb-util-errors (autodetected, has to be installed during build, detected at runtime w/ fallback, not an error if it's not).
pow

I'll print them with DBGP though because they can be very spammy now with BUILD_MOUSE_EVENTS+BUILD_XINPUT and curor tracking.

Additional tweaks and docs improvements

Signed-off-by: Tin <[email protected]>
Cleanup modifiers
- This is preparation so that once Wayland adds support for these
  they're not an alien X11 bitset.
- Removed ones that aren't actual modifiers.

Tidy up code

Signed-off-by: Tin <[email protected]>
Cleanup debug statements

Signed-off-by: Tin <[email protected]>
@Caellian
Copy link
Collaborator Author

Caellian commented Nov 10, 2023

@brndnmtthws Ready for review 🥲

1 runner seems to have pulled an old file before last ammend.

@Caellian
Copy link
Collaborator Author

I'll print them with DBGP though because they can be very spammy now with BUILD_MOUSE_EVENTS+BUILD_XINPUT and curor tracking.

I reverted back to XQueryPointer (needed to look up last descendant), so this error doesn't occur anymore, but I didn't revert removing abort().

@Caellian
Copy link
Collaborator Author

Caellian commented Nov 10, 2023

Added nice notices to CMake so features don't just disappear. It also untangles the build script for such features (no more if&else handling) so the script is much clearer, and makes it very easy to modify dependencies.

2023-11-10-122627_hyprshot

This is completely out of scope, but I did it along the way as it makes it clearer why CI is failing in some cases, esp. when adding options.

Reformat files and hope it invalidates CI cache :)

Signed-off-by: Tin <[email protected]>
Fix half-saved change in display-wayland.cc

Signed-off-by: Tin <[email protected]>
@Caellian Caellian changed the title [WIP] Add mouse events for Wayland [WIP] Add mouse events & friends for Wayland Nov 10, 2023
@Caellian Caellian changed the title [WIP] Add mouse events & friends for Wayland Add mouse events & friends for Wayland Nov 10, 2023
@Caellian
Copy link
Collaborator Author

All tweaks are done, can't seem to force CI to use the new x11.cc file that was pushed like 10 commits ago, and also reformatted with clang-format since then (so hash should be different).

This ended up being a bit larger PR than I thought 😆, but it tackles a lot of issues with both the previous implementation and Wayland support.

Let me know whether there's anything you'd like me to change @brndnmtthws.

Caellian and others added 7 commits November 11, 2023 00:45
There was no way to propagate move events otherwise

Also, clang-format decided to kick in and reformat Xinput related
code...

Signed-off-by: Tin <[email protected]>
Cleanup number namespaces so they use cstdint types instead of stdint.h
types. Latter could've caused some issues in the future.

Simplify valuator mask checking expression.

Signed-off-by: Tin <[email protected]>
This resolves #1669.

Also bumped some dependencies.
@github-actions github-actions bot added the web Issue or PR related to documentation website label Nov 19, 2023
Copy link
Owner

@brndnmtthws brndnmtthws left a comment

Choose a reason for hiding this comment

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

Thanks again for the PR. In the future it would be nice to have separate changes (like the formatting or VS Code extension stuff) broken out into separate PRs just to make the review process a little easier, but I understand that is more work as well.

@github-actions github-actions bot removed the web Issue or PR related to documentation website label Nov 19, 2023
@brndnmtthws
Copy link
Owner

This is merged from afa5f32 on, but GitHub seems to have been unable to handle the merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3rdparty Issue or PR that suggests changes to 3rd party dependencies documentation Issue or PR that suggests documentation improvements gh-actions Issue or PR that suggest changing GitHub actions sources PR modifies project sources
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants