RFR 9: JDK-8177076: jshell tool: usability of completion
Jan Lahoda
jan.lahoda at oracle.com
Thu Mar 30 12:48:07 UTC 2017
Thanks. Fixed the two below here:
http://cr.openjdk.java.net/~jlahoda/8177076/webrev.03/
Thanks,
Jan
On 30.3.2017 00:49, Robert Field wrote:
> 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