Fix Caret Rescaling for win32 #62 #1847
Conversation
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Caret.java
Show resolved
Hide resolved
|
I would like to have one more win32 developer to test and verify before giving approval. @HannesWell @HeikoKlare or someone else would you please test/review. |
|
@eclipse-platform/eclipse-platform-project-leads If some of you can review please step in as we are pretty close to RC1. |
I tried to test it, but I didn't find sufficient instructions to reproduce the issue. I tried out all examples in SWT that reference the Caret class but none of them hit the break-point at the location that is changed. |
|
I added the information on how to reproduce the error. |
I'm sorry but I'm still not able to reproduce it, not even to trigger the code in question. |
|
@HannesWell The issue description seems to miss the information that it is related to monitor-specific scaling only. The code in question is only executed when that feature is enabled. To test in a runtime Eclipse, it's not sufficient to enable the preference (as it is stored in the configuration area which is usually cleared on every restart), so it's usually best so set the according VM argument The above mentioned also means that the change will only have an effect when the (experimental) monitor-specific scaling preference is enabled, which makes the change rather safe for being that late in the development cycle. |
There was a problem hiding this comment.
@HannesWell The issue description seems to miss the information that it is related to monitor-specific scaling only. The code in question is only executed when that feature is enabled.
That was indeed the missing part. Now I was able to reproduce it and can confirm that this fixes the issue. Thanks for this.
The above mentioned also means that the change will only have an effect when the (experimental) monitor-specific scaling preference is enabled, which makes the change rather safe for being that late in the development cycle.
Agree. But I'd leave the final decision to the platform PLs. :)
There was a problem hiding this comment.
@amartya4256 I agree that this change is reasonable. However, part of the issue addressed here seems to also be that StyledText::updateAndRefreshCarets calls the update functionality on the same caret multiple times. Isn't that somehow unexpected and unintended as well? I.e., shouldn't StyledText::updateAndRefreshCarets probably first collect all carets to update (removing duplicated) and then update them instead of calling the update on the same caret multiple times?
But that could be done in a follow-up PR for cleanup. This PR seems to be fine as is to fix the underlying issue.
@HeikoKlare approved? @eclipse-platform/eclipse-platform-project-leads can this still be merged (now for RC2)? It's been approved by 2 (soon maybe even 3) committers and it's been deemed safe to merge. |
|
I think this should be merged but it would be nice if at least one project lead agrees. |
|
I personally would not merge non trivial SWT changes in RC2. This change is not a trivial one. |
I agree that non-trivial changes should not be merged for RC2, but I would like to emphasize that this change has basically no risk (and may thus be considered rather "trivial"). The adapted code is only used for monitor-specific scaling, which is an experimental feature on Windows that is not enabled by default. An experimental feature might not work perfectly fine anway (which is why it's flagged "experimental"), but improving it as far as possible for a release (such as with this change) improves the chance that people keep testing it and report back issues so that once delivering that feature as non-experimental and enabled-by-default, it will be in a better state. |
I haven't reviewed the code in debugger but I trust your assessment. So no objections from my side then. |
|
Thank you! FWIW, here is a screenshot showing the single call of that method (done by the DPI change handler only used for monitor-specific scaling on Windows): So with no objections from platform PL side, I consider your comment as PMC+1, @merks? |
This commit fixes caret rescaling by making sure the the field height is updated for every caret and Caret::resize is called for those carets where applicable. contributes to eclipse-platform#62 and eclipse-platform#127
HeikoKlare
force-pushed
the
amartya4256/fix_caret_rescaling
branch
from
1727589 to
abd904b
Compare
|
I too trust @HeikoKlare's assessments and give this a PMC +1 when he deems this ready to be merged. |
|
Thank you all! |
This PR fixes caret rescaling by making sure the the field height is updated for every caret and Caret::resize is called for those carets where applicable.
contributes to
#62 and #127
To reproduce
This is my configuration and how I reproduce the error in my current Eclipse IDE.
General > Appearance) or by using the flag-Dswt.autoScale.updateOnRuntime=trueIn this video you can see how I moved from the monitor on the right to the one on the left and then back. The caret is broken after I move to the monitor on the left and stays broken when I come back to the monitor on the right.