Skip to content
This repository has been archived by the owner. It is now read-only.

JDK-8247994: Localize javadoc search #16

Closed

Conversation

@jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented Dec 13, 2020

This is for JDK16, as a precursor to fixing JDK-8258002.

While it is good to be using localized strings in the generated output, the significance for JDK-8258002 is that the strings are now obtained from a resource file, and not hardcoded in JavaScript file itself.

The source file search.js is renamed to search.js.template, and (unlike other template files) is copied as-is into the generated image. The values in the template are resolved when javadoc is executed, depending on the locale in use at the time. Because of the change in the file's extension, two makefiles are updated to accommodate the new extension: one is for the "interim" javadoc used to generate the API docs; the other is for the primary javadoc in the main JDK image.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk16 pull/16/head:pull/16
$ git checkout pull/16

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 13, 2020

👋 Welcome back jjg! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Loading

@openjdk openjdk bot added the rfr label Dec 13, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 13, 2020

@jonathan-gibbons The following labels will be automatically applied to this pull request:

  • build
  • compiler
  • core-libs
  • javadoc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

Loading

@@ -74,7 +74,7 @@ define SetupInterimModule
EXCLUDE_FILES := $(TOPDIR)/src/$1/share/classes/module-info.java \
Standard.java, \
EXTRA_FILES := $(BUILDTOOLS_OUTPUTDIR)/gensrc/$1.interim/module-info.java, \
COPY := .gif .png .xml .css .js .txt javax.tools.JavaCompilerTool, \
COPY := .gif .png .xml .css .js .js.template .txt javax.tools.JavaCompilerTool, \
Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons Dec 13, 2020

Choose a reason for hiding this comment

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

Build-folk: it would be nice if this macro could use $(jdk.javadoc_COPY) instead of having to duplicate entries.
(Future RFE?)

Loading

Copy link
Member

@magicus magicus Dec 15, 2020

Choose a reason for hiding this comment

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

I agree. The entire design of CompileJavaModules.gmk needs to be updated. I've been procrastinating on cleaning this up, maybe it's time to get going on it...

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 13, 2020

Loading

Copy link
Member

@hns hns left a comment

Looks good in general. <Unnamed> shouldn't be localized as it is an internal tag (see inline comment).

Loading

var highlight = "<span class=\"result-highlight\">$&</span>";
var searchPattern = "";
var fallbackPattern = "";
var RANKING_THRESHOLD = 2;
var NO_MATCH = 0xffff;
var MIN_RESULTS = 3;
var MAX_RESULTS = 500;
var UNNAMED = "<Unnamed>";
var UNNAMED = "##REPLACE:doclet.search.unnamed##";
Copy link
Member

@hns hns Dec 14, 2020

Choose a reason for hiding this comment

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

<Unnamed> is not a string that is ever shown to the user, it is what is used by javadoc to denote the default package (see toolkit.util.DocletConstants.DEFAULT_PACKAGE_NAME). This variable shouldn't be localized as that would break behaviour for code in the default package (and show the localized string as package name, instead of no package name).

Loading

Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons Dec 16, 2020

Choose a reason for hiding this comment

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

Noted, thanks

Loading

try {
sb.append(resources.getText(m.group("key")));
} catch (MissingResourceException e) {
sb.append(m.group());
Copy link
Member

@hns hns Dec 14, 2020

Choose a reason for hiding this comment

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

Shouldn't we propagate an error here, or issue a warning? If a key is missing localized properties the value from default properties should be used, so this should never happen, right?

Loading

Copy link
Member

@hns hns Dec 14, 2020

Choose a reason for hiding this comment

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

I see the added check for "##REPLACE:" in TestSearch.java will catch that case, so I guess it's ok.

Loading

Copy link
Member

@magicus magicus left a comment

Build changes look good.

Loading

@@ -74,7 +74,7 @@ define SetupInterimModule
EXCLUDE_FILES := $(TOPDIR)/src/$1/share/classes/module-info.java \
Standard.java, \
EXTRA_FILES := $(BUILDTOOLS_OUTPUTDIR)/gensrc/$1.interim/module-info.java, \
COPY := .gif .png .xml .css .js .txt javax.tools.JavaCompilerTool, \
COPY := .gif .png .xml .css .js .js.template .txt javax.tools.JavaCompilerTool, \
Copy link
Member

@magicus magicus Dec 15, 2020

Choose a reason for hiding this comment

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

I agree. The entire design of CompileJavaModules.gmk needs to be updated. I've been procrastinating on cleaning this up, maybe it's time to get going on it...

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 15, 2020

@jonathan-gibbons This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8247994: Localize javadoc search

Reviewed-by: hannesw, ihse

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 6 new commits pushed to the master branch:

  • 61cbf0f: 8258293: tools/jpackage/share/RuntimePackageTest.java#id0 with RuntimePackageTest.testUsrInstallDir2
  • 7aac4dc: 8257621: JFR StringPool misses cached items across consecutive recordings
  • 61390d8: 8257999: Parallel GC crash in gc/parallel/TestDynShrinkHeap.java: new region is not in covered_region
  • 952dc70: 8257636: Update usage of "type" terminology in java.lang.Class and java.lang.reflect
  • 04a1e5b: 8258505: [TESTBUG] TestDivZeroWithSplitIf.java fails due to missing UnlockDiagnosticVMOptions
  • 41f312e: 8254023: A module declaration is not allowed to be a target of an annotation that lacks an @target meta-annotation

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label Dec 15, 2020
@@ -37,7 +37,7 @@ var RANKING_THRESHOLD = 2;
var NO_MATCH = 0xffff;
var MIN_RESULTS = 3;
var MAX_RESULTS = 500;
var UNNAMED = "##REPLACE:doclet.search.unnamed##";
var UNNAMED = "<Unnamed>>";
Copy link
Member

@hns hns Dec 17, 2020

Choose a reason for hiding this comment

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

Oops, an additional closing angle bracket slipped there.

Loading

hns
hns approved these changes Dec 17, 2020
@jonathan-gibbons
Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons commented Dec 17, 2020

/integrate

Loading

@openjdk openjdk bot closed this Dec 17, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Dec 17, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 17, 2020

@jonathan-gibbons Since your change was applied there have been 8 commits pushed to the master branch:

  • 47c180d: 8258515: javac should issue an error if an annotation is nested in a local class or interface
  • cb5a6b1: 8258225: compiler/c2/cr6340864/TestIntVect.java runs faster in interpreter
  • 61cbf0f: 8258293: tools/jpackage/share/RuntimePackageTest.java#id0 with RuntimePackageTest.testUsrInstallDir2
  • 7aac4dc: 8257621: JFR StringPool misses cached items across consecutive recordings
  • 61390d8: 8257999: Parallel GC crash in gc/parallel/TestDynShrinkHeap.java: new region is not in covered_region
  • 952dc70: 8257636: Update usage of "type" terminology in java.lang.Class and java.lang.reflect
  • 04a1e5b: 8258505: [TESTBUG] TestDivZeroWithSplitIf.java fails due to missing UnlockDiagnosticVMOptions
  • 41f312e: 8254023: A module declaration is not allowed to be a target of an annotation that lacks an @target meta-annotation

Your commit was automatically rebased without conflicts.

Pushed as commit 30ca0a5.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Loading

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
3 participants