Created attachment 184617 [details] [review]
edge-tiling: Fix cancelling maximize tiling

If a window can not be tiled, e.g. due to its minimum size hints,
dragging away from the top after activating the maximize tile preview
does not cancel the maximization request, the only way to do so is by
hitting Escape.
To fix, reset the tiling state in the maximize-tile code path as
well if necessary.

Review of attachment 184617 [details] [review]:

This leads to the comment above clearly mismatching the code - the logic error in that (whatever it is) is left.

I think this is 3.0.1 material in any case, don't have a lot of confidence in my understanding of this.

Created attachment 185241 [details] [review]
edge-tiling: Fix cancelling maximize tiling

(In reply to comment #2)
> Review of attachment 184617 [details] [review]:
> 
> I think this is 3.0.1 material in any case, don't have a lot of confidence in
> my understanding of this.

OK, slightly different patch version which is hopefully clearer.

Review of attachment 185241 [details] [review]:

OK, I'm still not happy with this. This code doesn't read like it has logic to it.

What if you fixed can_tile_maximized and can_tile_side_by_side so they only apply to the *hints* of the window, not the state, then first did logic not changing window->tile_mode:

 new_tile_mode = NONE
 if can_title_side_by_side:
   if on_left_edge:
      new_tile_mode = LEFT
   else if on_left_edge:
      new_tile_mode = RIGHT
 if new_tile_mode = NONE && can_tile_maximized:
   if on_top_edge:
      new_tile_mode = MAXIMIZED

Then after that, you combined window->tile_mode with new_tile_mode in whatever way is appropriate (I still don't quite understand what's going on, but hopefully if that code is separated out it will be somewhat clear.)

Basically, after multiple wrong attempts to update this code, I don't want to take a patch until I can understand what is going on without straining my brain, because apparently when I've *thought* I've understood what is going on in the past I've just been suffering from brain strain.

Created attachment 186226 [details] [review]
edge-tiling: Fix cancelling maximize tiling

(In reply to comment #4)
> What if you fixed can_tile_maximized and can_tile_side_by_side so they only
> apply to the *hints* of the window, not the state

I'd like to try without that suggestion - both can_tile_maximized and can_tile_side_by_side use ALLOWS_RESIZE(), which already mixed hints and state for maximized windows when tiling was implemented; extending it for tiling seemed like a rather logical step. There's also ALLOWS_RESIZE_EXCEPT_HINTS(), so adding an additional ALLOWS_RESIZE_ONLY_HINTS() looks like a bit too much to me :-)


> then first did logic not changing window->tile_mode:
> 
>  new_tile_mode = NONE
>  if can_title_side_by_side:
>    if on_left_edge:
>       new_tile_mode = LEFT
>    else if on_left_edge:
>       new_tile_mode = RIGHT
>  if new_tile_mode = NONE && can_tile_maximized:
>    if on_top_edge:
>       new_tile_mode = MAXIMIZED
> 
> Then after that, you combined window->tile_mode with new_tile_mode in whatever
> way is appropriate (I still don't quite understand what's going on, but
> hopefully if that code is separated out it will be somewhat clear.)

I separated the code slightly differently, e.g. by setting a local variable strictly according to the current cursor position, and then using that to set window->tile_mode as appropriate - not the exact change you suggest, but should be much clearer anyway (in fact, it even allows getting rid of the confusing (tile_mode == NONE) check)


> Basically, after multiple wrong attempts to update this code, I don't want to
> take a patch until I can understand what is going on without straining my
> brain, because apparently when I've *thought* I've understood what is going on
> in the past I've just been suffering from brain strain.

OK, hopefully this iteration doesn't go too hard on your brain - I'd still like to see a (proper!) fix in, but let's not forget that the bug only shows up if windows can maximize, but not tile (which basically translates to some windows on small screens and devices in portrait orientation - neither one should be too common), so if we have to delay a patch for 3.2, it wouldn't be the end of the world either ...

Review of attachment 186226 [details] [review]:

I'm still having a lot of trouble deciphering this code. As far as I can tell,

	/* Only unset the window's tile mode if it allows resizing, e.g.
    	 * it is resizable and not tiled or maximized.
	 */
	if (META_WINDOW_ALLOWS_RESIZE (window))
	    window->tile_mode = META_TILE_NONE;

Is there because a) resetting the tile mode to NONE without unsetting maximized_vertically leaves things in a funny state b) the edge-tiling drag behavior isn't applicable to maximized or tiled windows until they have been shook loose. So it's basically about a random subset of flags that ALLOWS_RESIZE() happens to check, not whether the window is resizable.

