Re: RFR: 8220497: Improve Javadoc search feature and add test coverage

[Jonathan Gibbons]

In the Markdown spec, inconsistent capitalization of "javadoc search".
We can't do much about it, but all those links to JLS contain a version 
number that will have to be periodically updated.

Core vs Peripheral Matches ... "Although a method's signature is not 
considered a core component" ... earlier you defined a signature as 
including the name, which is surely part of the signature.  The rest of 
this sentence  "it is possible to search for method signatures by 
starting the search string with|(|" suggests that you are trying to just 
refer to the method's parameters. Also, in this sentence you focus on 
"methods" but elsewhere you take care to differentiate "methods" and 
"constructors", suggesting that constructors are not included here.

White space: what is the rationale for the special case?  I would have 
expected the whitespace to be ignored by the "beginning or ending" 
clause in the previous bullet, leaving an empty query string, which 
should be treated as "no query".

Supported Browsers ... it is "wishy-washy" to say that "it should 
work...". That implies you don't actually know if it works in those 
browsers.  I suggest changing the sentence to "It is supported in the 
following browsers:"  The rows in the table seem to be randomly 
ordered.  You give a version for MSIE, but not for any other browsers. 
I'm surprised Chrome is limited to Windows, as compared to "All OSes 
that support Chrome".

(I was reading spec.01, as given in the links below, but I see there are 
.02 versions available. spec.02 does not contain the stylesheet.

http://cr.openjdk.java.net/~hannesw/8220497/webrev.02/test/langtools/jdk/javadoc/doclet/testSearchScript/javadoc-search.js.html
Test has wrong license header ... tests should use plain GPL2, not GPL2+CE.

New tests look good. If you fix the test license, the webrev is 
approved. I presume the spec will be in a different changeset.

-- Jon

On 4/11/19 8:19 AM, Hannes Wallnöfer wrote:
Thanks for the review, Jon.

New spec/webrev/docs:

Spec: http://cr.openjdk.java.net/~hannesw/8220497/spec.01/
Webrev: http://cr.openjdk.java.net/~hannesw/8220497/webrev.01/
API docs: http://cr.openjdk.java.net/~hannesw/8220497/api.01/


Am 29.03.2019 um 21:34 schrieb Jonathan Gibbons <jonathan.gibb...@oracle.com>:


The spec:
The overview  is a bit of a mix of spec and impl details.
I scrapped the overview and replaced it with a short rationale of why we need 
this spec, which is basically as an addendum to JEP 225 that defines the actual 
search behaviour.


Under Definitions

I was surprised to see the new word "entity" until I realized it needs to 
include both elements and custom searchable items. Maybe       that should be said 
somewhere.
Added.

"Upper case" and "lower case" are normally written as single words.

Fixed.

Are we allowed non-letter characters in a camel-case search?  i.e. should the 
definition of camel-case include the non-alphabetic characters
I changed the definition of camel-case so it doesn’t sound like it lists all 
valid types of characters - that’ s what we refer to the JLS for.

Should "all-caps" (and "camel-case" include $

I removed all-caps since the underscore as word boundary now applies to all 
identifiers, see below.

We already do support ‚$‘ in camel-case and partial matches.


Under Searchable Entities

typo: entitis

Under Types


Change "consist" to "consists"

Under each of Packages, Types and Members


The current pattern is "The signature of <plural> consists of the ..."  It would be more 
correct to say "The signature of a <single-item> consists of …“.

All fixed.


Under Matching Rules/Left Boundaries:


3rd bullet: why restrict letter following "_" to all-caps, and not after "_" in 
all cases?

Great idea, I made that change and it works well.


Under Matching Rules/Camel case:


Wouldn't it be better to allow digits and other non-alpha characters, treating 
them as the same as lower-case?

Again, tweaked the text to make that clear.


Under Matching Rules/White space:


It seems a bug that typing excess whitespace should cause the match to fail. 
You should ignore all whitespace everywhere in the search string.
I spent quite a bit of time on getting whitespace right. Ignoring all 
whitespace seems wrong as it would mean that „ob ject“ matches 
java.lang.Object. The solution I found is to make white space significant if it 
separates two words/identifiers, and make it optional if it occurs at the edge 
of the query string or before/after a separator character.

This more or less replicates white space in Java syntax, and it happens to also 
work well for search tags.

    
Under Browser Requirements:


"Requirements" seems like the wrong word. How about "Supported Browsers", with 
a note that it may work on other browsers as well. Also, you should support Chrome on All OSs that 
support Chrome.

Changed it to „supported browsers“.

Code

I note the watermark is fixed in English. Is there any easy way to make it 
localizable?

That would have been part of my HTML5 placeholder attribute solution for 
JDK-8221366. It is still possible as things are, but a bit more cumbersome and 
I’d prefer to keep that separate from this issue.

General question, for later: should we publish/use a minified version of 
search.js?

I don’t think that’s necessary. The file is below 15 kb, so one of the smaller 
assets we load.

Hannes


Other functionality questions are implicit in the comments on the spec.

-- Jon