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

[23] Partition support for markdown comments #1542 #1549

Merged
merged 6 commits into from
Aug 27, 2024
Merged

Conversation

jarthana
Copy link
Member

What it does

Fixes #1542

How to test

Author checklist

@jarthana jarthana marked this pull request as draft July 26, 2024 07:35
@jarthana jarthana self-assigned this Jul 26, 2024
@jarthana jarthana changed the base branch from master to BETA_JAVA23 July 29, 2024 12:01
@jarthana jarthana added this to the BETA_JAVA23 milestone Jul 30, 2024
@jarthana
Copy link
Member Author

@ktatavarthi Can you please point me to the relevant test suits where I can find examples and add new tests?

@jarthana jarthana marked this pull request as ready for review August 2, 2024 08:46
@stephan-herrmann
Copy link
Contributor

It seems with pulling this change all markdown comments in the editor lose their coloring. Is this intended? Will you add new colors / preferences?

Or should be declare that javadoc == javadoc, independent of format? (At face value we're mainly replacing HTML with markdown, but it's still javadoc, right?).

Or should we use colors to remind users of which format they are seeing, so they won't accidentally mix both?

@jarthana
Copy link
Member Author

It seems with pulling this change all markdown comments in the editor lose their coloring. Is this intended? Will you add new colors / preferences?

Not intentional. I will work on this.

Or should be declare that javadoc == javadoc, independent of format? (At face value we're mainly replacing HTML with markdown, but it's still javadoc, right?).

Or should we use colors to remind users of which format they are seeing, so they won't accidentally mix both?

I would keep it simple and use the same color as Javadoc, at least for now.

@@ -57,4 +61,8 @@ public interface IJavaPartitions {
* @since 3.20
Copy link
Contributor

Choose a reason for hiding this comment

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

not related to this change: it looks like #1011 forgot to fix this version during move to another plugin. core.manipulation has no 3.20 to match this.

Comment on lines +468 to +470
ContentAssistProcessor markdownProcessor= new JavadocCompletionProcessor(getEditor(), assistant);
assistant.setContentAssistProcessor(markdownProcessor, IJavaPartitions.JAVA_MARKDOWN_COMMENT);

Copy link
Contributor

Choose a reason for hiding this comment

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

There's some inconsistency:

  • the processor is registered for markdown partition
  • the ctor of JavadocCompletionProcessor still associates it to the javadoc partition

When I tweak JavadocCompletionProcessor to pass the markdown partition into the super ctor, then no proposals will be shown in markdown any more.

IIUC, the reason is that plugin.xml does not associate any proposal computer with the markdown partition type.

Suggestion: add a parameter partition to the ctor of JavadocCompletionProcessor, then selectively add markdown partition in plugin.xml. This should give us control to enable desired processors while excluding, e.g., HTMLTagCompletionProposalComputer.

Copy link
Member Author

Choose a reason for hiding this comment

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

In order to boost my confidence in this area (and to get this code to a build), I will take this up also as part of my next iteration.

@stephan-herrmann
Copy link
Contributor

stephan-herrmann commented Aug 24, 2024

Unsorted observations & ideas while playing with this PR and stepping through code:

  • would it be interesting to define yet another partition type "codeblock within markdown"?
  • I like the auto insertion of "///", how hard would it be to let this add as many blanks as the least indented line so far, minimally 1 (per convention)?
  • folding completely hides the markdown region, whereas traditional javadoc will still show the first line
  • seeing a change in CodeTemplateSourceViewerConfiguration (related to editing comment templates) lets me think we might want a full new set of comment templates in the future ...
  • shift-hover over markdown link works, cool! ✔️

Concerning completion:

  • completion for @param works, both tag name and actual parameter name are proposed. ✔️
  • java templates are also proposed which is not useful
    • not a big issue, but at least priorities could be improved to show tag proposals above java templates
  • after @param also HTML tags are proposed, which is not helpful
  • I don't yet get the expected proposals after [
  • After [# it proposes to insert {@link #foo()}, which is not what I want :), I want [#foo()]
    works mostly, see next comment ✔️

In summary: I could not find any obvious bugs, so the PR can be seen as an incremental step forward. I don't think all of the above topics need to be dealt with while this is a preview feature.

  • Perhaps the last item (completion after [) would be highest on my list, but then this needs to be addressed in jdt.core, not ui.
    -> corner cases addressed in [23] Code assist changes for markdown Javadoc comments eclipse.jdt.core#2861
  • On the UI side, perhaps folding (showing one line when folded) would be my first wish.
  • For auto-insertion I'd suggest to just constantly insert one blank for now, to make it /// .

@stephan-herrmann
Copy link
Contributor

After [# it proposes to insert {@link #foo()}, which is not what I want :), I want [#foo()]

This actually worked already in many cases. Just some edge-conditions need slight improvement, see eclipse-jdt/eclipse.jdt.core#2861

@stephan-herrmann
Copy link
Contributor

@jarthana given that [] pairs are relevant for link completion I figured we should let smart-insert for brackets work in markdown partitions, too. The following seems to do the trick already:

diff --git a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/CompilationUnitEditor.java b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/CompilationUnitEditor.java
index bf6465b..716be39 100644
--- a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/CompilationUnitEditor.java
+++ b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/CompilationUnitEditor.java
@@ -532,7 +532,7 @@
 				}
 
 				ITypedRegion partition= TextUtilities.getPartition(document, IJavaPartitions.JAVA_PARTITIONING, offset, true);
-				if (!IDocument.DEFAULT_CONTENT_TYPE.equals(partition.getType()))
+				if (!IDocument.DEFAULT_CONTENT_TYPE.equals(partition.getType()) && !IJavaPartitions.JAVA_MARKDOWN_COMMENT.equals(partition.getType()))
 					return;
 				if (!validateEditorInputState())
 					return;

Do you want to adopt this in your current change?

@jarthana
Copy link
Member Author

* On the UI side, perhaps folding (showing one line when folded) would be my first wish.

The PR addresses this now.

@jarthana jarthana merged commit 351e805 into BETA_JAVA23 Aug 27, 2024
7 checks passed
@stephan-herrmann stephan-herrmann deleted the gh1542 branch August 27, 2024 13:53
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.

[23] Partition support for markdown comments
2 participants