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

introduce FinalLineCodeMining to draw minings at the end of the document #2785

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tobiasmelcher
Copy link
Contributor

It is currently not possible to render a Line Code Mining at the very end of the document when the last character is not a line break. This pull request introduces the class FinalLineCodeMining which allows to achieve this requirement.

Copy link
Contributor

github-actions bot commented Feb 10, 2025

Test Results

 1 818 files  ±0   1 818 suites  ±0   1h 30m 22s ⏱️ + 1m 42s
 7 723 tests +2   7 495 ✅ +2  228 💤 ±0  0 ❌ ±0 
24 330 runs  +6  23 581 ✅ +6  749 💤 ±0  0 ❌ ±0 

Results for commit 7ff665e. ± Comparison against base commit 328646d.

♻️ This comment has been updated with latest results.

*
* @since 3.27
*/
public class DocumentFooterCodeMining extends LineHeaderCodeMining {
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it confusing that DocumentFooterCodeMining is actually a LineHeaderCodeMining. Would it be possible/good to remove this relationship?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you please review change 3a5f096 ? I updated the class hierarchy.

Copy link
Member

Choose a reason for hiding this comment

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

Can you please rebase? There are merge conflicts

@tobiasmelcher tobiasmelcher force-pushed the final_line_code_mining branch 2 times, most recently from 3a5f096 to 0632c5d Compare February 12, 2025 19:00
Copy link
Contributor

@BeckerWdf BeckerWdf left a comment

Choose a reason for hiding this comment

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

so we now have two new classes that are part of the API? Is this by intention?

@@ -76,23 +72,4 @@ public LineHeaderCodeMining(Position position, ICodeMiningProvider provider, Con
throws BadLocationException {
super(position, provider, action);
}

@Override
public Point draw(GC gc, StyledText textWidget, Color color, int x, int y) {
Copy link
Contributor

@BeckerWdf BeckerWdf Feb 13, 2025

Choose a reason for hiding this comment

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

I assume this method (with the exact same signature is now inherited from the new super-class?
Is this a compatible chang?

@BeckerWdf
Copy link
Contributor

there are errors in the build that seem to be unrelated to this change:

org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.eclipse.tycho:tycho-apitools-plugin:4.0.11:verify (verify) on project org.eclipse.e4.ui.ide: Execution verify of goal org.eclipse.tycho:tycho-apitools-plugin:4.0.11:verify failed: A required class was missing while executing org.eclipse.tycho:tycho-apitools-plugin:4.0.11:verify: org/eclipse/pde/api/tools/internal/provisional/IApiFilterStore

What's the issue here? Does anybody know?

@iloveeclipse
Copy link
Member

What's the issue here? Does anybody know?

See eclipse-platform/eclipse.platform.releng.aggregator#2815

Should be hopefully fixed with the SDK built that should be there in few minutes

/**
* @since 3.27
*/
public class MultilineCodeMining extends AbstractCodeMining {
Copy link
Contributor

Choose a reason for hiding this comment

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

This class seems to have very little logic in it. Instead of a new class in the hierarchy, couldn't it be a package-visible utility method?

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 thought that you want me to correct the class hierarchy. And this is what I did with the latest change. Looks like that I misunderstood you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, a simpler class hierarchy was what I had in mind. But I'm afraid introducing new intermediary APIs doesn't make things simpler, both from consumer and maintainer POV. I would instead suggest that we try to have DocumentFooterCodeMining extends AbstractCodeMining.

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 have adapted the class hierarchy with 9ec8815 as you requested. Could you please check again? Thanks.

@iloveeclipse
Copy link
Member

Should be hopefully fixed with the SDK built that should be there in few minutes

I've retriggered build. If it fails now, will be due something different issue.

@BeckerWdf BeckerWdf force-pushed the final_line_code_mining branch from 0632c5d to eb071a9 Compare February 13, 2025 15:35
@BeckerWdf
Copy link
Contributor

@mickaelistria: Can you pls. have a look at the new state?

import org.eclipse.jface.text.source.ISourceViewer;

/**
* @since 3.27
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add some documentation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

javadoc added with 7ff665e

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