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

simplify out-of-bounds check #3091

Merged
merged 4 commits into from
Oct 20, 2023
Merged

Conversation

overheadhunter
Copy link
Member

This is a follow-up PR to #3017.

Due to major communication deficiencies (my bad), here is what I intended to mean in #3017 (comment):

The original implementation was great due to its simplicity and absence of fp math. It was easy to understand and thus to maintain. I requested a little change due to an edge case where a single pixel of each border was deemed sufficient to keep the window at its position. I suggested to fix this by simply insetting the borders by a little amount.

Here is a visualization, where the four borders (blue, red, pink, orange) are insetted. The top border (blue) a little less than the others, as the top bar of the window is more important to grab it.

Notice how no repositioning is triggered if the window is just slightly out of bounds (second case).

OutOfBounds

Explaining this took way longer than adding the four numbers to the existing code. I am very sorry about not being able to transport the message correctly from the beginning.

What is still open for debate is the amount of insetting. @Rexbas suggested in #3017 (comment) to use a variable amount, depending on the window dimensions. E.g. insetting the red, orange and pink border by 50% and only keep the blue border fixed.

as suggested in #3017 (comment) with minor adjustments
@Rexbas
Copy link
Contributor

Rexbas commented Aug 29, 2023

The original implementation was great due to its simplicity and absence of fp math.

It is actually not so simple considering the implicit assumptions we make about the user's display configuration. This "simple" version is actually not working for the same edge case you described. That is why I changed the entire implementation to sum all pixels and compare it to the window size. See the example below, which is the same as your last example above but with the window more towards the corner. This window wont't be reset even though it is basically 90% out of bounds. This happens because all rows and columns evaluate to false in isRectangleOutOfScreen(...).

image

@infeo and I agreed that a window is allowed to be up to 10px (or whatever the amount should be) out of bounds but no more (#3017 (comment)). This edge case clearly does not obey that rule.

Another better solution, which I tested last week, is to only check the corners of the insetted window. Then this edge case would work because the bottom right corner is out of bounds. However, then there are other edge cases that won't work. The problem is that we only check a subset of the pixels of the window and make assumptions about the other parts of the window, making such a solution error prone. The current implementation covers all pixels and I can say with a high degree of certainty that it will work regardless of the user's display configuration.

So I recommend to not use the "simple" version and stick with the current pixel counting version.

to use a variable amount, depending on the window dimensions.

@infeo requested to use a fixed pixel amount, but we could also change it to 50% of the default size, which is also a fixed value.

@overheadhunter
Copy link
Member Author

overheadhunter commented Aug 29, 2023

This window wont't be reset even though it is basically 90% out of bounds.

Which is totally fine, since you have still sufficient space to drag it out. The goal is to make sure there is a "large enough" area, nothing more.

@overheadhunter overheadhunter merged commit 5c5777f into develop Oct 20, 2023
7 checks passed
@overheadhunter overheadhunter deleted the feature/simplify-out-of-bounds branch October 20, 2023 09:56
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.

None yet

2 participants