-
Notifications
You must be signed in to change notification settings - Fork 227
Revert "Workaround for unexpected scrollbars in MultipleHyperlink" #3498
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
base: master
Are you sure you want to change the base?
Conversation
The workaround was introduced to resolved the unexpected scrollbars in MultiHyperLinks but later it was fixed as a result of fix in eclipse-platform/eclipse.platform.swt#2324
ab9af83 to
8a90d49
Compare
HeikoKlare
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.
Can you please explain the relation to eclipse-platform/eclipse.platform.swt#2324 and how to test for regression against ist?
I just reverted that change and applied this PR, which should, according to the PR description, reintroduce the mentioned issue. However, everything looks fine for me (with 150% primary monitor and 225% secondary monitor on both of them).
|
Sorry for any confusion. Referring to my comment in vi-eclipse/Eclipse-Platform#355 The issue fixed by eclipse-platform/eclipse.platform.swt#2324 is a completely different one it addressed scrollbars appearing even when the workaround was applied. The issue that was originally solved by the workaround from #2998 appears to be fixed now due to 56379d5edf29a33b9b7cd4d639a921f2b2465820. Since pixelToPoint now rounds up instead of down so it adds one pixel. However, I think this issue should be kept in mind if we plan to modify rounding behavior in pixelToPoint |
|
@arunjose696 is right in this case. I tried reverting eclipse-platform/eclipse.platform.swt@56379d5 and at 225% I got these scroll bars again. So this commit what fixed this issue and workaround is not needed anymore.
|
|
The mentioned commit was basically just a partial revert of what was done 2 week before: eclipse-platform/eclipse.platform.swt@55b5adf So that one is not the original fix for the scrollbar issue. Without knowing which original commit properly fixed the scrollbar issue, I am a bit reluctant to revert the workaround, as we do not know how and whether it was properly fixed. |
|
I agree, In this case I am also not sure what commit really fixed it and why we don't need the workaround anymore. |

The workaround was introduced to resolved the unexpected scrollbars in MultiHyperLinks but later it was fixed as a result of fix in eclipse-platform/eclipse.platform.swt@56379d5
How to test
Use this Snippet and open it on a 225% monitor.
Hover over main and press control key.
Result
I have tested it with primary monitor = 150% and secondary = 125, 150, 175, 200, 225, 250, 300.