ssokolow / quicktile

Adds window-tiling hotkeys to any X11 desktop. (An analogue to WinSplit Revolution for people who don't want to use Compiz Grid)
https://ssokolow.com/quicktile/
GNU General Public License v2.0
860 stars 78 forks source link

Quicktile wastes space left clear by a small panel covering less than half of a screen dimension (GTK3 branch regression) #108

Closed fidergo-stephane-gourichon closed 4 years ago

fidergo-stephane-gourichon commented 4 years ago

Context

gtk3_port branch

How to reproduce

Expected

Observed

quicktile_regression

ssokolow commented 4 years ago

So it doesn't get lost, I'll reiterate that this is a known regression caused by...

  1. gtk.gdk.Region getting replaced with cairo.Region
  2. cairo.Region's Python bindings being such a buggy mess that I had to write my own solution to make progress.
  3. My custom solution not yet having caught up, feature-wise.
ssokolow commented 4 years ago

Note to Self: See also #64 and #65 when resolving this and designing regression tests.

ssokolow commented 4 years ago

I like to use real-world numbers in my regression tests when possible.

Could you post the results of quicktile --debug so I can use your DEBUG: Loaded monitor geometry and DEBUG: Gathered _NET_WM_STRUT_PARTIAL value: values?

ssokolow commented 4 years ago

Status update: I'm part-way through refactoring to solve this by deferring the integration of panel reservations until after a candidate rectangle for the window has been generated.

It'll mean that, in the common case, the row closest to the panel will get short shrift but "What's ~10-20px between friends?" and I can always special-case "panel extends the full length of the monitor's edge" to crop the usable rectangle at the beginning once I have more automated tests.

Speaking of automated tests, the part I'm currently caught up on is the subtract or, rather, the new moved_off_of method I've re-implemented it in terms of in the un-pushed changes. (Specifically, self.intersect(self.moved_off_of(other)), to keep the tricky math unified in one place.)

One of my unrelated tests managed to trigger an edge case I knew about but had put off, and now I'm trying to decide the least disruptive way to fix it properly.

(moved_off_of operates by bumping the window the shortest distance that will resolve the intersection and, without being intersected with the un-moved version, is also used by the Ctrl+Alt+Shift+Numpad code to move wndows without resizing them... and my test rectangle is less than the panel's width from the screen edge, so the naive algorithm shoves it off the screen, causing things that depend on it to return failure (empty rectangles get converted to None to make that clear) when clipping to screen bounds.)

EDIT: The problem is trying to find a way to keep the "what direction should I go?" code unified.

For moved_off_of, the theoretically ideal solution would be a constrain_to argument that would be a monitor rectangle but, for subtract, you don't want to move the rectangle... just clip off as little of the area as possible.

At the moment, I'm planning to try changing the algorithm from "minimum distance moved" to "maximum area preserved for old.intersect(new)" to see if that still preserves the desired behaviour for direct use of moved_off_of while removing the need for subtract to take a constraining rectangle.

If it works, it'll result in calculating the intersection twice for subtraction, but, as they say, premature optimization is the root of all evil.

fidergo-stephane-gourichon commented 4 years ago

Thanks for the long and highly technical status update. It goes quickly from topic to topic regarding quicktile internals. May be interesting to anyone willing to dive into code.

Could you post the results of quicktile --debug so I can use your DEBUG: Loaded monitor geometry and DEBUG: Gathered _NET_WM_STRUT_PARTIAL value: values?

Sure.

On "old" branch (not gtk3):

DEBUG: Gathered _NET_WM_STRUT_PARTIAL value: [StrutPartial(left=0, right=0, top=0, bottom=33, left_start_y=0, left_end_y=0, right_start_y=0, right_end_y=0, top_start_x=0, top_end_x=0, bottom_start_x=1022, bottom_end_x=1919)] DEBUG: Usable desktop region calculated as: Region()

On gtk3_port branch, v0.2.1-273-ga2b0e9f

DEBUG: Loaded monitor geometry: [Rectangle(x=0, y=0, width=1920, height=1080)] DEBUG: Gathered _NET_WM_STRUT_PARTIAL value: [StrutPartial(left=0, right=0, top=0, bottom=33, left_start_y=0, left_end_y=0, right_start_y=0, right_end_y=0, top_start_x=0, top_end_x=0, bottom_start_x=1022, bottom_end_x=1919)] DEBUG: Usable desktop region calculated as: Region()

ssokolow commented 4 years ago

Thanks for the long and highly technical status update. It goes quickly from topic to topic regarding quicktile internals. May be interesting to anyone willing to dive into code.

I've been trying to incorporate the relevant bits into the API docs, both as text and as cross-references.

...and, as I suspected, switching to "maximize area of overlap with original position" seems to have done the trick except for one new pathological case... if the test window is entirely within the reserved area, in which case, all overlaps have zero area and the motion is essentially random.

I made it more deterministic and minimally pathological by adding euclidean distance as a secondary sorting key to fall back to and then wound up factoring out the resulting code into a Rectangle.closest_of(List[Rectangle]) method when I discovered I also needed it for UsableRegion.find_monitor_for(Rectangle).

It's not perfect, but I don't want to get carried away refactoring to plumb in a constrain_within argument for "if the target rectangle is already mostly outside the desktop, moved_off_of will shove it onto the wrong side of the panel" when the ideal behaviour is probably "Let people post issues with --debug output for how the higher-level code managed to request such an odd positioning and wind up deciding that there was no valid motion as a result".

After all, how much of a pressing need is there for target rectangles that are narrower/shorter than the panels on the edges they're trying to occupy?

(A big part of my strategy for maintainability these days is building solid primitives with extensive unit tests that I can trust to match my intuition when I'm working on the higher-level stuff, but learning not to get caught up in making things perfect to the detriment of actually making things was a long, hard journey.)

On gtk3_port branch, v0.2.1-273-ga2b0e9f

Added to the test suite and my local copy is currently passing the test. In fact, the only thing keeping me from pushing an 0.4.0 release candidate for testing is that I want to write some unit tests for my solution to #45.

fidergo-stephane-gourichon commented 4 years ago

I don't have an exact view on everything, seems sane. Well quite a lot on activity on quicktile these days. I guess your degree and other stuff is okay. In all cases, thanks for the past, current and future work!

ssokolow commented 4 years ago

OK. Anyone who wants to test it, the contents of gtk3_port are now the 0.4.0 release candidate.

fidergo-stephane-gourichon commented 4 years ago

I confirm that behavior is now correct (tested on bce6d7fcb840fbf0da641bb6e798a1eff7036a7a which includes c94b95d0e4195e496cedc597ea5f6695a84a8a6b).