On Thu, 11 Jan 2024 14:46:10 GMT, Hannes Wallnöfer <hann...@openjdk.org> wrote:
>> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/script.js.template
>> line 1:
>>
>>> 1: /*
>>
>> I note that at least in part this is a rename of `script.js` (and rightly
>> so) that Git has failed to detect (grrr.) Others may want to use external
>> tools to compare the old and new forms. The differences are primarily a
>> significant expansion of these lines in the old code:
>>
>>
>> // Dynamically set scroll margin to accomodate for draft header
>> document.addEventListener("DOMContentLoaded", function(e) {
>> document.querySelectorAll(':not(input)[id]').forEach(
>> function(c) {
>> c.style["scroll-margin-top"] =
>> Math.ceil(document.querySelector("header").offsetHeight) + "px"
>> });
>> });
>
> Right. I wondered if I had done anything wrong when renaming the file, but it
> seems `git` always handles renames as remove-add, and the only way to
> influence rename detection is in `git diff`. I guess I could have prevented
> this by renaming and adding/updating in two different commits, which I will
> do in the future.
While doing rename and edit in separate commits may help the PR, it does not
help the long term code archaeology, because the commits will be squashed when
integrating them to the mainline repo. The rename-detection is a regrettable
shortcoming of Git itself; there is nothing we can do about it.
>> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/script.js.template
>> line 251:
>>
>>> 249: setTopMargin();
>>> 250: // Make sure current element is visible in breadcrumb navigation
>>> on small displays
>>> 251: const subnav = document.querySelector("ol.sub-nav-list");
>>
>> This is but one instance of many similar `querySelector` calls that leverage
>> CSS class names and/or HTML ids.
>>
>> In the Java code, we used named constants for such names, defined in
>> `HtmlStyle` and `HtmlId`, which allows us to use an IDE to track their
>> usage. But that does not work across the language boundary into code like
>> this. I wonder (just asking) if it is worth manually marking the Java
>> constants in some way to indicate the names are also used in JavaScript
>> code. As an example, this could be done with an individual compile-time
>> arg-free annotation on each constant that needs to be marked, or a single
>> annotation on (say) the `HtmlStyle` class containing an array of enum values
>> for the relevant constants.
>
> Interesting suggestion! What would the advantages of using annotations be
> compared to just using comments, which could also contain a description of
> where/how the constants are used?
Annotations may provide slightly more checking than stylized comments ... such
as for typos and/or renames, etc. They can also be the basis for checks in
tests. For example, the `javac` code (and to a lesser extent, `javadoc`) uses
the `@DefinedBy` annotation to indicate which methods are exported through
public API, and should not be modified without due consideration. IIRC, there
are tests that methods that have such an annotation are indeed part of the
stated API (given as an argument to the annotation.)
>> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/search.js.template
>> line 33:
>>
>>> 31: loading: "##REPLACE:doclet.search.loading##",
>>> 32: searching: "##REPLACE:doclet.search.searching##",
>>> 33: redirecting: "##REPLACE:doclet.search.redirecting##",
>>
>> Suggestion ...
>> Would it make sense to "minimize" the use of templates by moving all the
>> template-y stuff to a single template file, that can be included by the
>> other JavaScript files, which would then become "normal" JavaScript `.js`
>> files.
>
> That's something we should consider. We could use `script.js` for this as it
> is the first loaded script file, therefore available in all other script
> files.
I was suggesting that the template-y stuff should be moved to a new
template-only file, sort-of like a "template header file", to be used by "pure
JavaScript" `*.js` files.
>> test/langtools/jdk/javadoc/doclet/testCopyFiles/TestCopyFiles.java line 60:
>>
>>> 58: // check top navbar
>>> 59: """
>>> 60: <a href="../package-tree.html">Tree</a>""",
>>
>> What is the significance of this filename change?
>
> For context-specific links in the top navigation bar we always link to the
> target closest to the current page; for example, The "USE" link in class
> documentation links to the Class Use page of the current class, and the
> "TREE" link in files belonging to a package links to the Package Tree page.
> This was previously not handled correctly for package-level doc files. It is
> now fixed, so the test has to be updated.
Thanks for the explanation.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17062#discussion_r1453889234
PR Review Comment: https://git.openjdk.org/jdk/pull/17062#discussion_r1453895227
PR Review Comment: https://git.openjdk.org/jdk/pull/17062#discussion_r1453898283
PR Review Comment: https://git.openjdk.org/jdk/pull/17062#discussion_r1453899149
