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

Wed Apr 5 13:52:45 UTC 2017

Hi Robert,

Thanks for the feedback, comments are inlined.

On 5.4.2017 04:18, Robert Field wrote:
>
> On 04/04/17 06:27, Jan Lahoda wrote:
>> Thanks for the comments. I prepared three patches (see inline
>> comments) for individual parts (so that we can skip some of them for 9
>> if needed), a complete webrev is here:
>> http://cr.openjdk.java.net/~jlahoda/8178013/webrev.00/
>>
>> On 1.4.2017 01:57, John Rose wrote:
>>> On Mar 27, 2017, at 1:39 AM, Jan Lahoda <jan.lahoda at oracle.com
>>> <mailto:jan.lahoda at oracle.com>> 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
>>>
>>> A couple of comments (on webrev.03):
>>>
>>> This help string has the wrong whitespace:
>>>
>>> +        a variable declaration whose type is based on the type of the
>>> expression.\n\t\t\
>>> +Shift-<tab> i\n\t\t\
>>>
>>> The first \n\t\t\ should be \n\n.
>>>
>>> Help strings should not mention <fix> anymore:
>>>
>>> +Invalid <fix> character.  Use "i" for auto-import or "v" for variable
>>> creation.  For more information see:\n\
>>> +   /help shortcuts
>>>
>>> I suggest:  Unexpected character after Shift-Tab.  Use "i" …
>>
>> Webrev for the two above is here:
>> http://cr.openjdk.java.net/~jlahoda/8178013/webrev.a.00/
>
> These changes look good.
>
> This help ends (line 507) with two tabs as well.  Help text should end
> with no special character (and yes, very inconsistent), see help.context
> So, this should be "...classpath."
>
> Oops, we haven't updated the copyright, can do that now.
>
> Don't need a re-review for these.

An updated webrev for the record is here:
http://cr.openjdk.java.net/~jlahoda/8178013/webrev.a.01/

>
> This is a noreg-doc if it were a stand-alone bug, and could be pushed
> without approval.  This might be a good idea as we are running out of
> l10n rope.
>
>>
>>>
>>> I built your patch and tried it out.  I like the new behavior very much.
>>> The "Shift-Tab i" command is more friendly.
>>>
>>> Thanks!
>>>
>>> I noticed a couple of oddities.
>>>
>>> First of all, "/help /set <Tab>" prints the "/set" subcommands, but
>>> "/help set <Tab>" does not.
>>>
>>> jshell> /help /set <Tab>
>>> editor       feedback     format       mode         prompt start
>>>        truncation
>>> jshell> /help set <Tab>
>>> get information about jshell   <==== YES, I THOUGHT I WAS DOING THAT?
>>
>> Webrev for this is here:
>> http://cr.openjdk.java.net/~jlahoda/8178013/webrev.b.00/
>
> Looks good.
>
> This will need approval (of course) but it is very simple, and that
> should help.  Or we wait for an update release an save the hassle.
>
> Oh, my, we haven't updated copyright on this either.

I'd like to ask for an approval to push the two above (i.e. a&b) under 
JDK-8178011. I've split out the third part below into: JDK-8178109. I 
can we can push that for 10 and probably an 9 update if you prefer.

>
>
>>
>>>
>>>
>>> Second, when completing inside help subcommands, the "todo" list is
>>> packed with generic
>>> "help on help", which promises information but isn't that useful.  If
>>> you are already saying
>>> "/help intro <Tab>" it doesn't seem very helpful to back up to the same
>>> help text as
>>> "/help <Tab><Tab>".
>>>
>>> A further example of the odd help-doc is below.
>>
>> Webrev for this is here:
>> http://cr.openjdk.java.net/~jlahoda/8178013/webrev.c.00/index.html
>
> 1708: This should also work if the prefix were "/?" or "/he"...

Ah, right.

> 1709: This loses the cursor info

Yes, but the cursor is only use for clipping the input code, so 
shouldn't be a problem.

> 1716: Since commands includes hidden and pseudo commands they will show
> up here.
> And since commands includes subjects (intended here) I believe that the
> comparison starting with the second letter will include help starting
> with the letters of "ontext", "ntro", and "hortcut"

Huh, right - I copied this from help, which also answers to "/help ntro".

In:
http://cr.openjdk.java.net/~jlahoda/8178013/webrev.c.01/

I fixed all the above, including cmdHelp.

How does that look?

Thanks,
     Jan

>
> This is complex enough and solves a small enough problem that I can't
> see trying to approve it for JDK 9.
> We pressing 9 issues, so let's save our strength.
>
> Thanks,
> Robert
>
>>
>> Any comments are welcome.
>>
>> Thanks,
>>     Jan
>>
>>>
>>> — John
>>>
>>> jshell> /help /set tr<Tab>
>>> ===REDRAW===>
>>> jshell> /help /set truncation<Tab>
>>> <press tab again to see synopsis>
>>>
>>> jshell> /help /set truncation<Tab>
>>> get information about jshell   <==== NOT A SYNOPSIS OF "truncation"
>>>
>>> <press tab again to see full documentation>
>>>
>>> jshell> /help /set truncation<Tab>
>>> Display information about jshell.   <==== NOT FULL DOC OF "truncation"
>>> /help
>>>      List the jshell commands and help subjects.
>>>
>>> /help <command>
>>>      Display information about the specified command. The slash must be
>>> included.
>>>      Only the first few letters of the command are needed -- if more
>>> than one
>>>      each will be displayed.  Example:  /help /li
>>>
>>> /help <subject>
>>>      Display information about the specified help subject. Example:
>>> /help intro
>>>
>>> jshell>
>>>
>