[DOXIA-722] Optionally create anchors for index entries (used in TOC macro) #180

kwin · 2023-11-28T17:06:03Z

Introduce SinkWrapper concept
Enforce anchor name uniqueness across documents

Some preliminary documentation in apache/maven-doxia-site#30.

doxia-core/src/main/java/org/apache/maven/doxia/sink/impl/NamesForIndexEntriesSinkWrapper.java

michael-o

Putting the wrong JIRA issue aside...I am not convinced (convoluted at first glance) by this for several reasons:

I consider it a conceptual flaw that a macro can manipulate a parser, I had the same idea and dumped it
This seems like a reimplementation of RandomAccessSink: The RandomAccessSink provides the ability to create a {@link Sink} with hooks. Why not evaluate it if it there.
This should not be related to the TOC macro although the TOC macro will benefit. A user can request auto anchors regardless of the TOC macro. Don't conflate them.
I think that a solution should be logically identical to org.apache.maven.doxia.parser.Parser.setEmitComments(boolean). A parser feature will intercept section title text and buffer until sectionTitle_() is hit, anchor and title (heading) are emitted.

doxia-core/src/main/java/org/apache/maven/doxia/macro/MacroRequest.java

kwin · 2023-11-29T09:47:16Z

This seems like a reimplementation of RandomAccessSink: The RandomAccessSink provides the ability to create a {https://github.com/link Sink} with hooks. Why not evaluate it if it there.

I was not aware of this class, but RandomAccessSink is complex to grasp for me. I would rather go for reimplementing RandomAccessSink as special SinkWrapper. AFAIK RandomAccessSink does not allow to intercept sink API calls and enrich them but just does some buffering and concatenating multiple sink outputs into one effective one.

This should not be related to the TOC macro although the TOC macro will benefit. A user can request auto anchors regardless of the TOC macro. Don't conflate them.

For me the TOC is the primary use case, but I am ok with always generating anchors and TOC macro will just use those.

I think that a solution should be logically identical to org.apache.maven.doxia.parser.Parser.setEmitComments(boolean). A parser feature will intercept section title text and buffer until sectionTitle_() is hit, anchor and title (heading) are emitted.

As long as the default is "emit anchors for every section title" I am fine with adding an option to disable that. I don't think we should make that an opt-in behaviour as that would be breaking change from a user's perspective.

michael-o · 2023-12-02T21:42:51Z

This seems like a reimplementation of RandomAccessSink: The RandomAccessSink provides the ability to create a {https://github.com/link Sink} with hooks. Why not evaluate it if it there.

I was not aware of this class, but RandomAccessSink is complex to grasp for me. I would rather go for reimplementing RandomAccessSink as special SinkWrapper. AFAIK RandomAccessSink does not allow to intercept sink API calls and enrich them but just does some buffering and concatenating multiple sink outputs into one effective one.

I don't see need to modify/intercept the sink at all, a parser can do this perfectly with some buffering. TOCMacro does this buffering with a fresh parser as well.

This should not be related to the TOC macro although the TOC macro will benefit. A user can request auto anchors regardless of the TOC macro. Don't conflate them.

For me the TOC is the primary use case, but I am ok with always generating anchors and TOC macro will just use those.

Correct, TOC will use what has been generated, therefore a parser switch...

I think that a solution should be logically identical to org.apache.maven.doxia.parser.Parser.setEmitComments(boolean). A parser feature will intercept section title text and buffer until sectionTitle_() is hit, anchor and title (heading) are emitted.

As long as the default is "emit anchors for every section title" I am fine with adding an option to disable that. I don't think we should make that an opt-in behaviour as that would be breaking change from a user's perspective.

Well, making it default would be breaking because Apt, for example does not have implicit anchors. What I see is tht the opt-in will be enabled from the Maven Site Plugin invocation and not Doxia directly. That will cover your use case and won't break anything for others.

kwin · 2023-12-03T10:40:07Z

I don't see need to modify/intercept the sink at all, a parser can do this perfectly with some buffering. TOCMacro does this buffering with a fresh parser as well.

The point is that a sink wrapper can be used with all parsers, while buffering on parser level has to be implemented per parser.

michael-o · 2023-12-03T19:23:11Z

I don't see need to modify/intercept the sink at all, a parser can do this perfectly with some buffering. TOCMacro does this buffering with a fresh parser as well.

The point is that a sink wrapper can be used with all parsers, while buffering on parser level has to be implemented per parser.

Are you certain about this? Can't this be done in the AbstractParser?

doxia-core/src/main/java/org/apache/maven/doxia/parser/Parser.java

doxia-core/src/main/java/org/apache/maven/doxia/index/IndexEntry.java

doxia-core/src/main/java/org/apache/maven/doxia/index/IndexingSink.java

doxia-core/src/main/java/org/apache/maven/doxia/parser/AbstractParser.java

doxia-core/src/main/java/org/apache/maven/doxia/parser/SinkWrapperFactory.java

michael-o

One last nit

doxia-core/src/main/java/org/apache/maven/doxia/index/IndexingSink.java

michael-o

This is fine for me now. I cannot come up with a better solution in reasonable time. Before this get merged I'd prefer that @hboutemy will also take a look as well. This is a lot of code and needs eyes. Give him some time.

doxia-core/src/main/java/org/apache/maven/doxia/index/IndexEntry.java

michael-o · 2023-12-16T20:43:44Z

@kwin Please rename with a proper DOXIA JIRA issue. Maybe it would even make sense to split up in (a) Wrapper Factory approach and (b) Auto Indexer Feature

doxia-core/src/main/java/org/apache/maven/doxia/index/IndexEntry.java

michael-o · 2023-12-27T20:26:11Z

...a-core/src/main/java/org/apache/maven/doxia/sink/impl/UniqueAnchorNamesValidatorFactory.java

+    @Override
+    public int getPriority() {
+        // should come last (after potential preprocessing/modification of anchor names)
+        return -1000;


Does using Integer.MIN_VALUE make sense?

michael-o · 2023-12-27T20:27:05Z

@hboutemy Are you able to review within a week or so?

slachiewicz

can we somehow check it based on our maven-site and the doxia converter?

kwin · 2023-12-30T12:32:51Z

can we somehow check it based on our maven-site and the doxia converter?

Doesn't affect Doxia Converter, for checking with m-site-p you need a patch for m-site-p and m-doxia-sitetools (see some WIP PRs in https://issues.apache.org/jira/browse/MSITE-1000 and https://issues.apache.org/jira/browse/DOXIASITETOOLS-322).

michael-o

The the actual change is perfectly fine, this needs a new JIRA issue because it does not really solve DOXIA-710 because we cannot solve the problem within the skin.
Please create a new issue and change the commit message. It can only solve external linking and interplay between TOC macro and the content itself.

macro)

kwin · 2024-01-04T16:35:48Z

The the actual change is perfectly fine, this needs a new JIRA issue because it does not really solve DOXIA-710 because we cannot solve the problem within the skin. Please create a new issue and change the commit message. It can only solve external linking and interplay between TOC macro and the content itself.

Done in 4518d58 associated with new JIRA issue DOXIA-722

michael-o · 2024-01-05T15:24:32Z

See my request to rephrase the the issue summary/title.

kwin · 2024-01-05T15:25:32Z

I renamed the JIRA title, the commit message should be fine already.

michael-o

Good work. Let's merge!

kwin requested review from gnodet, hboutemy, michael-o and slawekjaranowski November 28, 2023 17:06

kwin force-pushed the feature/auto-generate-anchors branch from b2eab0c to 66ae55b Compare November 28, 2023 17:09

kwin commented Nov 28, 2023

View reviewed changes

doxia-core/src/main/java/org/apache/maven/doxia/sink/impl/NamesForIndexEntriesSinkWrapper.java Outdated Show resolved Hide resolved

michael-o requested changes Nov 28, 2023

View reviewed changes

doxia-core/src/main/java/org/apache/maven/doxia/macro/MacroRequest.java Outdated Show resolved Hide resolved

kwin force-pushed the feature/auto-generate-anchors branch from 66ae55b to 0bebc8c Compare December 15, 2023 16:53

michael-o requested changes Dec 15, 2023

View reviewed changes

doxia-core/src/main/java/org/apache/maven/doxia/parser/Parser.java Outdated Show resolved Hide resolved

michael-o reviewed Dec 16, 2023

View reviewed changes

doxia-core/src/main/java/org/apache/maven/doxia/index/IndexEntry.java Show resolved Hide resolved

michael-o reviewed Dec 16, 2023

View reviewed changes

michael-o requested changes Dec 16, 2023

View reviewed changes

doxia-core/src/main/java/org/apache/maven/doxia/index/IndexingSink.java Show resolved Hide resolved

michael-o approved these changes Dec 16, 2023

View reviewed changes

michael-o requested changes Dec 16, 2023

View reviewed changes

doxia-core/src/main/java/org/apache/maven/doxia/index/IndexEntry.java Outdated Show resolved Hide resolved

kwin changed the title ~~[DOXIASITETOOLS-320] Auto-generate anchor for TOC entries~~ [DOXIA-710] Auto-generate anchor for TOC entries Dec 16, 2023

michael-o requested changes Dec 23, 2023

View reviewed changes

doxia-core/src/main/java/org/apache/maven/doxia/index/IndexEntry.java Outdated Show resolved Hide resolved

doxia-core/src/main/java/org/apache/maven/doxia/index/IndexEntry.java Outdated Show resolved Hide resolved

kwin force-pushed the feature/auto-generate-anchors branch from ca72f70 to 921d2f4 Compare December 27, 2023 15:52

kwin marked this pull request as ready for review December 27, 2023 20:24

michael-o approved these changes Dec 27, 2023

View reviewed changes

slachiewicz approved these changes Dec 30, 2023

View reviewed changes

michael-o self-requested a review January 2, 2024 16:00

michael-o requested changes Jan 2, 2024

View reviewed changes

[DOXIA-722] Optionally create anchors for index entries (used in TOC

4518d58

macro)

kwin force-pushed the feature/auto-generate-anchors branch from 1c2cf8a to 4518d58 Compare January 4, 2024 16:34

kwin changed the title ~~[DOXIA-710] Auto-generate anchor for TOC entries~~ [DOXIA-722] Optionally generate anchors for TOC entries Jan 4, 2024

kwin mentioned this pull request Jan 4, 2024

[DOXIASITETOOLS-324] Allow configuration of Doxia parser apache/maven-doxia-sitetools#126

Closed

kwin requested a review from michael-o January 5, 2024 15:13

michael-o changed the title ~~[DOXIA-722] Optionally generate anchors for TOC entries~~ [DOXIA-722] Optionally create anchors for index entries (used in TOC macro) Jan 5, 2024

michael-o approved these changes Jan 5, 2024

View reviewed changes

kwin merged commit 7a04c03 into master Jan 5, 2024
22 checks passed

kwin deleted the feature/auto-generate-anchors branch January 5, 2024 15:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DOXIA-722] Optionally create anchors for index entries (used in TOC macro) #180

[DOXIA-722] Optionally create anchors for index entries (used in TOC macro) #180

kwin commented Nov 28, 2023 •

edited

Loading

michael-o left a comment

kwin commented Nov 29, 2023 •

edited

Loading

michael-o commented Dec 2, 2023

kwin commented Dec 3, 2023

michael-o commented Dec 3, 2023

michael-o left a comment

michael-o left a comment

michael-o commented Dec 16, 2023

michael-o Dec 27, 2023

michael-o commented Dec 27, 2023

slachiewicz left a comment

kwin commented Dec 30, 2023

michael-o left a comment

kwin commented Jan 4, 2024

michael-o commented Jan 5, 2024 •

edited

Loading

kwin commented Jan 5, 2024

michael-o left a comment

[DOXIA-722] Optionally create anchors for index entries (used in TOC macro) #180

[DOXIA-722] Optionally create anchors for index entries (used in TOC macro) #180

Conversation

kwin commented Nov 28, 2023 • edited Loading

michael-o left a comment

Choose a reason for hiding this comment

kwin commented Nov 29, 2023 • edited Loading

michael-o commented Dec 2, 2023

kwin commented Dec 3, 2023

michael-o commented Dec 3, 2023

michael-o left a comment

Choose a reason for hiding this comment

michael-o left a comment

Choose a reason for hiding this comment

michael-o commented Dec 16, 2023

michael-o Dec 27, 2023

Choose a reason for hiding this comment

michael-o commented Dec 27, 2023

slachiewicz left a comment

Choose a reason for hiding this comment

kwin commented Dec 30, 2023

michael-o left a comment

Choose a reason for hiding this comment

kwin commented Jan 4, 2024

michael-o commented Jan 5, 2024 • edited Loading

kwin commented Jan 5, 2024

michael-o left a comment

Choose a reason for hiding this comment

kwin commented Nov 28, 2023 •

edited

Loading

kwin commented Nov 29, 2023 •

edited

Loading

michael-o commented Jan 5, 2024 •

edited

Loading