Skip to content

Conversation

@tobiasmelcher
Copy link
Contributor

This change allows code mining implementors to react on mouse move events and call setCursor on the text widget for example.

We would like to implement a similar feature like shown in the following screenshot based on the eclipse code minings.
vscode_links
The code mining text color can already be set by overriding the draw method of LineHeaderCodeMining. The mouse cursor handle currently cannot be currently specified by the implementor; it is hard coded in AbstractInlinedAnnotation. This pull request introduces the methods getMouseHover, getMouseOut and getMouseMove in the ICodeMining interface so that the implementor can influence what mouse cursor is shown when hovering and moving over the mining. This API is very low level and technical, but it gives the implementor all possibilities to provide its own link-like controls in the code minings.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 31, 2025

Test Results

 1 821 files  ±0   1 821 suites  ±0   1h 29m 53s ⏱️ - 5m 51s
 7 719 tests ±0   7 491 ✅ +1  228 💤 ±0  0 ❌  - 1 
24 318 runs  ±0  23 569 ✅ +1  749 💤 ±0  0 ❌  - 1 

Results for commit 24173ef. ± Comparison against base commit 5039550.

♻️ This comment has been updated with latest results.

@BeckerWdf BeckerWdf added this to the 4.35 M3 milestone Feb 3, 2025
@tobiasmelcher tobiasmelcher force-pushed the code_mining_mouse_move branch 2 times, most recently from 85c70a2 to 5c95b3f Compare February 3, 2025 09:23
@BeckerWdf
Copy link
Member

BeckerWdf commented Feb 3, 2025

we have API Errors here.

As far as I see the version was incremented to 3.27 with 816a056

So it should be sufficient to add since tags with 3.27 to the new methods.
This was done with https://github.com/eclipse-platform/eclipse.platform.ui/compare/1213c88d1ce65113923fe104a9a9206c54023f96..85c70a26b6762e451edf75878ab0a8d42b02f640
but the build was complaining.

Incrementing to 3.28 now complains that the since tag on SurroundWithBracketsStrategy is wrong.

What's correct here?
Can somebody pls. help?

@BeckerWdf
Copy link
Member

BeckerWdf commented Feb 3, 2025

Do API tools have issues with "Default" methods?

@tobiasmelcher tobiasmelcher force-pushed the code_mining_mouse_move branch from 5c95b3f to cf9fa42 Compare February 3, 2025 14:31
@tobiasmelcher tobiasmelcher force-pushed the code_mining_mouse_move branch from cf9fa42 to e73bcf6 Compare February 3, 2025 14:54
@BeckerWdf
Copy link
Member

@mickaelistria: What do you think about this change? Is it fine from your point of view?

/**
* @since 3.27
*/
public interface ICodeMiningExtension {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a new interface, you can add the new methods as default returning null to the existing interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this with [1213c88] but then got errors from the API checks. Looks like that adding a default method implementation to an existing interface makes troubles with existing interface implementations, see https://github.com/eclipse-platform/eclipse.platform/blob/master/docs/Evolving-Java-based-APIs-2.md (Adding a default method will break an existing client type if it already implements another interface that declares a default method with a matching signature)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, this would require API exception. But if we evaluate that the risk of method with colliding signature is about 0, then it's fine to add default interface methods and to add API tools exceptions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how can an "API tools exception" be added?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for default implementation. Exception can be added via quick fix

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so does the current version of the PR look good to you @mickaelistria and @vogella regarding the "default" methods and the API tools exception?

Copy link
Contributor

@mickaelistria mickaelistria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no strong objection, just a suggestion inline.
I'm also wondering whether the API is actually on the best grain: here it looks like from the example that a link SelectionEvent could be a better fit. Thinking much further, it looks like the trend with code mining is to allow any rendering in here. Currently, it's just Text (drawn with native bits), but maybe it should become a free SWT Canvas to allow full widget power.

But again, no objection if you want to merge that now.

This change allows code mining implementors to react on mouse move
events and call setCursor on the text widget for example.
@tobiasmelcher tobiasmelcher force-pushed the code_mining_mouse_move branch from e73bcf6 to 24173ef Compare February 4, 2025 09:06
@BeckerWdf
Copy link
Member

BeckerWdf commented Feb 4, 2025

Currently, it's just Text (drawn with native bits)

One can also draw icons with it. We do that in our product "ABAP Development Tools" e.g. as a means to execute unit tests:
image

I find the current API quite powerful. Adding further convenience API on top (e.g. like rendering something like "Styled Text" or "Text with links" should be handled separately.

@tobiasmelcher tobiasmelcher merged commit 9f3733a into master Feb 4, 2025
17 checks passed
@BeckerWdf BeckerWdf deleted the code_mining_mouse_move branch February 5, 2025 06:31
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.

6 participants