Skip to content
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

Merged

Conversation

tobiasmelcher
Copy link
Contributor

fix split screen issue eclipse-platform/eclipse.platform.ui#2142 by not setting StyledText#topIndex if the control has not the focus.

@tobiasmelcher
Copy link
Contributor Author

@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?

Copy link
Contributor

github-actions bot commented Nov 24, 2024

Test Results

   486 files  ±0     486 suites  ±0   10m 45s ⏱️ -18s
 4 306 tests ±0   4 293 ✅ ±0   13 💤 ±0  0 ❌ ±0 
16 408 runs  ±0  16 300 ✅ ±0  108 💤 ±0  0 ❌ ±0 

Results for commit ee1381a. ± Comparison against base commit 238bc0c.

♻️ This comment has been updated with latest results.

@vogella
Copy link
Contributor

vogella commented Nov 24, 2024

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.

@mickaelistria
Copy link
Contributor

This PR looks good.

We should try to delete the code that we do not know why it is there

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.

@BeckerWdf BeckerWdf added this to the 4.35 M1 milestone Nov 25, 2024
@akurtakov akurtakov force-pushed the styled_text_fix_split_screen branch from c12914b to 9892ab9 Compare December 9, 2024 15:24
@tobiasmelcher
Copy link
Contributor Author

test error
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_custom_StyledText.test_notFixedLineHeightDoesntChangeLinePixelIfUnnecessary
might be related to this change? I need to check.

@mickaelistria
Copy link
Contributor

might be related to this change?

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)

@BeckerWdf BeckerWdf force-pushed the styled_text_fix_split_screen branch from 9892ab9 to 985d3d8 Compare December 13, 2024 11:44
@tobiasmelcher tobiasmelcher force-pushed the styled_text_fix_split_screen branch from 985d3d8 to f0fcbe5 Compare December 14, 2024 19:59
@BeckerWdf
Copy link
Contributor

mh...
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_custom_StyledText.test_notFixedLineHeightDoesntChangeLinePixelIfUnnecessary
still fails.

@mickaelistria
Copy link
Contributor

Can you reproduce the failure locally?

@tobiasmelcher
Copy link
Contributor Author

mh... org.eclipse.swt.tests.junit.Test_org_eclipse_swt_custom_StyledText.test_notFixedLineHeightDoesntChangeLinePixelIfUnnecessary still fails.

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.

@tobiasmelcher tobiasmelcher force-pushed the styled_text_fix_split_screen branch from f0fcbe5 to 7f7f26f Compare December 20, 2024 23:00
@BeckerWdf BeckerWdf force-pushed the styled_text_fix_split_screen branch from 7f7f26f to ee1381a Compare December 23, 2024 08:43
@BeckerWdf BeckerWdf merged commit 49fd6b0 into eclipse-platform:master Dec 23, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants