RFR 9: JDK-8177076: jshell tool: usability of completion
Jan Lahoda
jan.lahoda at oracle.com
Tue Mar 28 16:56:01 UTC 2017
Hi Robert,
An updated version of the patch:
http://cr.openjdk.java.net/~jlahoda/8177076/webrev.01/
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