[PATCH 00/11] contrib: pick: keybindings

[Tomi Ollila]

On Fri, Jul 05 2013, Mark Walters <markwalters1009 at gmail.com> wrote:

> This series adds lots of keybindings and funtionality to pick: in
> particular it adds the stash keymap, the ability to tab between and
> activate buttons in the message pane, and it reduces a lot of code
> duplication between pick and show.
>
> It is a large series: but most of it is a lot of small changes. These
> small changes are mostly logically independent but as they add
> keybindings their contexts all clash. I have made most of the
> keybindings as single separate patches to make discussion of them
> individually easier.
>
> The key patches for review/discussion (apart from bike-shedding on
> key-bindings!) are patches 1, 5 and 8.

Now that you "asked", I'd ask what was used as a reason to decide "/"
as keybinding for notmuch-pick-button-activate -- something common
or as a convenience. In US/UK keyboard "/" is left to Right Shift,
but for example in my keyboard it is Shift-7...

> Patch 1 is the most "controversial": it over-rides
> notmuch-show-get-prop so that whether it uses
> notmuch-show-get-message-properties or
> notmuch-pick-get-message-properties depends on the major-mode (ie
> whether it is called from pick or show).

At the moment the code comments could have something like XXX to
emphasize the situation. IMHO it is ok to have this in contrib
code -- when this is going to be "official" part of emacs MUA
then this code needs to be ... well, at least moved to other place :D

Apart from these code LGTM.

Tomi


> This means that functions from show which just use message properties
> (most often just the message id) "just work" when called from pick. In
> particular it gives us access to lots of functions without having to
> duplicate the code.
>
> In the longer term it would be better to have some show/pick common
> file and migrate the common functions there.
>
> Patch 5 and 8 add in functions for creating fucntions ready to be used
> in keybindings. The one in patch 5 takes a show fucntion and creates a
> function which switches from pick to the message pane, applies the
> function and then switches back to pick. The one in patch 8 takes a
> function and creates a function which closes the message pane and the
> calls this function.
>
> Both of these make the keybinding section clearer. They also have the
> advantage that the user can use them easily to create their own
> keybindings which do this.
>
> This completes all the keybindings I use and I think means that pick
> doesn't have any glaring omissions.
>
> Finally, this will clash with the thread archive patches
> id:1371195472-441-1-git-send-email-markwalters1009 at gmail.com
>
> Best wishes
>
> Mark
>
> Mark Walters (11):
>   contrib: pick: override notmuch-show-get-prop
>   contrib: pick: Link in notmuch-show-pipe-message
>   contrib: pick: Link in attachment functions straight from
>     notmuch-show
>   contrib: pick: Link in stash map straight from notmuch-show
>   contrib: pick: add in to-message-window function
>   contrib: pick: add button press helper
>   contrib: pick: pass tab through to the message pane
>   contrib: pick: close window function
>   contrib: pick: make help close the message pane first
>   contrib: pick: add in binding to view raw message
>   contrib: pick: use close-message-pane for reply etc
>
>  contrib/notmuch-pick/notmuch-pick.el |  139 +++++++++++++++++-----------------
>  1 files changed, 70 insertions(+), 69 deletions(-)
>
> -- 
> 1.7.9.1
>
> _______________________________________________
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch