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

fix LineHeaderCodeMining redraw logic on empty lines #2133

Conversation

tobiasmelcher
Copy link
Contributor

LineHeaderCodeMinings on empty lines were not drawn after calling ISourceViewerExtension5#updateCodeMinings.
Reason: InlinedAnnotationDrawingStrategy#draw triggered call StyledText#redrawRange with length = 0 which is ignored by SWT and no redraw event was triggered

CodeMiningPdeTest is documenting the problem scenario. It opens a plain text editor and the registered provider org.eclipse.jface.text.tests.codemining.CodeMiningTestProvider wants to contribute code minings.
The StaticContentLineCodeMining at line 1, column 1 with label 'mining' will be immediately rendered when the text editor is opened. It is used as indication in the unit test to verify that the text editor is opened and the code minings are all loaded and rendered properly.

pde_test_1

Afterwards the global variable CodeMiningTestProvider.provideLineHeaderMining= true; is set which lets the provider also return a LineHeaderCodeMining at the second line. The second line has no text and is empty (there is only one \n character).
The unit test is then calling((ISourceViewerExtension5) viewer).updateCodeMinings(); with the goal to make the additional LineHeaderCodeMining visible.

pde_test_2

This was not working before and this pull request will fix the scenario.

I hope you can follow the explanation and understand how this pull request wants to fix this scenario.
The redrawRange call in org.eclipse.jface.text.source.inlined.InlinedAnnotationDrawingStrategy did not trigger a repaint event when length has value 0. The pull request sets the length value to the line delimiter length (value 1 in in the unit test)

textWidget.redrawRange(offset, length, true);

One question to the PDE test. Is it ok to add an additional PDE test in org.eclipse.jface.text.tests? I needed to add a dependency to the plugins org.eclipse.core.resources and org.eclipse.ui.ide so that the text editor could be opened in the unit test. Is that ok?

Copy link
Contributor

github-actions bot commented Jul 26, 2024

Test Results

 1 209 files   - 1   1 209 suites   - 1   1h 4m 8s ⏱️ + 9m 23s
 7 686 tests +2   7 455 ✅  -  1  231 💤 + 3  0 ❌ ±0 
16 143 runs  +1  15 630 ✅  - 43  513 💤 +44  0 ❌ ±0 

Results for commit c9a1c2d. ± Comparison against base commit 50061d4.

This pull request skips 3 tests.
UiTestSuite org.eclipse.ui.tests.api.ApiTestSuite org.eclipse.ui.tests.api.WorkbenchPluginTest ‑ testGetImageRegistryFromAdditionalDisplay
org.eclipse.jface.text.tests.contentassist.ContextInformationTest ‑ testContextInfo_hide_focusOut
org.eclipse.urischeme.internal.registration.TestUnitWinRegistry ‑ testWinRegistry

♻️ This comment has been updated with latest results.

@BeckerWdf
Copy link
Contributor

One question to the PDE test. Is it ok to add an additional PDE test in org.eclipse.jface.text.tests? I needed to add a dependency to the plugins org.eclipse.core.resources and org.eclipse.ui.ide so that the text editor could be opened in the unit test. Is that ok?

Can somebody answer this question?

@BeckerWdf
Copy link
Contributor

The jface.text (none-test)-bundle does not have any dependencies to org.eclipse.ide. Only to org.eclipse.swt, org.eclipse.jface and org.eclipse.core.runtime. And org.eclipse.swt also only has basic dependencies (to swt and equinox).
So I thnik that jface.text test-bundles should not get the org.eclipse.ide dependcies.
Maybe we should move the new test to org.eclipse.ui.editors.tests.

@merks, @mickaelistria What do you think?

@merks
Copy link
Contributor

merks commented Jul 29, 2024

Yes, better to move/have the test somewhere that already has the necessary dependencies.

@tobiasmelcher tobiasmelcher force-pushed the refreshLineHeaderMiningOnEmptyLine branch 2 times, most recently from 111abe8 to a6fc392 Compare July 30, 2024 15:19
@tobiasmelcher
Copy link
Contributor Author

Yes, better to move/have the test somewhere that already has the necessary dependencies.

Thanks a lot for the clarification. I moved the test to org.eclipse.ui.editors.tests.

