RFR 9: JDK-8177076: jshell tool: usability of completion

Wed Mar 29 17:33:32 UTC 2017

An updated version of the patch addressing the comments is here:
http://cr.openjdk.java.net/~jlahoda/8177076/webrev.02/

On 28.3.2017 21:31, Robert Field wrote:
>
> On 03/28/17 09:56, Jan Lahoda wrote:
>> Hi Robert,
>>
>> An updated version of the patch:
>> http://cr.openjdk.java.net/~jlahoda/8177076/webrev.01/
>
> I like the new CompletionTask design!
>
> ConsoleIOContext.java
> ----------------------------
> Copyright need update.

Fixed.

>
> l10n.properties
> ------------------
> 156 jshell.console.see.next.page = <press space for next page, any other
> key to quit>
> tab also works, so should be "tab or space"

The text is different now.

>
> 157 jshell.console.see.next.javadoc = <press tab or space for next
> javadoc, any other key to quit>
> If this remains (see below suggestion to change this), then it should be
> "documentation" rather than "javadoc", as elsewhere.

Done.

>
> 493         If there is more than one completion, show possible
> completions.\n\t\t\
> " then possible completions will be shown"

Done.

>
> 496         After a complete expression, press "Shift-<tab> v" to
> introduce a new variable\n\t\t\
> It may not be clear to people what those keyboard actions are
> "After a complete expression, hold down <shift> while pressing <tab>,
> then release and press "v", the expression will be converted to a
> variable declaration"

Done.

>
> 498 Shift-<tab> i\n\t\t\
> Same treatment.  And maybe "propose possible imports which will resolve
> the name..."
>

Done.

> MergedTabShiftTabTest.java
> ---------------------------------
> No bug ID on test.

Fixed.

>
>
> User Interface
> ------------------
>
> I'm wondering, would it be more consistent, when displaying javadoc, if
> instead of displaying this prompt between pages:
>
>      <press tab or space for next javadoc, any other key to quit>
>
> That it did a redrawLine()?
>
>       <press tab again for next documentation>
>
>       jshell> l.add(
>
> Implementation-wise, that each doc page was an item in the the todo list.
> Basically, I think getting rid of all prompts would be cleanest.

Done.

>
> -------
>
> What a tease, tell me there is documentation, and then... ;-)

Unfortunately, detecting whether there will be an actual javadoc is 
costly, so I don't think we would want to do that upfront.

>
> jshell> l
> l      long
>
> Signatures:
> l:java.util.List
>
> <press tab again to see documentation>
>
> jshell> l
> l:java.util.List
> <no javadoc found>
>
> jshell> l
>
> -------
>
> What to do when there are no completions?  Like:
>
> jshell> /zz<tab>
>
> or
>
> jshell> zz<tab>
>
> Before it did nothing, which is confusing.
> It could print "No completions" and redraw, but that would probably be
> noisy.
> I think what it does with this patch, which is redraw right below, is
> probably just right for snippets.
>
> jshell> /zz<tab>
> jshell> /zz
>
> Though, for commands (but not command arguments), maybe "No such
> command" and a redraw???

Done.

Jan

