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

Fix window resizing not always updating layout #1378

Closed
wants to merge 1 commit into from

Conversation

overhacked
Copy link

Fixes #1199: resizing a window didn't always update
the layout split. The underlying reason is that the
mouse down coordinates for window resizing are not
always within the CGRect of the window's frame;
often they are just outside the window's frame when
the resize cursor appears.

I changed the event tap location to kCGAnnotatedSessionEventTap,
which includes the field kCGMouseEventWindowUnderMousePointerThatCanHandleThisEvent
in the event metadata that always accurately identifies the window
that the click affects. This does alter the point in the input
stack at which Yabai observes events, but examining EVENT_TAP_CALLBACK(mouse_handler)
and user-testing the mouse features seems to show that changing
the observation point does not cause any regression.

I might be wrong about this. Please let me know if changing
the event tap location is a bad idea, and I will try to find
another workaround for this problem.

Fixes koekeishiya#1199: resizing a window didn't always update
the layout split. The underlying reason is that the
mouse down coordinates for window resizing are not
always within the `CGRect` of the window's frame;
often they are just outside the window's frame when
the resize cursor appears.

I changed the event tap location to `kCGAnnotatedSessionEventTap`,
which includes the field `kCGMouseEventWindowUnderMousePointerThatCanHandleThisEvent`
in the event metadata that always accurately identifies the window
that the click affects. This **does** alter the point in the input
stack at which Yabai observes events, but examining `EVENT_TAP_CALLBACK(mouse_handler)`
and user-testing the mouse features seems to show that changing
the observation point does not cause any regression.
@Liquidmantis
Copy link

Ooooh! I hadn't seen the issue but I'm glad to know it's not just me. Any chance this fix would also resolve the issue where Yabai doesn't always refresh when a window is dragged?

@overhacked
Copy link
Author

It might, because, before this fix, Yabai was matching mouse-down cursor coordinates to window frame extents. I can't imagine why that would go wrong when you have to drag from inside a window's frame in order to move it, but it seems reasonable that getting the window ID directly from the event stream might eliminate ambiguity. Let me know if this does resolve the issue.

@koekeishiya
Copy link
Owner

This doesn't actually fix the core issue of #1199 , which is caused by event order. https:/koekeishiya/yabai/blob/master/src/mouse.c#L3 is the logic that causes problem, because the AX API has not yet reported that the window size has changed, so we use an outdated cached value and perform an incorrect comparison, causing the layout not to update. This code is triggered from a mouse-up event.

If we actually fail to determine the window on a mouse-down event as you mention in the description, that would be another bug.

@overhacked
Copy link
Author

tl;dr: Agreed, this is only a partial fix for #1199. Would you merge but leave the issue open?

Explanation

During troubleshooting, I added some debug statements to the code to print:

  • window_id and position/size (via new_frame) during every WINDOW_RESIZED event
  • Position/size from both g_mouse_state.window->frame and g_mouse_state.window_frame during every MOUSE_DRAGGED and MOUSE_UP event

What I discovered was that sometimes the size in g_mouse_state.window->frame was not being updated even though I could see accurate data in messages from WINDOW_RESIZED. Looking at the unchanging sizes being printed from the MOUSE_DRAGGED events led me to realize that they were for a different window. Adding a few more debug print statements confirmed that the window_id values were different between what MOUSE_DOWN assigned to g_mouse_state.window (and cached in .window_frame) and what WINDOW_RESIZED events were printing. This is what led to this patch.

I never observed the case of the late-arriving WINDOW_RESIZED event causing a failure of the layout to update, but I don't doubt that it occasionally happens and that a fix for that issue is necessary to completely eliminate the layout falling out of sync with actual window sizes. I think the failure mode of late WINDOW_RESIZED events is that the layout is partially out of sync with the new window size, while the mode of misidentifying the window being resized is that the layout misses the entire resize operation.

Now that I've rambled out my inner thoughts... I agree that this does not completely close #1199. Should I open a separate issue, or would you consider merging this as a partial fix referencing that issue and leave it open?

@overhacked
Copy link
Author

overhacked commented Sep 2, 2022

I did some further investigation and implemented a preliminary fix to handle the late WINDOW_RESIZED events, but it yielded broken behavior. The behavior from the fix I observed was that the affected window sizes seemed to "wobble" while the late events were being handled.

I added some debug prints of the before & after sizes seen by the WINDOW_RESIZED event handler. All of the late WINDOW_RESIZED events I was seeing were those generated by yabai to adjust the layout via mouse_drop_try_adjust_bsp_grid(), in response to the MOUSE_UP event. The size in the late resize event was very far off from the last on-time, before MOUSE_UP, resize event, and the difference was approximately half the delta (the other half being "given" to the other window in the split view).

See my non-working patch here: overhacked/yabai@b2a995f

@koekeishiya
Copy link
Owner

Made some similar adjustments in d4aa4c9

#1199 can probably be fixed by using the faster CG notification system, but that API can only track a limited number of windows.

@koekeishiya
Copy link
Owner

koekeishiya commented Mar 13, 2024

I guess we need a different workaround to this problem. See #1809, #1877, #1552, #2154 which are all solved by using HID-location instead of AnnotatedSession-location.

@koekeishiya
Copy link
Owner

Found a different fix.

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.

Resizing a window doesn't always update the layout
3 participants