Re: [STUMP] Tab searchable menus

[Ben Spencer]

Hi Chris,

I've rebased this patch against the menu code refactoring I committed
this morning and tweaked it a bit in the process.  New patch attached,
comments follow.


On Mon, Mar 07, 2011 at 11:47:41PM +0100, Chris Kelly wrote:
> I've added some code to the latest menu.lisp to do the following:
> 
> - highlight the letters being searched for where they appear in the menu
> - tab cycles search results
> - backspace deletes characters from the search

I wonder whether it would be more useful to actually remove
non-matches from the list rather than just highlighting matches?


> I've (temporarily?) disabled jJ kK keybindings in the patch although I
> really think it is better if there is a search feature at all not to
> have searchable letters reserved for navigation. However, the patch
> will still work with j/k keybindings at least as well it does now and
> otherwise all keys work exactly as they did.

Personally I don't have a problem with this, and I guess anyone who
does can add them back in.  I don't think we support vi-style bindings
out of the box anywhere else.


> Perhaps requiring the
> user to type '/' before search could be a good compromise?

That seems like more of a breaking change, though I guess we could add
it as an option.


> This is my first contribution and I'm not the worlds greatest lisp
> programmer, so all comments/advice are welcome and please let me know
> of any style errors or bugs I should fix.

highlight-substring:

It seems a bit inefficient to do the search for a match twice, maybe
this function could accept non-matching strings and just do nothing to
them.  That said, it's currently not used anywhere else so does this
even need to be in its own function?

I've found a slight issue in that the highlighted characters have the
case that was used in the search string rather than the original
string - eg search for 'X' and urxvt will appear as urXvt.

I'm a bit confused by both of the comments on this function.  'only
searches the first part' presumably means that it will only highlight
the first match, but I have no idea about the union-mode bit.


menu-cycle-match:

Not sure why you're building matchlist up front rather than doing the
search inside the loop?

You can avoid the progn by using when instead of if - the same should
probably apply to the other two ifs since they have no else.

You can use the 'collecting' clause to build up a list in a LOOP.  It
also might be nicer to use 'for x from 0' rather than incf.

The last chunk could be rewritten with a single setf with the ifs
inside.


select-from-menu:

I've made fairly significant tweaks here to fit in better with my
changes to the function, let me know what you think.


Ben

0001-updated-menu-so-that-it-highlights-search-matches-wh.patch
Description: Text Data