[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] WINID completion for herbstcommander



Hi Gabor,

I'll ignore the previous herbstcommander patch in preference of your
latter patch and because I didn't like the wselect.sh modifications (see
the %2d-section in this mail for more details).

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).

> +				                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)

> +				                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:

> + printf "%2d) %s %-3s [%s] %s\n" $i $id $tag $comm "$title"

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/'

Regards,
Thorsten