Re: [PATCH] WINID completion for herbstcommander

[Gabor Adam Toth <tg _at_ tgbit _dot_ net>,]

>>>>> On Tue, 2 Oct 2012 02:53:05 +0200, Thorsten WiÃmann <edu _at_ thorsten _minus_ wissmann _dot_ de> said:

> I like your patch and this feature will be merged eventually, but first
> I'd like you to resend the patch with these problems fixed:

> On Tue, Sep 25, 2012 at 10:59:14AM +0200, Gabor Adam Toth wrote:
> > +			raise|jumpto|bring)
> > +				tags=( $($herbstclient_cmd tag_status) )
> > +				i=1
> > +				completion=$(
> > +                                        wmctrl -lp | while read line; do
> > +				                fields=( $line )
> > [...]
> > +				        done
> > +                                )

> Here, and in the lines between the completion=$( and the matching ) tabs
> and spaces are mixed. Please format it properly (the initial author's
> decision was: Use tabs for indentation).

Indeed, fixed that, seems like the rest of the codebase is using spaces and my
editor was configured for that.

> > +				                comm=$(ps -o comm= ${fields[2]})

> This prints the incorrect command name if herbstcommander.sh is running
> on a different host than the application that is described in $field. So
> either omit the command name or just print the pid or directly read the
> command name directly from WM_COMMAND property of the application.

> Note that WM_COMMAND is optional and doesn't need to be set (just like
> the _NET_WM_PID property)

Yes, good point, I decided to use WM_CLASS instead, that's always present
for both local and remote apps and either the class or instance name contains the
application name, which I would often use to search in the list, and sometimes
WM_NAME does not contain this information.

> > +				                if [ $i -gt 1 ]; then
> > +					                echo
> > +				                fi

> That's unneeded, you can just remove these 3 lines and add a \n at the
> end of the format string:

Agreed, this is not needed, it remained in there from a previous approach.

> Because of the %2d, spaces are sometimes added in front the first
> number. We shouldn't do that (and they were explicitly removed in
> wselect.sh) because of the ranking algorithm of dmenu: When entering a
> filter, menu items that start with that filter are ranked higher than
> items that only contain the filter somewhere.

> So it would be better if a window is ranked higher if the number of its
> line is entered. You probably added it so that the columns are pretty
> formated. To retain formatted columns you can move the format spaces
> next to the ")" instead of in front of the number by adding this after
> the done:

> + 			done | sed 's/\([ ]*\)\([0-9])\)/\2\1/'
..or just use %-3s in the format string and "$i)" as the argument

A patch will follow with these changes.

Regards,
Gabor