Why not put the entire block as:

  if (meta_prefs_get_edge_tiling () && 
       !META_WINDOW_MAXIMIZED (w) && 
       !META_WINDOW_TILED_SIDE_BY_SIDE(w))
    {

Make it explicitly about the case where the user is dragging a not-currently-tiled window about. Then it can just *always* update tile_mode to a new value. When the user is dragging a not-tiled window about, there's no hysteresis - the tile mode is determined by the pointer position.

If you do that, then it doesn't really matter if can_tile_side_by_side / can_tile_maximized accidentally happen to check the current state, since we only ever call them for non-tiled/maximized windows. I still think it's confusing that they depend on current state - you wouldn't expect that can_tile_side_by_side() would be false for a window that is already tiled! but it's not a big deal.

Created attachment 186603 [details] [review]
edge-tiling: Fix cancelling maximize tiling

(In reply to comment #6)
> Review of attachment 186226 [details] [review]:
> 
> I'm still having a lot of trouble deciphering this code. As far as I can tell,
> 
>     /* Only unset the window's tile mode if it allows resizing, e.g.
>          * it is resizable and not tiled or maximized.
>      */
>     if (META_WINDOW_ALLOWS_RESIZE (window))
>         window->tile_mode = META_TILE_NONE;
> 
> Is there because a) resetting the tile mode to NONE without unsetting
> maximized_vertically leaves things in a funny state b) the edge-tiling drag
> behavior isn't applicable to maximized or tiled windows until they have been
> shook loose.

Yes.


> Why not put the entire block as:
> 
>   if (meta_prefs_get_edge_tiling () && 
>        !META_WINDOW_MAXIMIZED (w) && 
>        !META_WINDOW_TILED_SIDE_BY_SIDE(w))

Sure.


> Make it explicitly about the case where the user is dragging a
> not-currently-tiled window about. Then it can just *always* update tile_mode to
> a new value. When the user is dragging a not-tiled window about, there's no
> hysteresis - the tile mode is determined by the pointer position.

Not entirely - the size hints may still prevent a window from being tiled, so meta_window_can_tile_side_by_side()/meta_window_can_tile_maximized() are still needed.

Review of attachment 186603 [details] [review]:

::: src/core/window.c
@@ +8231,2 @@
         {
+          if (meta_window_can_tile_side_by_side (window))

I wasn't actually saying that you should get rid of the check - it's clearly still needed. But what I was saying is that I don't see a reason why window->tile_mode should be un-updated in this case - if I have a window that I can't tile side-by-side and I drag it, with no intermediate positions from wanting to maximize at the top of the screen to the side of the screen, then it should get a tile mode of NONE, not be left with MAXIMIZED, so the check should be moved into the outer if as an && an not be nested inside.

This is obviously a tiny difference in practical behavior, but I think the code is considerably clearer if tile_mode is always set. (And if it *doesn't* work in practice then I still don't understand the code ;-)

Created attachment 186606 [details] [review]
edge-tiling: Fix cancelling maximize tiling

(In reply to comment #8)
> This is obviously a tiny difference in practical behavior, but I think the code
> is considerably clearer if tile_mode is always set. (And if it *doesn't* work
> in practice then I still don't understand the code ;-)

Nah, I can't see a reason why it shouldn't work either :-)

The difference in behavior is tiny and should only affect corner cases - and I'd even argue that the difference is for the better (e.g. top corners triggering maximization when side-by-side tiling is unavailable rather than being non-reactive), so ...

Review of attachment 186606 [details] [review]:

I really like this version because it looks trivially correct when I read it! (even if it was far from trivial to find this version). It seems to work well in my testing. Let's go with it.

Attachment 186606 [details] pushed as 3543782 - edge-tiling: Fix cancelling maximize tiling

(In reply to comment #10)
> (even if it was far from trivial to find this version).

Oh yes :-)