On Thu, 28 Oct 2021 21:02:04 GMT, Hannes Wallnöfer <[email protected]> wrote:
> Please review a change to add a nested `<code>` element for snippet tags as > well as HTML attributes for the snippet's `id` and `lang` attributes. The > change is quite simple, I did however encounter some gotchas. Below is a > short list of notes to explain some aspects of the change. > > - Both `id` and `lang` are only meaningful if they have non-empty values, so > defining them with empty or missing values will cause the respective HTML > attributes to be not defined. > - I decided to put the logic for extracting the `id` and `lang` attributes > into the already quite long `SnippetTaglet.getInlineTagOutput` method as this > is where the required preparation and infrastructure are located. Both values > are then passed as additional arguments to `TagletWriter.snippetTagOutput` > which is not great. I think it's ok for now, but maybe if we need to pass > more values along that way we should reconsider this design. > - I'm not sure whether we should assume `lang=java` for internal snippets. > This change currently does not, it only derives implicit `lang` attributes > for external snippets based on the extension of the snippet source file. > - In `SnippetTaglet.languageFromFilename()` I tried to cover what I reckoned > would be the most useful languages for javadoc users, including a few major > JVM languages. I'm sure I forgot some, but I hope I got at least those file > extensions right. > - We previously added a newline character to the snippet's `<pre>` element. > I think this was done in order to make sure the element is rendered even if > the snippet content is empty. This was fine with just the `<pre>` element, > but with a nested `<code>` element that newline is visible in the browser. I > therefore replaced the newline with `HtmlTree.EMPTY`. > - For the test, I was able to convert the existing `testIdAndLangAttributes` > test to cover explicit `id` and `lang` attributes. I added a new test called > `testExternalImplicitAttributes` to test derived `lang` attributes. Remove trailing whitespace that jcheck does not like. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SnippetTaglet.java line 336: > 334: case ".scala", ".sc" -> "scala"; > 335: case ".sh" -> "bash"; > 336: case ".tex" -> "latex"; My initial reaction to this is that we should not bake this list in its current form in javadoc. I might be okay with just two cases: `.java` and `.properties`. Perhaps we could provide a way for users to configure any mappings beyond those two externally. ------------- PR: https://git.openjdk.java.net/jdk/pull/6165