@BeckerWdf BeckerWdf force-pushed the refreshLineHeaderMiningOnEmptyLine branch from a6fc392 to ff5ea3f Compare July 31, 2024 06:44
@BeckerWdf
Copy link
Contributor

build is failing with

No test report files were found. Configuration error?
Error when executing always post condition:
Also:   hudson.remoting.Channel$CallSiteStackTrace: Remote call to JNLP4-connect connection from 10.40.67.54/10.40.67.54:57290
		at hudson.remoting.Channel.attachCallSiteStackTrace(Channel.java:1787)
		at hudson.remoting.UserRequest$ExceptionResponse.retrieve(UserRequest.java:356)
		at hudson.remoting.Channel.call(Channel.java:1003)
		at hudson.FilePath.act(FilePath.java:1226)
		at hudson.FilePath.act(FilePath.java:1215)
		at hudson.tasks.junit.JUnitParser.parseResult(JUnitParser.java:125)
		at hudson.tasks.junit.JUnitResultArchiver.parse(JUnitResultArchiver.java:160)
		at hudson.tasks.junit.JUnitResultArchiver.parseAndSummarize(JUnitResultArchiver.java:254)
		at hudson.tasks.junit.pipeline.JUnitResultsStepExecution.run(JUnitResultsStepExecution.java:63)
		at hudson.tasks.junit.pipeline.JUnitResultsStepExecution.run(JUnitResultsStepExecution.java:29)
		at org.jenkinsci.plugins.workflow.steps.SynchronousNonBlockingStepExecution.lambda$start$0(SynchronousNonBlockingStepExecution.java:47)
		at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539)
		at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
		at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
		at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
		at java.base/java.lang.Thread.run(Thread.java:857)

I don't understand this. Is this an infrastructure issue?

@BeckerWdf BeckerWdf force-pushed the refreshLineHeaderMiningOnEmptyLine branch from ff5ea3f to 70ca125 Compare July 31, 2024 13:14
@BeckerWdf
Copy link
Contributor

One of the build errors is due to eclipse-pde/eclipse.pde#1310.

@BeckerWdf
Copy link
Contributor

@tobiasmelcher: Looks like your changes broke org.eclipse.ui.editors.tests.SegmentedModeTest

@tobiasmelcher tobiasmelcher force-pushed the refreshLineHeaderMiningOnEmptyLine branch from 70ca125 to 1195450 Compare August 5, 2024 08:54
@tobiasmelcher
Copy link
Contributor Author

[1195450](/eclipse-platform/eclipse.platform.ui/pull/2133/commits/1195450a06cf1a8b26f563919ca9f0b960c072ac)

thanks a lot for pointing me to this test error. Should be now fixed with 1195450

@BeckerWdf
Copy link
Contributor

@HeikoKlare: Is this good to go? I am still not able to understand why "pr-head" fails

@BeckerWdf BeckerWdf added this to the 4.33 M3 milestone Aug 5, 2024
@HeikoKlare
Copy link
Contributor

The log shows:

Failed to execute goal org.eclipse.tycho.extras:tycho-p2-extras-plugin:4.0.8:compare-version-with-baselines (compare-attached-artifacts-with-release) on project org.eclipse.e4.ui.widgets: Baseline and reactor have the same fully qualified version, but different content

I guess this is because the PR branch is not rebased on current master and org.eclipse.e4.ui.widgets has changed and increased version by a recently merged PR on which this PR has not been rebased. So rebasing this PR branch on master should resolve that build failure.

CodeMinings on empty lines and at end of line were not drawn after
calling ISourceViewerExtension5#updateCodeMinings.
Reason: InlinedAnnotationDrawingStrategy#draw triggered call
StyledText#redrawRange with length = 0 which is ignored by SWT and no
redraw event was triggered
@tobiasmelcher tobiasmelcher force-pushed the refreshLineHeaderMiningOnEmptyLine branch from 1195450 to c9a1c2d Compare August 5, 2024 11:06
@BeckerWdf
Copy link
Contributor

I think this is good to be merged.

@BeckerWdf BeckerWdf merged commit 158c71a into eclipse-platform:master Aug 5, 2024
14 of 16 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