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

Implement support for EWMH _NET_WM_MOVERESIZE client messages #3859

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

Conversation

phantamanta44
Copy link

This PR adds support for the EWMH client message _NET_WM_MOVERESIZE, which allows clients to delegate mouse-based movement and resizing to the window manager. It does this by:

  1. adding a handler for the _NET_WM_MOVERESIZE message that emits the request::mouse_move, request::mouse_resize, and request::mouse_cancel signals when a client makes a corresponding request.
  2. adding default handlers for those signals to awful.permissions that carry out the mouse movement/resizing operations using awful.mouse.resize.
  3. adding extra configuration options to the mouse resizing and placement library functions so that they behave the way clients would expect them to (default behaviour is unchanged).

Some implementation notes:

  • The "keyboard movement" and "keyboard resizing" operations defined in the _NET_WM_MOVERESIZE spec are ignored. It's unclear what they're supposed to mean and they are similarly ignored by i3 and bspwm.
  • The _NET_WM_MOVERESIZE spec mentions that a "race condition" is possible in the case where the user has already released the mouse button before the mouse movement message is processed. This is already dealt with by way of awful.mouse.resize cancelling the operation if the mouse button is no longer pressed.
  • It's a little ugly that the request::mouse_move and request::mouse_resize signals and the awful.mouse.client library functions move and resize apparently do the same thing independently of each other. I could not find a way to delegate one to the other without requiring breaking API or behavioural changes, and so I elected, for the time being, to just leave them independent.

I've tested this on my amd64 NixOS system on the following programs:

  • Firefox — fully functional.
  • Thunderbird — fully functional.
  • Steam — fully functional.
  • Visual Studio Code — functional when windowed, but un-maximizing a maximized window seems to put it in a broken state. This also seems to happen on awesome 4.3 without this PR, so it's probably unrelated...?

This should close #2140 and, by extension, #3136.

@codecov
Copy link

codecov bot commented Sep 30, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (aa8c7c6) 91.02% compared to head (f093114) 91.24%.
Report is 28 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3859      +/-   ##
==========================================
+ Coverage   91.02%   91.24%   +0.22%     
==========================================
  Files         901      928      +27     
  Lines       57537    59695    +2158     
==========================================
+ Hits        52372    54469    +2097     
- Misses       5165     5226      +61     
Flag Coverage Δ
gcov 91.24% <96.07%> (+0.22%) ⬆️
luacov 93.89% <98.13%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
lib/awful/mouse/resize.lua 90.17% <100.00%> (+0.27%) ⬆️
objects/client.c 83.94% <ø> (+0.02%) ⬆️
tests/_client.lua 98.75% <100.00%> (+0.03%) ⬆️
tests/test-ewmh-wm_moveresize.lua 100.00% <100.00%> (ø)
lib/awful/permissions/init.lua 88.47% <95.00%> (+0.85%) ⬆️
lib/awful/placement.lua 92.14% <86.66%> (+0.08%) ⬆️
ewmh.c 67.99% <85.36%> (+1.72%) ⬆️

... and 42 files with indirect coverage changes

@actionless
Copy link
Member

it's adding some new api and changing several existing code branches - so would be nice to add some test

@phantamanta44 phantamanta44 force-pushed the ewmh-net-wm-moveresize branch from 231d334 to 6bb8d68 Compare October 3, 2023 13:31
@phantamanta44
Copy link
Author

  • Added documentation
  • Added integration tests for mouse movement/resizing
    • Augmented the test client with an option that equips it with a GTK 3 custom title bar for this purpose
    • Also fixed a bug I found while writing the tests where the mouse offset calculation wasn't accounting for the client border
  • Updated the default awesomerc so that the rule for enabling title bars doesn't apply to clients with requests_no_titlebar = true
    • I assume title bars were originally added to all clients because of lack of _NET_WM_MOVERESIZE support; therefore, this PR should obsolete that rule

