-
Notifications
You must be signed in to change notification settings - Fork 186
don't modify topIndex in handleTextChanged if control has not the focus #1617
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
don't modify topIndex in handleTextChanged if control has not the focus #1617
Conversation
|
@mickaelistria would you prefer that we delete the complete if-block "if (!isFixedLineHeight() && topIndex > firstLine) {" or is the approach of this pull request also fine? |
|
We should try to delete the code that we do not know why it is there (eclipse-platform/eclipse.platform.ui#2142) early in the next release cycle to find if this causes any yet unknown issues. |
|
This PR looks good.
Here I think the reason for this code are clear enough: it can happen while editing some lines of non fixed height that the current position of the topIndex doesn't allow to see the whole first edited line. So I think it's best to keep it as proposed. |
c12914b to
9892ab9
Compare
|
test error |
That's extremely probable. Note that this test is important, it may not be written properly so maybe tweaking it so it is doing the job better can help, but in any case, the behavior it does test (notFixedLineHeightDoesntChangeLinePixelIfUnnecessary is hopefully explicit enough) is important to be guaranteed by a test. If this test is failing then it means that whenever a line header annotation is printed, the content of the text widget moves accordingly and may even hide (whose position is supposed to be stable for comfortable usage) |
9892ab9 to
985d3d8
Compare
985d3d8 to
f0fcbe5
Compare
|
mh... |
|
Can you reproduce the failure locally? |
looks like an issue in linux? central windows build is fine and test is locally also green on windows. I need to check the linux behavior here. |
f0fcbe5 to
7f7f26f
Compare
7f7f26f to
ee1381a
Compare
fix split screen issue eclipse-platform/eclipse.platform.ui#2142 by not setting StyledText#topIndex if the control has not the focus.