-
Notifications
You must be signed in to change notification settings - Fork 187
[Win32] MultiZoomCoordinateSystemMapper: zero-sized rectangle handling #1928
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
[Win32] MultiZoomCoordinateSystemMapper: zero-sized rectangle handling #1928
Conversation
Test Results 539 files - 6 539 suites - 6 32m 41s ⏱️ + 3m 25s For more details on these failures, see this check. Results for commit 5003293. ± Comparison against base commit 7c818d6. This pull request removes 37 and adds 2 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
ef5d23a to
379393a
Compare
|
@amartya4256 can you please review this? |
| private Monitor getContainingMonitorForPoints(int x, int y, int width, int height) { | ||
| if (width <= 0 || height <= 0) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to return the monitor of the containing point (x,y)in this case using getContainingMonitorForPoints(int x, int y)
Since this function's caller is getValidMonitorIfApplicable(int x, int y, int width, int height, Monitor monitor) and it assigns the rectangle the defaultMonitor in case null is returned. For correctness, we can keep the monitor consistent by calling getContainingMonitorForPoints(int x, int y). What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea, thank you! I have adapted the PR accordingly. Can you check whether it now fits to your proposal, @amartya4256?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is good.
The MultiZoomCoordinateSystemMapper currently produces a devide-by-zero error when transforming a rectangle of zero size. This can, for example, happen when a (dummy) shell with width=height=0 is created and its bounds are passed to the mapper. This change ensures that the problematic calculation is avoided in case width or height are zeroi by just taking the x/y coordinate into account.
379393a to
5003293
Compare
amartya4256
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR overall looks good to me.
|
Failing test is unrelated and documented: #1843 |
The MultiZoomCoordinateSystemMapper currently produces a devide-by-zero error when transforming a rectangle of zero size. This can, for example, happen when a (dummy) shell with width=height=0 is created and its bounds are passed to the mapper.
This change ensures that the problematic calculation is avoided in case width or height are zero. In that case, no reasonable monitor can be identified anyway.