On Tue, Nov 27, 2018 at 11:28:31AM -0600, Scott Cheloha wrote:
> No objections here to the feature in general. We already support reversing
> select orderings in systat(1), which I've found useful in practice, so this
> is not without precedent and is consistent with at least one other monitoring
> tool in userland.
>
> I think it's a bit unfortunate that 'r' in interactive mode is already
> assigned
> to renice the process. If we did that here it'd be consistent with
> systat(1)'s
> interactive keybindings and a boon for overall UX. But 'r' has mean "renice"
> since before top(1) was even imported, so at this late date I don't think we
> can
> change the binding for 'r'. However, adding an 'R' shortcut to reverse the
> ordering
> for the current primary key might be a reasonable addition in a later diff.
I thought about a toggle as well but went with the "-" prefix semantics
since the existing toggles we currently have (`C', `h', `H') are
distinct features not related to filters like `g' or `o'. That is,
currently you cannot toggle something like "only user kn/all but kn".
> > Index: display.c
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/top/display.c,v
> > retrieving revision 1.57
> > diff -u -p -r1.57 display.c
> > --- display.c 17 Nov 2018 23:10:08 -0000 1.57
> > +++ display.c 24 Nov 2018 14:09:38 -0000
> > @@ -817,7 +817,8 @@ show_help(void)
> > "I | i - toggle the display of idle processes\n"
> > "k [-sig] pid - send signal `-sig' to process `pid'\n"
> > "n|# count - show `count' processes\n"
> > - "o field - specify sort order (size, res, cpu, time, pri, pid,
> > command)\n"
> > + "o [-]field - specify sort order (size, res, cpu, time, pri, pid,
> > command)\n"
> > + " (o -field sorts in reverse)\n"
>
> "o" isn't needed, as contextually you're talking about the 'o' shortcut.
Keeping "o" is consistent with the rest: "g+ selects all commands" and
"u -user hides user" for example.
> > @@ -879,10 +877,10 @@ rundisplay(void)
> > new_message(MT_standout,
> > "Order to sort: ");
> > if (readline(tempbuf, sizeof(tempbuf)) > 0) {
> > - if ((i = string_index(tempbuf,
> > - statics.order_names)) == -1) {
> > + if ((i = getorder(tempbuf)) == -1) {
> > new_message(MT_standout,
> > " %s: unrecognized sorting order",
> > + tempbuf[0] == '-' ? tempbuf + 1 :
> > tempbuf);
>
> You aren't ducking the '-' during init so I don't think you should hide it
> here.
> You could even refactor the two into something like
We're also not "ducking" it for `u' in the option parsing. Admittedly,
this is inconsistent but I'd like to keep both in the command loop. The
command line options are parsed once so with an erroneous `-U --kn'
users might see "--kn: unknown user" only once on startup when in fact
"-kn" is the unknown user.
Stripping the dash in error messages from the command line which is
likely used repeatedly at runtime ensures that the correct username
is printed.
> void setorder(const char *);
>
> to keep the messages in sync, but that should be done in a subsequent diff.
So I if at all, I suggest stripping the dash in the option parsing
routine as well for consistency in separate diff.