@actionless
Copy link
Member

I assume title bars were originally added to all clients because of lack of _NET_WM_MOVERESIZE support; therefore, this PR should obsolete that rule

i think it was no specific reason for doing that, it just wasn't implemented that specific case, but thanks for looking into it 👍

@actionless
Copy link
Member

also you need to do this:

$ rg 'unpack.*='
tests/test-awful-rules.lua
5:local unpack = unpack or table.unpack -- luacheck: globals unpack (compatibility with Lua 5.1)

i think we should eventually move that to gears.table but it is what it is now 🥲

@phantamanta44
Copy link
Author

bleh fixed

@phantamanta44
Copy link
Author

seems like the failed checks are just formatting issues. should i just fix them then squash the last few commits?

@actionless
Copy link
Member

yup, exactly 👍

@phantamanta44 phantamanta44 force-pushed the ewmh-net-wm-moveresize branch from 71cfc9e to 4d6d7a3 Compare October 3, 2023 17:02
actionless
actionless previously approved these changes Oct 3, 2023
Copy link
Member

@Aire-One Aire-One left a comment

Choose a reason for hiding this comment

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

Thank you for this work!

awesomerc.lua Show resolved Hide resolved
lib/awful/permissions/init.lua Show resolved Hide resolved
Aire-One
Aire-One previously approved these changes Nov 17, 2023
Copy link
Member

@Aire-One Aire-One left a comment

Choose a reason for hiding this comment

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

🚀

@phantamanta44
Copy link
Author

phantamanta44 commented Nov 17, 2023

agh, awful.mouse.client passes corner as part of the args for awful.mouse.resize, but the same args table then gets passed down to the placement function used for the resizing, which conflicts with my definition of corner. that's a bit awkward, and i would guess that's why i originally introduced corner_lock

if anything, i would argue that this new behaviour of locking the resizing axes is more "correct" than the existing behaviour, where moving the mouse from a side of the window towards a corner will change from a single-axis resize to a two-axis (i.e. corner) resize. i'm unsure whether this warrants a breaking behavioural change, however

@Elv13
Copy link
Member

Elv13 commented Dec 31, 2023

(finally...) have time to look at this. First of all, wow, this is very well implemented.

Is this ready to be merged, the tests are still failing.

@phantamanta44
Copy link
Author

phantamanta44 commented Jan 1, 2024

there are a couple things holding this back:

  • the tests are failing because of my comment above about corner locking in mouse resizing operations. probably the most sensible way to deal with it here is to apply a "bandage" fix like the corner_lock solution from earlier and to leave breaking behavioural changes to a separate PR, regardless of whether this new behaviour is "more correct" or not

  • i've been running on this patch for a while and i've noticed an issue where some windows ignore screen borders (e.g. a task bar) when they're initially laid out on opening. i'm not sure why it happens yet, but as far as i'm aware, it doesn't happen on the main branch. notable apps include intellij idea, okular, and strawberry music player

i'm a bit busy right now so i haven't had time to deal with this stuff, but i'll get back to it soon™

@phantamanta44
Copy link
Author

  • the tests are failing because of my comment above about corner locking in mouse resizing operations. probably the most sensible way to deal with it here is to apply a "bandage" fix like the corner_lock solution from earlier and to leave breaking behavioural changes to a separate PR, regardless of whether this new behaviour is "more correct" or not

  • i've been running on this patch for a while and i've noticed an issue where some windows ignore screen borders (e.g. a task bar) when they're initially laid out on opening. i'm not sure why it happens yet, but as far as i'm aware, it doesn't happen on the main branch. notable apps include intellij idea, okular, and strawberry music player

what I've just pushed reintroduces the corner_lock thing to deal with the first issue here. after more experimentation, I believe the second issue is actually just #3778, and so it's out of the scope of this PR. at any rate, the tests should pass now

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.

Support _NET_WM_MOVERESIZE
4 participants