From: Mark Walters <markwalters1009@gmail.com>
To: Tomi Ollila <tomi.ollila@iki.fi>, notmuch@notmuchmail.org
Subject: Re: [PATCH 00/11] contrib: pick: keybindings
In-Reply-To: <m2ppuwt7db.fsf@guru.guru-group.fi>
References: <1373047878-20822-1-git-send-email-markwalters1009@gmail.com>
	<m2ppuwt7db.fsf@guru.guru-group.fi>
User-Agent: Notmuch/0.15.2+192~g8222af3 (http://notmuchmail.org) Emacs/23.4.1
	(i486-pc-linux-gnu)
Date: Sun, 21 Jul 2013 09:36:38 +0100
Message-ID: <87ehas8g2x.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

Tomi Ollila <tomi.ollila@iki.fi> writes:

> On Fri, Jul 05 2013, Mark Walters <markwalters1009@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.

Hi 

Thanks for the review.

> 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...

That is just my not knowing (and I regret to say even thinking of) other
keyboard layouts. I agree that makes it a silly key. What about 'e'
(Enter as the mnemonic)?
>
>> 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

Yes that is a good point. I will add this in the next version.

Best wishes

Mark


> 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@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@notmuchmail.org
>> http://notmuchmail.org/mailman/listinfo/notmuch