Replying to Jon's email rather than to Hannes's to acknowledge that I have had
a chance to read both.
Many thanks for doing this, Hannes. This is certainly a big improvement as it
addresses the issue of having no UI feedback in case of slow connections. I'd
say that this change solves 80% of the original issue and the remaining 20%
should be left for future improvements.
1. That said, I don't like the idea of cramming individual index files into a
single file as a future improvement. Although I appreciate that it would make
the solution more simple, it would be less user-friendly: the user will have to
load the complete index even if they actually need only a part of it. Depending
on the codebase individual index files can differ in size significantly. For
example, here is the index for the upcoming JDK 15:
$ find build/macosx-x64/images/docs -iname "*-index.js" -exec du -h '{}' \;
24K build/macosx-x64/images/docs/api/tag-search-index.js
4.0K build/macosx-x64/images/docs/api/module-search-index.js
12K build/macosx-x64/images/docs/api/package-search-index.js
4.3M build/macosx-x64/images/docs/api/member-search-index.js
224K build/macosx-x64/images/docs/api/type-search-index.js
Jon mentioned that "types" is probably the most searched category. If that is
true, which I think it is, then to access those 224K of "types" index, a user
will have to wait until all 4.6M have been loaded. While HTTP compression
reduces absolute times required to load index, the relative differences remain
significant:
3K tag-search-index.js
1K module-search-index.js
2K package-search-index.js
559K member-search-index.js
35K type-search-index.js
If the main motivation for coalescing index files was this:
> search results are updated multiple times - once for each search index file
> that is loaded. This is a bit annoying
then it could be approached differently.
2. If you care about empty files, maybe the doclet could inject the list of
expected files into script.js? (The doclet generates JavaScript code anyway:
the "*-index.js" files are JavaScript.)
3. I think it's the first time I noticed it: shouldn't the user-facing labels
in script.js be localized?
var noResult = {l: "No results found"};
var loading = {l: "Loading search index..."};
var catModules = "Modules";
var catPackages = "Packages";
var catTypes = "Types";
var catMembers = "Members";
var catSearchTags = "SearchTags";
(If so, it should be addressed in a separate RFE, obviously.)
The below are ideas for future improvement.
4. Change the label to "Loading search index (results may be incomplete)..." or
communicate that extra piece of information on "incompleteness" some other way.
5. Keep displaying that "Loading..." label on top (first line) of the dropdown
until *the complete* index has been loaded and restore the selection every time
results have been incrementally updated; currently the selection resets to the
first line. That last bit might prove difficult since a position of the mouse
pointer can interfere with the selection process.
I'm not fluent in JavaScript, but from what I can see the code *looks good*.
The solution seems robust. Again, many thanks for doing this.
-Pavel
> On 11 Jun 2020, at 02:14, Jonathan Gibbons <jonathan.gibb...@oracle.com>
> wrote:
>
> Hannes,
>
> Code looks good and seems to work OK.
>
> It would be nice to see a version without the 5 second delay, but I guess we
> will see soon enough. The 5 second delay makes clear that the index files
> are loaded for every page. I hope that once the files have been cached, we
> don't get any flashing from a message popping up to say that files are being
> loaded.
>
> I agree with deferring the discussion on using a single index file. The
> original intent was to make it faster to load types, for which the perception
> was that these are probably the most common search items.
>
> -- Jon
>
> Separate/bonus discussion: triggered by thinking of the 5 second delay ...
> would it work to have a "debug mode" such that the generated docs may contain
> an extra page which can be used to set cookies for the API presentation,
> which can be used to "configure" search?
>
> On 6/10/20 2:44 PM, Hannes Wallnoefer wrote:
>> Please review a fix to display a „Loading search index…“ message in javadoc
>> search while the search index files are loading and updating the results
>> when loading is completed.
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8236935
>> Webrev: http://cr.openjdk.java.net/~hannesw/8236935/webrev.00/
>>
>> This introduces an updateSearchResults function in script.js that is empty
>> by default. When a search is performed before the index files have been
>> fully loaded, the updateSearchResults function is replaced with a version
>> that will update the search results when an index file is loaded and
>> evaluated.
>>
>> An invocation of updateSearchResults() is added at the end of each search
>> index file. Also, as a consequence of this change all search index files are
>> generated regardless of whether it is empty or not (provided the index
>> option is enabled). This means a bit of added overhead for docs without
>> modules or search tags, but the cost should be neglectible for a mostly
>> empty script file.
>>
>> JDK API docs generated with this patch at the URL below. These docs also
>> feature an extra 5 second delay for loading the search index files to make
>> it easier to test the new feature.
>>
>> http://cr.openjdk.java.net/~hannesw/8236935/api.00/
>>
>> One thing you might notice is that sometimes search results are updated
>> multiple times - once for each search index file that is loaded. This is a
>> bit annoying, and it also reminded me that loading each index in a separate
>> file probably adds quite a bit of network and evaluation overhead. As an
>> experiment, I created a version that combines all search indices in one
>> single file. I’m not proposing that for 15 since it’s a bigger change that
>> shouldn’t be rushed, but I’m including it here as a possible next step.
>>
>> Single search index file (just for evaluation, not proposed for JDK 15):
>> Webrev: http://cr.openjdk.java.net/~hannesw/8236935/webrev.singleindex.00/
>> API docs: http://cr.openjdk.java.net/~hannesw/8236935/api.singleindex.00/
>>
>> Hannes