>
> -------
>
> shift-tab for fixes sure is nice.
>
> --------------
>
> Nice work!
>
> -Robert
>
>
>
>>
>> On 27.3.2017 23:20, Robert Field wrote:
>>> ConsoleIOContext.java
>>> ----------------------------
>>>
>>> One of the key pieces of usability feedback was to step away from modal
>>> prompts.
>>> Where, the cycle is on-tab: next-info/redraw.
>>> These changes move to that mechanism.
>>> However, the "too many completions" case uses a modal prompt (556).
>>> Seems this could be handled the same way.
>>
>> Not sure if there's a modal prompt on ConsoleIOContext:556, rev.00 -
>> there is a single read and after that the execution continues one way
>> or another? It is true the code looked a little bit different that in
>> the other cases. Anyway, this code does not exist in rev.01.
>>
>>>
>>> Nit: the variable declared on line 211: "String test", is that intended
>>> to be "text"?
>>
>> Yes, thanks. Fixed.
>>
>>>
>>> SourceCodeAnalysisImpl.java
>>> ------------------------------------
>>>
>>> These changes appear to be incomplete or abandoned.  They introduce a
>>> variable that is never set.
>>
>> Used by tests. Comment added.
>>
>>>
>>> User Interface
>>> ------------------
>>>
>>> jshell> List li
>>> l ==> null
>>>
>>> jshell> li.<tab>
>>> add(               addAll(            clear() contains(
>>> containsAll(       equals( forEach(           get( getClass()
>>> hashCode()         indexOf(           isEmpty() iterator()
>>> lastIndexOf(       listIterator( notify()           notifyAll()
>>> parallelStream()
>>> remove(            removeAll(         removeIf( replaceAll(
>>> retainAll(         set( size()             sort( spliterator()
>>> stream()           subList(           toArray( toString()         wait(
>>>
>>> jshell> li.add<tab>
>>> add(      addAll(
>>>
>>> jshell> li.add(<tab>
>>> Current signatures:
>>> boolean List<E extends Object>.add(E e)
>>> void List<E extends Object>.add(int index, E element)
>>>
>>> Possible completions:
>>>
>>> l     xxx   xyz
>>>
>>> <press tab again to see documentation>
>>>
>>> jshell> li.add(
>>>
>>> The first and second tab used in the example above give possible
>>> completions without a leading header "Possible completions:" of the
>>> third case. This should be consistent.
>>> The header could be added everywhere, but I think that would be noisy.
>>> The change in ordering is confusing anyhow.
>>> The prompt "<press tab again to see documentation>" would be more
>>> intuitive grouped with the method signatures.
>>> The header "Current signatures:", I think would be better
>>> "Signatures:",  "Method signatures:", or "Signatures of add:".
>>> Applying these:
>>>
>>> jshell> li.add(<tab>
>>> l     xxx   xyz
>>>
>>> Method signatures:
>>> boolean List<E extends Object>.add(E e)
>>> void List<E extends Object>.add(int index, E element)
>>>
>>> <press tab again to see documentation>
>>
>> Done. (The caption is "Signatures:", as in general, it is not always
>> method signatures there.)
>>
>>>
>>> jshell> li.add(
>>>
>>> See above, under ConsoleIOContext.java, additionally, the inlining of
>>> more text makes this more confusing, as no indication of what to do if
>>> you don't want it --
>>
>> Sorry, but I am not sure what is the action requested here.
>>
>>>
>>> jshell> 4 + <tab>
>>>
>>> Gets replaced with:
>>>
>>> jshell> 4 + 542 possible completions. Press <tab> or y to see them all.
>>>
>>> I'd opt for consistency (which also uses this prompt that is used in the
>>> after documentation case):
>>>
>>> jshell> 4 + <tab>
>>> <press tab again to see all possible completions; total possible
>>> completions: 542>
>>
>> Done. To do that, I re-wrote the support a little (as this basically
>> introduces a new state in the state "graph".)
>>
>>>
>>> jshell> 4 +
>>>
>>> I see also that if documentation is displayed and there are smart items,
>>> then the last tab choice is all items.  In no other case are all items
>>> offered if there are smart items.
>>
>> I believe all items may be shown where smart items exist and there's
>> no documnetation:
>> jshell> String str = new Str<tab>
>> String(
>>
>> <press tab again to see all possible completions>
>>
>> jshell> String str = new Str<tab>
>> StreamCorruptedException(          StreamTokenizer(
>> String(                            StringBuffer(
>> StringBufferInputStream(           StringBuilder(
>> StringIndexOutOfBoundsException(   StringJoiner(
>> StringReader(                      StringTokenizer( StringWriter(
>>
>> jshell> String str = new Str
>>
>>>
>>> The command help is a very nice feature.
>>>
>>> I'm unsure of the "<press tab again to see synopsis>" prompt line as the
>>> synopsis is also one line.
>>
>> There may be multiple lines with synopsis, I believe:
>> jshell> /e<tab>
>> /edit    /env     /exit
>>
>> <press tab again to see synopsis>
>>
>> jshell> /e<tab>
>> /edit
>> edit a source entry referenced by name or id
>>
>> /env
>> view or change the evaluation context
>>
>> /exit
>> exit jshell
>>
>> <press tab again to see full documentation>
>>
>> jshell> /e
>>
>>>
>>> Overall
>>> ---------
>>>
>>> Perhaps it would be good to lay this out as a UI design before turning
>>> it into code.
>>>
>>> Naively (as I haven't done it), I think it may be possible to regularize
>>> the processing (starting as pseudo-code derived from the UI design).
>>> I note 19 conditionals in the new code. I'm concerned about the
>>> branching complexity not only for code maintenance and testing but for
>>> what it implies about the user model.
>>
>> I think the primary question is if it works well for the users. Not
>> sure if the # of conditionals is so significant. I didn't really try
>> to minimize the # of conditions, I tried to make the code
>> understandable, making it simpler to say "if the situation is this,
>> then the outcome will be that". (The new revision goes one more step
>> in that direction, I think.)
>>
>> (Having this in the code also helps to cover all the corners, which
>> are easy to miss otherwise.)
>>
>> Anyway - if there's a UI design covering all the cases
>> (command/expression, has/hasn't doc, has/hasn't smart items,
>> has/hasn't additional items, there are/aren't too many items), I can
>> implement it.
>>
>> Jan
>>
>>>
>>> I will be available to help on this tomorrow, if you would like.
>>>
>>> -Robert
>>>
>>>
>>> On 03/27/17 01:39, Jan Lahoda wrote:
>>>> Hello,
>>>>
>>>> I'd like to ask for a review of a patch that merges shift-tab and tab
>>>> completions, and changes the fix shortcut to shift-tab:
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8177076
>>>> Webrev: http://cr.openjdk.java.net/~jlahoda/8177076/webrev.00/
>>>>
>>>> Any feedback is welcome,
>>>>      Jan
>>>
>