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] DOM changes for markdown Javadoc comments #2738 #2807

Merged
merged 2 commits into from
Aug 14, 2024
Merged

Conversation

jarthana
Copy link
Member

@jarthana jarthana commented Aug 9, 2024

What it does

Partially fixes #2738

How to test

Author checklist

@jarthana jarthana changed the base branch from master to BETA_JAVA23 August 9, 2024 04:55
@jarthana
Copy link
Member Author

jarthana commented Aug 9, 2024

@stephan-herrmann , there's a long way to go for this PR. Plenty of new tests still failing. I already see that the flatteners are going to need some changes to the existing DOM Javadoc nodes to differentiate markdown comments. I would like you to try this out and let me know what you would need in terms of the DOM changes for rendering.

Copy link
Contributor

github-actions bot commented Aug 9, 2024

Test Results

    11 files  ±0      11 suites  ±0   19m 23s ⏱️ -41s
77 969 tests ±0  77 969 ✅ ±0  0 💤 ±0  0 ❌ ±0 
78 029 runs  ±0  78 029 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 02d70fc. ± Comparison against base commit 707447a.

♻️ This comment has been updated with latest results.

@stephan-herrmann
Copy link
Contributor

@stephan-herrmann , there's a long way to go for this PR. Plenty of new tests still failing. I already see that the flatteners are going to need some changes to the existing DOM Javadoc nodes to differentiate markdown comments. I would like you to try this out and let me know what you would need in terms of the DOM changes for rendering.

Thanks @jarthana. I do get some markdown Javadoc DOM nodes, great! As soon as links are involved, some details don't yet look right, but I need to take a closer look to find out if that is caused in DOM or during rendering. I'll let you know.

Copy link
Contributor

@stephan-herrmann stephan-herrmann left a comment

Choose a reason for hiding this comment

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

@jarthana once you feel somewhat comfortable with existing changes, despite any incompleteness merging this change would let me push some real changes in JDT/UI. That would establish a basis for investigating into details and corner cases.

As mentioned before a new API Javadoc.isMarkdown() or Javadoc.getCommentStyle() would allow for a cleaner check, but currently the workaround to test the raw javadoc for leading /// is functional.

@stephan-herrmann
Copy link
Contributor

@jarthana the following two changes help a great deal to pass correct IDocElements into the renderer:

diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/parser/AbstractCommentParser.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/parser/AbstractCommentParser.java
index 70a929a..bcba898 100644
--- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/parser/AbstractCommentParser.java
+++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/parser/AbstractCommentParser.java
@@ -504,7 +504,7 @@
 				}
 				refreshInlineTagPosition(textEndPosition);
 				setInlineTagStarted(false);
-			} else if (this.lineStarted && this.textStart != -1 && this.textStart <= textEndPosition && (this.textStart < this.starPosition || this.starPosition == lastStarPosition)) {
+			} else if (this.lineStarted && this.textStart != -1 && this.textStart <= textEndPosition && (this.textStart < this.starPosition || this.starPosition == lastStarPosition || this.markdown)) {
 				pushText(this.textStart, textEndPosition);
 			}
 			updateDocComment();
diff --git a/org.eclipse.jdt.core/dom/org/eclipse/jdt/core/dom/DocCommentParser.java b/org.eclipse.jdt.core/dom/org/eclipse/jdt/core/dom/DocCommentParser.java
index c125d44..bdd0b86 100644
--- a/org.eclipse.jdt.core/dom/org/eclipse/jdt/core/dom/DocCommentParser.java
+++ b/org.eclipse.jdt.core/dom/org/eclipse/jdt/core/dom/DocCommentParser.java
@@ -761,7 +761,7 @@
 						// Both tag elements must get the same source range.
 						TagElement previousTag = (TagElement) this.astStack[this.astPtr];
 						int parentStart = previousTag.getStartPosition();
-						int length = this.index - previousPosition + 1;
+						int length = this.index - previousPosition ;
 						previousTag.setSourceRange(parentStart, this.index - parentStart + 1);
 						List fragments = previousTag.fragments();
 						int size = fragments.size();
@@ -774,7 +774,7 @@
 							// If last fragment is a tag, then use it as previous tag
 							ASTNode lastFragment = (ASTNode) fragments.get(size-1);
 							if (lastFragment.getNodeType() == ASTNode.TAG_ELEMENT) {
-								//lastFragment.setSourceRange(lastFragment.getStartPosition(), length);
+								lastFragment.setSourceRange(lastFragment.getStartPosition(), length);
 								previousTag = (TagElement) lastFragment;
 							}
 						}
  • DocCommentParser: the same change discussed above, which avoids showing the closing ] of a link
  • AbstractCommentParser: this change fixes a bug whereby the last text element of a doc comment was dropped.

I would love to see these merged as part of this PR, whereas other findings can be conveniently handled separately, notably:

@jarthana
Copy link
Member Author

In the interest of getting ourselves unblocked on other issues, I have removed the ASTConverterMarkdownTest from the convert test suite and planning to release this today. The ASTConverterMarkdownTest takes after ASTConverterJavadocTest, which has another ad-hoc parser/scanner that parses the Javadoc and compares with the DOM output. I am yet to completely have that under control. Will take that up immediately after this.

@jarthana jarthana merged commit 16e2f4d into BETA_JAVA23 Aug 14, 2024
4 checks passed
@jarthana jarthana deleted the gh2738 branch August 14, 2024 05:59
@stephan-herrmann stephan-herrmann added this to the BETA_JAVA23 milestone Aug 14, 2024
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] DOM changes for markdown Javadoc comments
2 participants