Re: [PATCH] Completion: Improve _man

[Oliver Kiddle <okiddle@xxxxxxxxxxx>]

dana wrote:
> Here's a *much* more complicated change. It enhances _man as follows:

This looks like a big improvement, thanks.

> * Complete all options to most major man variants
> * More accurately match sections
> * Better handle certain edge cases (see below)
> * Show descriptions for sections (suggested by Mikael i think)

Those look useful.

> Notes:
>
> * The function would previously use the provided section name as the leading
>   part of a directory pattern, so that on Solaris for example if you gave it 3p
>   it would also complete pages from 3pool, 3proc, &c. I couldn't think of a
>   reason this functionality would be desirable (man won't show you a page from a
>   section it's not in anyway), so i made it match exactly

Is this the line where you removed * from:
  dirs=( $^_manpath/(sman|man|cat)${^sects}*/ )

I think I've seen systems with directories named man1.Z/cat1.Z etc.
HP/UX perhaps. Perhaps also locales have occured such as man3f.fi_FI
I suspect the * was to handle something like that rather than for the
undesirable functionality you describe.

> * I chose sections to describe pretty arbitrarily. Solaris variants have 9034234
>   different sections and i didn't want to enumerate every single one, but some

I'm only seeing 270 sections, still too many to start enumerating them.

It looks like the sections will be changing with Solaris 11.4:

https://blogs.oracle.com/solaris/normalizing-man-page-section-numbers-in-solaris-114-v2

>   of them seem like they might come up a lot in regular use. It may be nice in
>   the future to give separate tags to the described and undescribed ones, so
>   that navigation is a little nicer for people with `group-name ''`

Am not so sure about that. Navigation seemed fine without. And _describe
doesn't currently support a mix of tags.

> * Could someone please review the way i return from _arguments? I don't think
>   it's correct (because it doesn't account for prefix-needed?), but i've been
>   looking at this for too long

Indeed, with the prefix-needed style set, _arguments adds the options
and then we should also add man pages in the state. (I use this with
_approximate to handle mistakes in the prefix itself). As it is, by
doing a return, the function ends before the state can be handled.
Usually, this is solved with a ret variable, but care would be needed
not to disturb the existing ret variable used later in the function. A
lazy alternative is to use $compstate[nmatches]

> I've tested this on several different platforms, but it's still a pretty major
> alteration to a function that i imagine sees a lot of traffic, so i'm a little
> nervous about it. If someone else could try it out that might be a good idea

For the most part, this looks great. I'll push it as-is for now. Any
further tweaks can then be done as incrementals diffs.

> +   '(-X --gxditview)'{-X-,--gxditview=-}'[display output in gxditview using specified DPI (default: 75)]::DPI' 

It's a minor thing but I find it more useful to put hints on the
arguments (units, defaults) etc in the heading for them.

  '(-X --gxditview)'{-X-,--gxditview=-}'[display output in gxditview optionally using specified resolution]::resolution (DPI) [75]'

> +  [[ $state == sects ]] && {
> +    local s
> +    local -a specs
> +
> +    (( $#sects )) || {
> +      _message 'manual section'

This should use _message -e and a tag so that it honours the user's
setting of the format style with the descriptions tag. And, to allow
the user to add some matches with the fake style.

> +      return 1
> +    }
> +
> +    # Build specs from descriptions
> +    for s in $sects; do
> +      specs+=( "${s}[${(b)sect_descs[$s]}]" )
> +    done

This results in all sections for which we don't have a description
getting an empty description. The descriptions that we have are then
pushed right over to the right like this:

3zonestat    5openssl  5oldap   4b                                          
7openssl     8oldap    7ipp            --                                     
1openssl                               -- OpenSSL commands                    
1t                                     -- Tcl/Tk features  

Try:
  _values x {a,b,c,d,e,f}'[]' 'g[description]'
vs.
  _values x {a,b,c,d,e,f} 'g[description]'

I'd suggest using _sequence wrapping _describe instead.
Initialise sect_descs in _describe format to begin with and then it can
be filtered like this:

  sect_descs=( 1:one 2:two )
  sects=( 1 2 3 4 )
  remove=( ${sect_descs%%:*} )
  sect_descs+=( ${sects:|remove} )

_describe also makes it easier to apply a more useful tag than just
values which can match the one from _message -e mentioned above.

Oliver