RFR 9: JDK-8177076: jshell tool: usability of completion
Robert Field
robert.field at oracle.com
Wed Mar 29 22:49:02 UTC 2017
Looks really good.
Two small things which don't need another round of review --
503 which will resolve the identifiers based on the content of
the specified classpath.\n\t\t\
should be "identifier" not "identifiers"
163 jshell.console.no.javadoc = <no javadoc found>
Should, like the other cases, be "documentation" rather than "javadoc".
Thanks,
Robert
On 03/29/17 10:33, Jan Lahoda wrote:
> 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
>>>>
>>
More information about the kulla-dev
mailing list