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 offsetting with focus stealing prevention #3724

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

Conversation

tarek-y-ismail
Copy link
Contributor

@tarek-y-ismail tarek-y-ismail commented Jan 21, 2025

Closes #3695

Adds occlusion checking when placing down new windows. The checks continue with an increasingly larger radius until no positions around the window fit in the active output or no possible window rectangle overlaps with the active output. The checks are also capped to 16 "iterations" so they don't go on forever, just in case.

Here's how it looks like now:

Screencast.from.2025-01-21.17-58-52.mp4

How it works:

  • We repeatedly subtract the rectangles of existing windows from our candidate window rectangle. If after iterating through all windows (or the current window, as an early exit) we still have any visible area of the rectangle, then that placement is used.
  • We further filter placements by checking if at least a 50x50 chunk of the app is visible to prevent thin lines that could be hard to notice

@tarek-y-ismail tarek-y-ismail requested a review from a team as a code owner January 21, 2025 16:10
@tarek-y-ismail tarek-y-ismail self-assigned this Jan 22, 2025
@tarek-y-ismail tarek-y-ismail force-pushed the MIRENG-954/fix-window-offsetting-with-focus-stealing-prevention branch from 584bbc4 to 3dd2a30 Compare January 24, 2025 17:49
@tarek-y-ismail
Copy link
Contributor Author

There's a bug where a 1280x1024 fullscreen window is returned from for_each_window_in_workspace. It's not clear to me what this window is. My guess is either the background or something leftover from the spinner. Will investigate next week.

@AlanGriffiths
Copy link
Collaborator

There's a bug where a 1280x1024 fullscreen window is returned from for_each_window_in_workspace. It's not clear to me what this window is. My guess is either the background or something leftover from the spinner. Will investigate next week.

Check which layer it is in. You're likely seeing the background

Copy link
Collaborator

@Saviq Saviq left a comment

Choose a reason for hiding this comment

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

This looks like a piece of code that could use and would yield nicely to some unit testing?

Comment on lines +484 to +485
if(focus_stealing == FocusStealing::prevent)
try_place_new_window_and_account_for_occlusion(parameters);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Focus stealing prevention shouldn't be a factor when placing new windows, why wouldn't you just use the same in both cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like a piece of code that could use and would yield nicely to some unit testing?

Indeed, just wanted your opinions on the base idea before investing more time into it.

Focus stealing prevention shouldn't be a factor when placing new windows, why wouldn't you just use the same in both cases?

Well. For one: thou shalt not change established behavior. But in all seriousness, this much work isn't really needed when focus stealing prevention is off, you just place the window somewhere according to the old logic and it's going to be on top anyway. If you deem it suitable enough for FSP=off, then I'm okay with that

@tarek-y-ismail tarek-y-ismail requested a review from Saviq January 28, 2025 15:00
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.

Window offsetting shouldn't assume new windows are automatically focused
3 participants