Skip to content

Fix rectangle/point distance computation.#2020

Merged
nikitabobko merged 1 commit intonikitabobko:mainfrom
rickyz:rect_bounds
Mar 23, 2026
Merged

Fix rectangle/point distance computation.#2020
nikitabobko merged 1 commit intonikitabobko:mainfrom
rickyz:rect_bounds

Conversation

@rickyz
Copy link
Copy Markdown
Collaborator

@rickyz rickyz commented Mar 23, 2026

Previously, the implementation would treat the point (0, w) as having 0
distance to the rectangle defined by (0, 0), (w, h). The distance should
be 1, since the rectangle only contains the points (x, y) for x in [0,
w) and y in [0, h).

This could lead to screen/workspace assignments being computed
incorrectly for adjacent monitors. For example, for two monitors:

monitor 1: (0, 0) .. (100, 100)
monitor 2: (100, 0) .. (200, 100)

The point (100, 0) would be considered as having distance 0 to both
monitors, which could lead to monitorApproximation for that point
incorrectly returning monitor 1 if it appears first in the monitors
list.

Fix this by subtracting 1 from maxX/maxY when determining the
bottom/right bounds of a rectangle.

PR checklist

  • Explain your changes in the relevant commit messages rather than in the PR description. The PR description must not contain more information than the commit messages (except for images and other media).
  • Each commit must explain what/why/how and motivation in its description. https://cbea.ms/git-commit/
  • Don't forget to link the appropriate issues/discussions in commit messages (if applicable).
  • Each commit must be an atomic change (a PR may contain several commits). Don't introduce new functional changes together with refactorings in the same commit.
  • ./run-tests.sh exits with non-zero exit code.
  • Avoid merge commits, always rebase and force push.

Failure to follow the checklist with no apparent reasons will result in silent PR rejection.

Previously, the implementation would treat the point (0, w) as having 0
distance to the rectangle defined by (0, 0), (w, h). The distance should
be 1, since the rectangle only contains the points (x, y) for x in [0,
w) and y in [0, h).

This could lead to screen/workspace assignments being computed
incorrectly for adjacent monitors. For example, for two monitors:

monitor 1: (0, 0) .. (100, 100)
monitor 2: (100, 0) .. (200, 100)

The point (100, 0) would be considered as having distance 0 to both
monitors, which could lead to `monitorApproximation` for that point
incorrectly returning monitor 1 if it appears first in the monitors
list.

Fix this by subtracting 1 from maxX/maxY when determining the
bottom/right bounds of a rectangle.
@nikitabobko
Copy link
Copy Markdown
Owner

The patch makes sense, thanks! I am curious: how did you find this bug? Did you find it by just browsing the source code? Or do you have a real world scenario?

@rickyz
Copy link
Copy Markdown
Collaborator Author

rickyz commented Mar 23, 2026

I ran into this issue in my own usage after rebuilding from HEAD. I don't understand why this wasn't biting me before though - I was able to bisect the monitor mixup to 204f4aa, but I don't see how that change should have any effect on monitorApproximation.

@nikitabobko nikitabobko merged commit 6021c13 into nikitabobko:main Mar 23, 2026
5 checks passed
@nikitabobko
Copy link
Copy Markdown
Owner

nikitabobko commented Mar 23, 2026

I don't see how the mentioned commit could cause this bug either. Anyway, merged.

Given the positive interaction history, I gave you the write access to the repo, and allowed you to create issues: 6f37ad0

I am a big fan of the post-push review process, where commits are reviewed after being pushed to the main branch. This may sound wild as it goes against common industry practice, but I saw how it worked beautifully in practice on the IntelliJ codebase. This workflow puts more trust in people and reduces review friction. That said, I still review all commits on the main branch and comment directly on them if I have questions.

The bare minimum is to keep git commit message history elaborate (which you’re already doing great) and to keep the CI green (Mostly, run-tests.sh script) . Worst case, any commit can be reverted - not a big deal.

Feel free to push directly to the main if you consider your change straightforward. You can still use PRs for changes you’re unsure about, or simply if that’s your preferred workflow.

Thanks!

@nikitabobko nikitabobko added the pr-merged Pull Request is merged label Mar 23, 2026
@rickyz
Copy link
Copy Markdown
Collaborator Author

rickyz commented Mar 24, 2026

Much appreciated, I do happen to have two small fixes lined up that I'll push - will keep an eye out for any comments, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-merged Pull Request is merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants