[RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[David Edmondson]

Okay, I'll buy it. Patch version 5 in a while...
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: 

Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[David Edmondson]

Okay, I'll buy it. Patch version 5 in a while...


pgpc7jlWB9atp.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[David Edmondson]

On Mon, 26 Dec 2011 10:49:46 +, David Edmondson  wrote:
> > > + ((= start-of-message (point))
> > > +  ;; The cursor is at the start of the current message - move to
> > > +  ;; the previous open message.
> > > +  (notmuch-show-previous-open-message))
> > 
> > This will jump to the beginning of the previous message if I'm at the
> > beginning of a message.  I would expect rewind to show me the end of
> > the previous message in this case.
> 
> That would definitely more closely be the inverse of how advance works,
> but is it the most useful behaviour?

I thought about this a bit more (mostly because I don't want to write
tests for one behaviour and then have to change them - writing tests for
emacs with the current test suite is painful).

If you want to go back a page you can use M-v. The whole point of
binding DEL to something in `notmuch-show-mode' is that it implement
something other than the vanilla behaviour. Simply showing the previous
page (the equivalent of M-v) adds no value.

Hence, I'd like to keep the (jump back a full message) behaviour in the
patch.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: 

[RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[Aaron Ecay]

On Wed, 28 Dec 2011 15:22:45 +, David Edmondson  wrote:
> I thought about this a bit more (mostly because I don't want to write
> tests for one behaviour and then have to change them - writing tests for
> emacs with the current test suite is painful).
> 
> If you want to go back a page you can use M-v. The whole point of
> binding DEL to something in `notmuch-show-mode' is that it implement
> something other than the vanilla behaviour. Simply showing the previous
> page (the equivalent of M-v) adds no value.

If you want to jump back to the previous message, you can press `p'.
(And M-v is a chord whereas DEL and p are a simple keystroke, so it?s
arguably maximally convenient to duplicate a chord command on a single
key rather than duplicating a single key on another single key.)

The way I look at it, notmuch has two sets of movement keys ? n/p and
SPC/DEL.  The former moves by messages, and the latter by screenfuls
(with the added complication that the screenful movement commands also
stop at intervening message boundaries).  I?d prefer to maintain that
symmetry.

-- 
Aaron Ecay

Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[Jameson Graef Rollins]

On Wed, 28 Dec 2011 13:04:02 -0500, Aaron Ecay  wrote:
> The way I look at it, notmuch has two sets of movement keys – n/p and
> SPC/DEL.  The former moves by messages, and the latter by screenfuls
> (with the added complication that the screenful movement commands also
> stop at intervening message boundaries).  I’d prefer to maintain that
> symmetry.

agreed.

jamie.


pgpahp2ZUGPvL.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[Jameson Graef Rollins]

On Wed, 28 Dec 2011 13:04:02 -0500, Aaron Ecay  wrote:
> The way I look at it, notmuch has two sets of movement keys ? n/p and
> SPC/DEL.  The former moves by messages, and the latter by screenfuls
> (with the added complication that the screenful movement commands also
> stop at intervening message boundaries).  I?d prefer to maintain that
> symmetry.

agreed.

jamie.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
URL: 

Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[Aaron Ecay]

On Wed, 28 Dec 2011 15:22:45 +, David Edmondson  wrote:
> I thought about this a bit more (mostly because I don't want to write
> tests for one behaviour and then have to change them - writing tests for
> emacs with the current test suite is painful).
> 
> If you want to go back a page you can use M-v. The whole point of
> binding DEL to something in `notmuch-show-mode' is that it implement
> something other than the vanilla behaviour. Simply showing the previous
> page (the equivalent of M-v) adds no value.

If you want to jump back to the previous message, you can press `p'.
(And M-v is a chord whereas DEL and p are a simple keystroke, so it’s
arguably maximally convenient to duplicate a chord command on a single
key rather than duplicating a single key on another single key.)

The way I look at it, notmuch has two sets of movement keys – n/p and
SPC/DEL.  The former moves by messages, and the latter by screenfuls
(with the added complication that the screenful movement commands also
stop at intervening message boundaries).  I’d prefer to maintain that
symmetry.

-- 
Aaron Ecay
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[David Edmondson]

On Mon, 26 Dec 2011 10:49:46 +, David Edmondson  wrote:
> > > + ((= start-of-message (point))
> > > +  ;; The cursor is at the start of the current message - move to
> > > +  ;; the previous open message.
> > > +  (notmuch-show-previous-open-message))
> > 
> > This will jump to the beginning of the previous message if I'm at the
> > beginning of a message.  I would expect rewind to show me the end of
> > the previous message in this case.
> 
> That would definitely more closely be the inverse of how advance works,
> but is it the most useful behaviour?

I thought about this a bit more (mostly because I don't want to write
tests for one behaviour and then have to change them - writing tests for
emacs with the current test suite is painful).

If you want to go back a page you can use M-v. The whole point of
binding DEL to something in `notmuch-show-mode' is that it implement
something other than the vanilla behaviour. Simply showing the previous
page (the equivalent of M-v) adds no value.

Hence, I'd like to keep the (jump back a full message) behaviour in the
patch.


pgpnFJc4WwTsr.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[David Edmondson]

On Mon, 26 Dec 2011 14:24:11 -0800, Jameson Graef Rollins  wrote:
> On Mon, 26 Dec 2011 22:00:21 +, David Edmondson  wrote:
> > Understood. For me this fell inside the 'trivial other change' boundary.
> 
> Fwiw, I don't remember there ever being such a distinction.  I think
> we've always insisted that unrelated changes should be excluded.

I didn't mean to claim that the project had such a rule, just that I did.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: 

Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[David Edmondson]

On Mon, 26 Dec 2011 14:24:11 -0800, Jameson Graef Rollins 
 wrote:
> On Mon, 26 Dec 2011 22:00:21 +, David Edmondson  wrote:
> > Understood. For me this fell inside the 'trivial other change' boundary.
> 
> Fwiw, I don't remember there ever being such a distinction.  I think
> we've always insisted that unrelated changes should be excluded.

I didn't mean to claim that the project had such a rule, just that I did.


pgpunmanmk6KO.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[David Edmondson]

On Mon, 26 Dec 2011 15:09:55 +0400, Dmitry Kurochkin  wrote:
> Hi David.
> 
> On Mon, 26 Dec 2011 10:46:13 +, David Edmondson  wrote:
> > On Fri, 23 Dec 2011 23:01:33 +0400, Dmitry Kurochkin  > gmail.com> wrote:
> > > * Revert changes to notmuch-show-advance-and-archive.
> > 
> > Why? (I mean, because the change is poor or just that it's unrelated or
> > because I didn't mention it)
> > 
> 
> Because it is unrelated.

Understood. For me this fell inside the 'trivial other change' boundary.

> And can you please explain why `when' is better than `if' here?  Then I
> will know which one to use the next time :)

`if' allows only a single statement for `then', which results in code like:

 (if foo
(progn
  (this)
  (that)
  (theother)))

so if there is no `else' clause I've been preferring:

  (when foo
(this)
(that)
(theother))

but that's obviously personal and not important in this specific case.

> > > * Can we split this in two patches?  One for rewind and another for
> > >   advance.
> > 
> > I'll think about that. Is there a specific reason? I'm not particularly
> > in favour of splitting things just for the sake of it.
> > 
> 
> Because they are independent and can be split.  And it is easier to
> review (and work in general, I suppose) with two smaller patches than
> with a single bigger one.

Your git-fu is obviously much stronger than mine. :-) Rebasing (groups
of) patches takes more of my time and is more error prone than I'd like.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: 

[RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[Dmitry Kurochkin]

Hi David.

On Mon, 26 Dec 2011 10:46:13 +, David Edmondson  wrote:
> On Fri, 23 Dec 2011 23:01:33 +0400, Dmitry Kurochkin  gmail.com> wrote:
> > * Revert changes to notmuch-show-advance-and-archive.
> 
> Why? (I mean, because the change is poor or just that it's unrelated or
> because I didn't mention it)
> 

Because it is unrelated.

And can you please explain why `when' is better than `if' here?  Then I
will know which one to use the next time :)

> > * Can we split this in two patches?  One for rewind and another for
> >   advance.
> 
> I'll think about that. Is there a specific reason? I'm not particularly
> in favour of splitting things just for the sake of it.
> 

Because they are independent and can be split.  And it is easier to
review (and work in general, I suppose) with two smaller patches than
with a single bigger one.

Though, since you got two other reviews already, you can just ignore
this.

> > * Does this patch change the behavior of the functions or is it just
> >   meant to simplify the code?  If it is the former, it would be really
> >   nice to have tests for it.
> 
> I believe that it changes the behaviour. I'll write tests.

Thanks.

Regards,
  Dmitry

Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[Jameson Graef Rollins]

On Mon, 26 Dec 2011 22:00:21 +, David Edmondson  wrote:
> Understood. For me this fell inside the 'trivial other change' boundary.

Fwiw, I don't remember there ever being such a distinction.  I think
we've always insisted that unrelated changes should be excluded.  As a
general rule, I think all patches should be as atomic as they can
possibly be.  I would much rather have more smaller, atomic patches than
fewer longer, composite ones.

> > And can you please explain why `when' is better than `if' here?  Then I
> > will know which one to use the next time :)
> 
> `if' allows only a single statement for `then', which results in code like:
> 
>  (if foo
> (progn
>   (this)
>   (that)
>   (theother)))
> 
> so if there is no `else' clause I've been preferring:
> 
>   (when foo
> (this)
> (that)
> (theother))
> 
> but that's obviously personal and not important in this specific case.

That's good.  I like it.  The if construction always annoyed me a bit
for this reason.  The when construction is definitely cleaner.

jamie.


pgp1RQ8yrX9FA.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[Jameson Graef Rollins]

On Mon, 26 Dec 2011 22:00:21 +, David Edmondson  wrote:
> Understood. For me this fell inside the 'trivial other change' boundary.

Fwiw, I don't remember there ever being such a distinction.  I think
we've always insisted that unrelated changes should be excluded.  As a
general rule, I think all patches should be as atomic as they can
possibly be.  I would much rather have more smaller, atomic patches than
fewer longer, composite ones.

> > And can you please explain why `when' is better than `if' here?  Then I
> > will know which one to use the next time :)
> 
> `if' allows only a single statement for `then', which results in code like:
> 
>  (if foo
> (progn
>   (this)
>   (that)
>   (theother)))
> 
> so if there is no `else' clause I've been preferring:
> 
>   (when foo
> (this)
> (that)
> (theother))
> 
> but that's obviously personal and not important in this specific case.

That's good.  I like it.  The if construction always annoyed me a bit
for this reason.  The when construction is definitely cleaner.

jamie.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
URL: 

Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[David Edmondson]

On Mon, 26 Dec 2011 15:09:55 +0400, Dmitry Kurochkin 
 wrote:
> Hi David.
> 
> On Mon, 26 Dec 2011 10:46:13 +, David Edmondson  wrote:
> > On Fri, 23 Dec 2011 23:01:33 +0400, Dmitry Kurochkin 
> >  wrote:
> > > * Revert changes to notmuch-show-advance-and-archive.
> > 
> > Why? (I mean, because the change is poor or just that it's unrelated or
> > because I didn't mention it)
> > 
> 
> Because it is unrelated.

Understood. For me this fell inside the 'trivial other change' boundary.

> And can you please explain why `when' is better than `if' here?  Then I
> will know which one to use the next time :)

`if' allows only a single statement for `then', which results in code like:

 (if foo
(progn
  (this)
  (that)
  (theother)))

so if there is no `else' clause I've been preferring:

  (when foo
(this)
(that)
(theother))

but that's obviously personal and not important in this specific case.

> > > * Can we split this in two patches?  One for rewind and another for
> > >   advance.
> > 
> > I'll think about that. Is there a specific reason? I'm not particularly
> > in favour of splitting things just for the sake of it.
> > 
> 
> Because they are independent and can be split.  And it is easier to
> review (and work in general, I suppose) with two smaller patches than
> with a single bigger one.

Your git-fu is obviously much stronger than mine. :-) Rebasing (groups
of) patches takes more of my time and is more error prone than I'd like.


pgpd6Dp9TFmpP.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[David Edmondson]

On Sun, 25 Dec 2011 23:11:27 -0500, Aaron Ecay  wrote:
> > > +   ((> (let ((visible-bottom (notmuch-show-message-bottom)))
> > > +  (while (invisible-p visible-bottom)
> > > +(setq visible-bottom (max (point-min)
> > > +  (1- (previous-single-char-property-change
> > > +   visible-bottom 'invisible)
> > > +  visible-bottom) (window-end))
> 
> Can this (let...) be lifted out of the (cond...)?  IMO it is very
> confusing to be doing non-trivial computation in the test portion of a
> cond form.

It ends up a long way from where it's used, diluting the value of the
comment. I do like the current layout, but what if it was (something
like):

   ((let ((visible-bottom (1- (notmuch-show-message-bottom
  (while (invisible-p visible-bottom)
(setq visible-bottom (max (point-min)
  (1- (previous-single-char-property-change
   visible-bottom 'invisible)
  (> visible-bottom (window-end)))
;; The end of this message is not visible - scroll to show more of
;; it.
(scroll-up)
nil)

That would seem more palatable, perhaps.

> Agreed.  I would like to see this case move back one screenful of text or
> to the previous beginning-of-message, whichever is shorter.

See previous comment - I agreed that it's not symmetric - just wonder
which is more useful behaviour.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: 

[RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[David Edmondson]

On Sat, 24 Dec 2011 20:06:35 -0500, Austin Clements  wrote:
> Awesome.  This looks significantly cleaner.  I think this is worth
> pushing for the comment you added to notmuch-show-advance alone.

Thanks.

> This definitely changes the behavior of rewind, but other than one
> case I point out below, I think what you have now is much closer to an
> inverse of advance.  It would be nice to have tests for rewind (looks
> like we don't have any right now), but it would seem counterproductive
> to write tests for the current rewind only to rewrite them for this
> rewind.

I'll write some tests.

> Tailing whitespace.

Will fix.

> > + ((= start-of-message (point))
> > +  ;; The cursor is at the start of the current message - move to
> > +  ;; the previous open message.
> > +  (notmuch-show-previous-open-message))
> 
> This will jump to the beginning of the previous message if I'm at the
> beginning of a message.  I would expect rewind to show me the end of
> the previous message in this case.

That would definitely more closely be the inverse of how advance works,
but is it the most useful behaviour?
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: 

[RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[David Edmondson]

On Fri, 23 Dec 2011 23:01:33 +0400, Dmitry Kurochkin  wrote:
> * Revert changes to notmuch-show-advance-and-archive.

Why? (I mean, because the change is poor or just that it's unrelated or
because I didn't mention it)

> * Can we split this in two patches?  One for rewind and another for
>   advance.

I'll think about that. Is there a specific reason? I'm not particularly
in favour of splitting things just for the sake of it.

> * Does this patch change the behavior of the functions or is it just
>   meant to simplify the code?  If it is the former, it would be really
>   nice to have tests for it.

I believe that it changes the behaviour. I'll write tests.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: 

Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[Dmitry Kurochkin]

Hi David.

On Mon, 26 Dec 2011 10:46:13 +, David Edmondson  wrote:
> On Fri, 23 Dec 2011 23:01:33 +0400, Dmitry Kurochkin 
>  wrote:
> > * Revert changes to notmuch-show-advance-and-archive.
> 
> Why? (I mean, because the change is poor or just that it's unrelated or
> because I didn't mention it)
> 

Because it is unrelated.

And can you please explain why `when' is better than `if' here?  Then I
will know which one to use the next time :)

> > * Can we split this in two patches?  One for rewind and another for
> >   advance.
> 
> I'll think about that. Is there a specific reason? I'm not particularly
> in favour of splitting things just for the sake of it.
> 

Because they are independent and can be split.  And it is easier to
review (and work in general, I suppose) with two smaller patches than
with a single bigger one.

Though, since you got two other reviews already, you can just ignore
this.

> > * Does this patch change the behavior of the functions or is it just
> >   meant to simplify the code?  If it is the former, it would be really
> >   nice to have tests for it.
> 
> I believe that it changes the behaviour. I'll write tests.

Thanks.

Regards,
  Dmitry
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[David Edmondson]

On Sun, 25 Dec 2011 23:11:27 -0500, Aaron Ecay  wrote:
> > > +   ((> (let ((visible-bottom (notmuch-show-message-bottom)))
> > > +  (while (invisible-p visible-bottom)
> > > +(setq visible-bottom (max (point-min)
> > > +  (1- (previous-single-char-property-change
> > > +   visible-bottom 'invisible)
> > > +  visible-bottom) (window-end))
> 
> Can this (let...) be lifted out of the (cond...)?  IMO it is very
> confusing to be doing non-trivial computation in the test portion of a
> cond form.

It ends up a long way from where it's used, diluting the value of the
comment. I do like the current layout, but what if it was (something
like):

   ((let ((visible-bottom (1- (notmuch-show-message-bottom
  (while (invisible-p visible-bottom)
(setq visible-bottom (max (point-min)
  (1- (previous-single-char-property-change
   visible-bottom 'invisible)
  (> visible-bottom (window-end)))
;; The end of this message is not visible - scroll to show more of
;; it.
(scroll-up)
nil)

That would seem more palatable, perhaps.

> Agreed.  I would like to see this case move back one screenful of text or
> to the previous beginning-of-message, whichever is shorter.

See previous comment - I agreed that it's not symmetric - just wonder
which is more useful behaviour.


pgp1AQrb8ylu3.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[David Edmondson]

On Sat, 24 Dec 2011 20:06:35 -0500, Austin Clements  wrote:
> Awesome.  This looks significantly cleaner.  I think this is worth
> pushing for the comment you added to notmuch-show-advance alone.

Thanks.

> This definitely changes the behavior of rewind, but other than one
> case I point out below, I think what you have now is much closer to an
> inverse of advance.  It would be nice to have tests for rewind (looks
> like we don't have any right now), but it would seem counterproductive
> to write tests for the current rewind only to rewrite them for this
> rewind.

I'll write some tests.

> Tailing whitespace.

Will fix.

> > + ((= start-of-message (point))
> > +  ;; The cursor is at the start of the current message - move to
> > +  ;; the previous open message.
> > +  (notmuch-show-previous-open-message))
> 
> This will jump to the beginning of the previous message if I'm at the
> beginning of a message.  I would expect rewind to show me the end of
> the previous message in this case.

That would definitely more closely be the inverse of how advance works,
but is it the most useful behaviour?


pgpA6QlFRDbPJ.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[David Edmondson]

On Fri, 23 Dec 2011 23:01:33 +0400, Dmitry Kurochkin 
 wrote:
> * Revert changes to notmuch-show-advance-and-archive.

Why? (I mean, because the change is poor or just that it's unrelated or
because I didn't mention it)

> * Can we split this in two patches?  One for rewind and another for
>   advance.

I'll think about that. Is there a specific reason? I'm not particularly
in favour of splitting things just for the sake of it.

> * Does this patch change the behavior of the functions or is it just
>   meant to simplify the code?  If it is the former, it would be really
>   nice to have tests for it.

I believe that it changes the behaviour. I'll write tests.


pgpz5s3ZtMqTM.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[Aaron Ecay]

On Sat, 24 Dec 2011 20:06:35 -0500, Austin Clements  wrote:
> Awesome.  This looks significantly cleaner.  I think this is worth
> pushing for the comment you added to notmuch-show-advance alone.

+1

> Quoth David Edmondson on Dec 23 at  6:41 pm:
> > The advance/rewind functions had become complex, which made it hard to
> > determine how they are expected to behave. Re-implement them simply in
> > order to poll user-experience and expectation.
> > ---
> > 
> > Switched back to using `previous-single-char-property-change' now that
> > Aaron explained it. Fix a bug rewinding when the start of the current
> > message is visible.
> > 
> >  emacs/notmuch-show.el |  132 
> > +++--
> >  1 files changed, 73 insertions(+), 59 deletions(-)
> > 
> > diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> > index 46525aa..e914ce1 100644
> > --- a/emacs/notmuch-show.el
> > +++ b/emacs/notmuch-show.el
> > @@ -1156,38 +1156,56 @@ Some useful entries are:
> >  ;; Commands typically bound to keys.
> >  
> >  (defun notmuch-show-advance ()
> > -  "Advance through thread.
> > +  "Advance through the current thread.
> >  
> > -If the current message in the thread is not yet fully visible,
> > -scroll by a near screenful to read more of the message.
> > +Scroll the current message if the end of it is not visible,
> > +otherwise move to the next message.
> >  
> > -Otherwise, (the end of the current message is already within the
> > -current window), advance to the next open message."
> > +Return `t' if we are at the end of the last message, otherwise
> > +`nil'."
> >(interactive)
> > -  (let* ((end-of-this-message (notmuch-show-message-bottom))
> > -(visible-end-of-this-message (1- end-of-this-message))
> > -(ret nil))
> > -(while (invisible-p visible-end-of-this-message)
> > -  (setq visible-end-of-this-message
> > -   (previous-single-char-property-change visible-end-of-this-message
> > - 'invisible)))
> > -(cond
> > - ;; Ideally we would test `end-of-this-message' against the result
> > - ;; of `window-end', but that doesn't account for the fact that
> > - ;; the end of the message might be hidden.
> > - ((and visible-end-of-this-message
> > -  (> visible-end-of-this-message (window-end)))
> > -  ;; The bottom of this message is not visible - scroll.
> > -  (scroll-up nil))
> > -
> > - ((not (= end-of-this-message (point-max)))
> > -  ;; This is not the last message - move to the next visible one.
> > -  (notmuch-show-next-open-message))
> > -
> > - (t
> > -  ;; This is the last message - change the return value
> > -  (setq ret t)))
> > -ret))
> > +  (cond
> > +   ((eobp)
> > +;; We are at the end of the buffer - move to the next thread.
> > +t)
> > +
> > +   ;; Ideally we would simply do:
> > +   ;; 
> 
> Tailing whitespace.
> 
> > +   ;;  ((> (notmuch-show-message-bottom) (window-end))
> > +   ;; 
> 
> More trailing whitespace.
> 
> > +   ;; here, but that fails if the trailing text in the buffer is
> > +   ;; invisible (`window-end' returns the last _visible_ character,
> > +   ;; which can then be smaller than `notmuch-show-message-bottom').
> > +   ;;
> > +   ;; So we need to find the last visible character of the message. We
> > +   ;; do this by searching backwards from
> > +   ;; `notmuch-show-message-bottom' for changes in the `invisible'
> > +   ;; property until we find a non-invisible character. When we find
> > +   ;; such a character we test to see whether it is visible in the
> > +   ;; window.
> > +   ;;
> > +   ;; Properties change between characters - the return value of
> > +   ;; `previous-single-char-property-change' points to the first
> > +   ;; character _inside_ the region with the `invisible' property
> > +   ;; set. To allow for this we step backwards one character upon
> > +   ;; finding the start of the invisible region.
> > +
> > +   ((> (let ((visible-bottom (notmuch-show-message-bottom)))
> > +(while (invisible-p visible-bottom)
> > +  (setq visible-bottom (max (point-min)
> > +(1- (previous-single-char-property-change
> > + visible-bottom 'invisible)
> > +visible-bottom) (window-end))

Can this (let...) be lifted out of the (cond...)?  IMO it is very
confusing to be doing non-trivial computation in the test portion of a
cond form.

> > +;; The end of this message is not visible - scroll to show more of
> > +;; it.
> > +(scroll-up)
> > +nil)
> > +
> > +   (t
> > +;; All of the current message has been seen - show the start of
> > +;; the next open message.
> > +(notmuch-show-next-open-message)
> > +nil)))
> >  
> >  (defun notmuch-show-advance-and-archive ()
> >"Advance through thread and archive.
> > @@ -1201,44 +1219,40 @@ from each message), kills the buffer, and displays 
> > the nex

Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[Aaron Ecay]

On Sat, 24 Dec 2011 20:06:35 -0500, Austin Clements  wrote:
> Awesome.  This looks significantly cleaner.  I think this is worth
> pushing for the comment you added to notmuch-show-advance alone.

+1

> Quoth David Edmondson on Dec 23 at  6:41 pm:
> > The advance/rewind functions had become complex, which made it hard to
> > determine how they are expected to behave. Re-implement them simply in
> > order to poll user-experience and expectation.
> > ---
> > 
> > Switched back to using `previous-single-char-property-change' now that
> > Aaron explained it. Fix a bug rewinding when the start of the current
> > message is visible.
> > 
> >  emacs/notmuch-show.el |  132 
> > +++--
> >  1 files changed, 73 insertions(+), 59 deletions(-)
> > 
> > diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> > index 46525aa..e914ce1 100644
> > --- a/emacs/notmuch-show.el
> > +++ b/emacs/notmuch-show.el
> > @@ -1156,38 +1156,56 @@ Some useful entries are:
> >  ;; Commands typically bound to keys.
> >  
> >  (defun notmuch-show-advance ()
> > -  "Advance through thread.
> > +  "Advance through the current thread.
> >  
> > -If the current message in the thread is not yet fully visible,
> > -scroll by a near screenful to read more of the message.
> > +Scroll the current message if the end of it is not visible,
> > +otherwise move to the next message.
> >  
> > -Otherwise, (the end of the current message is already within the
> > -current window), advance to the next open message."
> > +Return `t' if we are at the end of the last message, otherwise
> > +`nil'."
> >(interactive)
> > -  (let* ((end-of-this-message (notmuch-show-message-bottom))
> > -(visible-end-of-this-message (1- end-of-this-message))
> > -(ret nil))
> > -(while (invisible-p visible-end-of-this-message)
> > -  (setq visible-end-of-this-message
> > -   (previous-single-char-property-change visible-end-of-this-message
> > - 'invisible)))
> > -(cond
> > - ;; Ideally we would test `end-of-this-message' against the result
> > - ;; of `window-end', but that doesn't account for the fact that
> > - ;; the end of the message might be hidden.
> > - ((and visible-end-of-this-message
> > -  (> visible-end-of-this-message (window-end)))
> > -  ;; The bottom of this message is not visible - scroll.
> > -  (scroll-up nil))
> > -
> > - ((not (= end-of-this-message (point-max)))
> > -  ;; This is not the last message - move to the next visible one.
> > -  (notmuch-show-next-open-message))
> > -
> > - (t
> > -  ;; This is the last message - change the return value
> > -  (setq ret t)))
> > -ret))
> > +  (cond
> > +   ((eobp)
> > +;; We are at the end of the buffer - move to the next thread.
> > +t)
> > +
> > +   ;; Ideally we would simply do:
> > +   ;; 
> 
> Tailing whitespace.
> 
> > +   ;;  ((> (notmuch-show-message-bottom) (window-end))
> > +   ;; 
> 
> More trailing whitespace.
> 
> > +   ;; here, but that fails if the trailing text in the buffer is
> > +   ;; invisible (`window-end' returns the last _visible_ character,
> > +   ;; which can then be smaller than `notmuch-show-message-bottom').
> > +   ;;
> > +   ;; So we need to find the last visible character of the message. We
> > +   ;; do this by searching backwards from
> > +   ;; `notmuch-show-message-bottom' for changes in the `invisible'
> > +   ;; property until we find a non-invisible character. When we find
> > +   ;; such a character we test to see whether it is visible in the
> > +   ;; window.
> > +   ;;
> > +   ;; Properties change between characters - the return value of
> > +   ;; `previous-single-char-property-change' points to the first
> > +   ;; character _inside_ the region with the `invisible' property
> > +   ;; set. To allow for this we step backwards one character upon
> > +   ;; finding the start of the invisible region.
> > +
> > +   ((> (let ((visible-bottom (notmuch-show-message-bottom)))
> > +(while (invisible-p visible-bottom)
> > +  (setq visible-bottom (max (point-min)
> > +(1- (previous-single-char-property-change
> > + visible-bottom 'invisible)
> > +visible-bottom) (window-end))

Can this (let...) be lifted out of the (cond...)?  IMO it is very
confusing to be doing non-trivial computation in the test portion of a
cond form.

> > +;; The end of this message is not visible - scroll to show more of
> > +;; it.
> > +(scroll-up)
> > +nil)
> > +
> > +   (t
> > +;; All of the current message has been seen - show the start of
> > +;; the next open message.
> > +(notmuch-show-next-open-message)
> > +nil)))
> >  
> >  (defun notmuch-show-advance-and-archive ()
> >"Advance through thread and archive.
> > @@ -1201,44 +1219,40 @@ from each message), kills the buffer, and displays 
> > the nex

[RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[Austin Clements]

Awesome.  This looks significantly cleaner.  I think this is worth
pushing for the comment you added to notmuch-show-advance alone.

This definitely changes the behavior of rewind, but other than one
case I point out below, I think what you have now is much closer to an
inverse of advance.  It would be nice to have tests for rewind (looks
like we don't have any right now), but it would seem counterproductive
to write tests for the current rewind only to rewrite them for this
rewind.

A few comments inline.

Quoth David Edmondson on Dec 23 at  6:41 pm:
> The advance/rewind functions had become complex, which made it hard to
> determine how they are expected to behave. Re-implement them simply in
> order to poll user-experience and expectation.
> ---
> 
> Switched back to using `previous-single-char-property-change' now that
> Aaron explained it. Fix a bug rewinding when the start of the current
> message is visible.
> 
>  emacs/notmuch-show.el |  132 
> +++--
>  1 files changed, 73 insertions(+), 59 deletions(-)
> 
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 46525aa..e914ce1 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -1156,38 +1156,56 @@ Some useful entries are:
>  ;; Commands typically bound to keys.
>  
>  (defun notmuch-show-advance ()
> -  "Advance through thread.
> +  "Advance through the current thread.
>  
> -If the current message in the thread is not yet fully visible,
> -scroll by a near screenful to read more of the message.
> +Scroll the current message if the end of it is not visible,
> +otherwise move to the next message.
>  
> -Otherwise, (the end of the current message is already within the
> -current window), advance to the next open message."
> +Return `t' if we are at the end of the last message, otherwise
> +`nil'."
>(interactive)
> -  (let* ((end-of-this-message (notmuch-show-message-bottom))
> -  (visible-end-of-this-message (1- end-of-this-message))
> -  (ret nil))
> -(while (invisible-p visible-end-of-this-message)
> -  (setq visible-end-of-this-message
> - (previous-single-char-property-change visible-end-of-this-message
> -   'invisible)))
> -(cond
> - ;; Ideally we would test `end-of-this-message' against the result
> - ;; of `window-end', but that doesn't account for the fact that
> - ;; the end of the message might be hidden.
> - ((and visible-end-of-this-message
> -(> visible-end-of-this-message (window-end)))
> -  ;; The bottom of this message is not visible - scroll.
> -  (scroll-up nil))
> -
> - ((not (= end-of-this-message (point-max)))
> -  ;; This is not the last message - move to the next visible one.
> -  (notmuch-show-next-open-message))
> -
> - (t
> -  ;; This is the last message - change the return value
> -  (setq ret t)))
> -ret))
> +  (cond
> +   ((eobp)
> +;; We are at the end of the buffer - move to the next thread.
> +t)
> +
> +   ;; Ideally we would simply do:
> +   ;; 

Tailing whitespace.

> +   ;;((> (notmuch-show-message-bottom) (window-end))
> +   ;; 

More trailing whitespace.

> +   ;; here, but that fails if the trailing text in the buffer is
> +   ;; invisible (`window-end' returns the last _visible_ character,
> +   ;; which can then be smaller than `notmuch-show-message-bottom').
> +   ;;
> +   ;; So we need to find the last visible character of the message. We
> +   ;; do this by searching backwards from
> +   ;; `notmuch-show-message-bottom' for changes in the `invisible'
> +   ;; property until we find a non-invisible character. When we find
> +   ;; such a character we test to see whether it is visible in the
> +   ;; window.
> +   ;;
> +   ;; Properties change between characters - the return value of
> +   ;; `previous-single-char-property-change' points to the first
> +   ;; character _inside_ the region with the `invisible' property
> +   ;; set. To allow for this we step backwards one character upon
> +   ;; finding the start of the invisible region.
> +
> +   ((> (let ((visible-bottom (notmuch-show-message-bottom)))
> +  (while (invisible-p visible-bottom)
> +(setq visible-bottom (max (point-min)
> +  (1- (previous-single-char-property-change
> +   visible-bottom 'invisible)
> +  visible-bottom) (window-end))
> +;; The end of this message is not visible - scroll to show more of
> +;; it.
> +(scroll-up)
> +nil)
> +
> +   (t
> +;; All of the current message has been seen - show the start of
> +;; the next open message.
> +(notmuch-show-next-open-message)
> +nil)))
>  
>  (defun notmuch-show-advance-and-archive ()
>"Advance through thread and archive.
> @@ -1201,44 +1219,40 @@ from each message), kills the buffer, and displays 
> the next
>  thread from the search from which t

Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[Austin Clements]

Awesome.  This looks significantly cleaner.  I think this is worth
pushing for the comment you added to notmuch-show-advance alone.

This definitely changes the behavior of rewind, but other than one
case I point out below, I think what you have now is much closer to an
inverse of advance.  It would be nice to have tests for rewind (looks
like we don't have any right now), but it would seem counterproductive
to write tests for the current rewind only to rewrite them for this
rewind.

A few comments inline.

Quoth David Edmondson on Dec 23 at  6:41 pm:
> The advance/rewind functions had become complex, which made it hard to
> determine how they are expected to behave. Re-implement them simply in
> order to poll user-experience and expectation.
> ---
> 
> Switched back to using `previous-single-char-property-change' now that
> Aaron explained it. Fix a bug rewinding when the start of the current
> message is visible.
> 
>  emacs/notmuch-show.el |  132 
> +++--
>  1 files changed, 73 insertions(+), 59 deletions(-)
> 
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 46525aa..e914ce1 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -1156,38 +1156,56 @@ Some useful entries are:
>  ;; Commands typically bound to keys.
>  
>  (defun notmuch-show-advance ()
> -  "Advance through thread.
> +  "Advance through the current thread.
>  
> -If the current message in the thread is not yet fully visible,
> -scroll by a near screenful to read more of the message.
> +Scroll the current message if the end of it is not visible,
> +otherwise move to the next message.
>  
> -Otherwise, (the end of the current message is already within the
> -current window), advance to the next open message."
> +Return `t' if we are at the end of the last message, otherwise
> +`nil'."
>(interactive)
> -  (let* ((end-of-this-message (notmuch-show-message-bottom))
> -  (visible-end-of-this-message (1- end-of-this-message))
> -  (ret nil))
> -(while (invisible-p visible-end-of-this-message)
> -  (setq visible-end-of-this-message
> - (previous-single-char-property-change visible-end-of-this-message
> -   'invisible)))
> -(cond
> - ;; Ideally we would test `end-of-this-message' against the result
> - ;; of `window-end', but that doesn't account for the fact that
> - ;; the end of the message might be hidden.
> - ((and visible-end-of-this-message
> -(> visible-end-of-this-message (window-end)))
> -  ;; The bottom of this message is not visible - scroll.
> -  (scroll-up nil))
> -
> - ((not (= end-of-this-message (point-max)))
> -  ;; This is not the last message - move to the next visible one.
> -  (notmuch-show-next-open-message))
> -
> - (t
> -  ;; This is the last message - change the return value
> -  (setq ret t)))
> -ret))
> +  (cond
> +   ((eobp)
> +;; We are at the end of the buffer - move to the next thread.
> +t)
> +
> +   ;; Ideally we would simply do:
> +   ;; 

Tailing whitespace.

> +   ;;((> (notmuch-show-message-bottom) (window-end))
> +   ;; 

More trailing whitespace.

> +   ;; here, but that fails if the trailing text in the buffer is
> +   ;; invisible (`window-end' returns the last _visible_ character,
> +   ;; which can then be smaller than `notmuch-show-message-bottom').
> +   ;;
> +   ;; So we need to find the last visible character of the message. We
> +   ;; do this by searching backwards from
> +   ;; `notmuch-show-message-bottom' for changes in the `invisible'
> +   ;; property until we find a non-invisible character. When we find
> +   ;; such a character we test to see whether it is visible in the
> +   ;; window.
> +   ;;
> +   ;; Properties change between characters - the return value of
> +   ;; `previous-single-char-property-change' points to the first
> +   ;; character _inside_ the region with the `invisible' property
> +   ;; set. To allow for this we step backwards one character upon
> +   ;; finding the start of the invisible region.
> +
> +   ((> (let ((visible-bottom (notmuch-show-message-bottom)))
> +  (while (invisible-p visible-bottom)
> +(setq visible-bottom (max (point-min)
> +  (1- (previous-single-char-property-change
> +   visible-bottom 'invisible)
> +  visible-bottom) (window-end))
> +;; The end of this message is not visible - scroll to show more of
> +;; it.
> +(scroll-up)
> +nil)
> +
> +   (t
> +;; All of the current message has been seen - show the start of
> +;; the next open message.
> +(notmuch-show-next-open-message)
> +nil)))
>  
>  (defun notmuch-show-advance-and-archive ()
>"Advance through thread and archive.
> @@ -1201,44 +1219,40 @@ from each message), kills the buffer, and displays 
> the next
>  thread from the search from which t

[RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[Dmitry Kurochkin]

Hi David.

On Fri, 23 Dec 2011 18:41:52 +, David Edmondson  wrote:
> The advance/rewind functions had become complex, which made it hard to
> determine how they are expected to behave. Re-implement them simply in
> order to poll user-experience and expectation.
> ---
> 
> Switched back to using `previous-single-char-property-change' now that
> Aaron explained it. Fix a bug rewinding when the start of the current
> message is visible.
> 

I did not do a proper review.  But here are few comments:

* Revert changes to notmuch-show-advance-and-archive.

* Can we split this in two patches?  One for rewind and another for
  advance.

* Does this patch change the behavior of the functions or is it just
  meant to simplify the code?  If it is the former, it would be really
  nice to have tests for it.

Regards,
  Dmitry

>  emacs/notmuch-show.el |  132 
> +++--
>  1 files changed, 73 insertions(+), 59 deletions(-)
> 
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 46525aa..e914ce1 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -1156,38 +1156,56 @@ Some useful entries are:
>  ;; Commands typically bound to keys.
>  
>  (defun notmuch-show-advance ()
> -  "Advance through thread.
> +  "Advance through the current thread.
>  
> -If the current message in the thread is not yet fully visible,
> -scroll by a near screenful to read more of the message.
> +Scroll the current message if the end of it is not visible,
> +otherwise move to the next message.
>  
> -Otherwise, (the end of the current message is already within the
> -current window), advance to the next open message."
> +Return `t' if we are at the end of the last message, otherwise
> +`nil'."
>(interactive)
> -  (let* ((end-of-this-message (notmuch-show-message-bottom))
> -  (visible-end-of-this-message (1- end-of-this-message))
> -  (ret nil))
> -(while (invisible-p visible-end-of-this-message)
> -  (setq visible-end-of-this-message
> - (previous-single-char-property-change visible-end-of-this-message
> -   'invisible)))
> -(cond
> - ;; Ideally we would test `end-of-this-message' against the result
> - ;; of `window-end', but that doesn't account for the fact that
> - ;; the end of the message might be hidden.
> - ((and visible-end-of-this-message
> -(> visible-end-of-this-message (window-end)))
> -  ;; The bottom of this message is not visible - scroll.
> -  (scroll-up nil))
> -
> - ((not (= end-of-this-message (point-max)))
> -  ;; This is not the last message - move to the next visible one.
> -  (notmuch-show-next-open-message))
> -
> - (t
> -  ;; This is the last message - change the return value
> -  (setq ret t)))
> -ret))
> +  (cond
> +   ((eobp)
> +;; We are at the end of the buffer - move to the next thread.
> +t)
> +
> +   ;; Ideally we would simply do:
> +   ;; 
> +   ;;((> (notmuch-show-message-bottom) (window-end))
> +   ;; 
> +   ;; here, but that fails if the trailing text in the buffer is
> +   ;; invisible (`window-end' returns the last _visible_ character,
> +   ;; which can then be smaller than `notmuch-show-message-bottom').
> +   ;;
> +   ;; So we need to find the last visible character of the message. We
> +   ;; do this by searching backwards from
> +   ;; `notmuch-show-message-bottom' for changes in the `invisible'
> +   ;; property until we find a non-invisible character. When we find
> +   ;; such a character we test to see whether it is visible in the
> +   ;; window.
> +   ;;
> +   ;; Properties change between characters - the return value of
> +   ;; `previous-single-char-property-change' points to the first
> +   ;; character _inside_ the region with the `invisible' property
> +   ;; set. To allow for this we step backwards one character upon
> +   ;; finding the start of the invisible region.
> +
> +   ((> (let ((visible-bottom (notmuch-show-message-bottom)))
> +  (while (invisible-p visible-bottom)
> +(setq visible-bottom (max (point-min)
> +  (1- (previous-single-char-property-change
> +   visible-bottom 'invisible)
> +  visible-bottom) (window-end))
> +;; The end of this message is not visible - scroll to show more of
> +;; it.
> +(scroll-up)
> +nil)
> +
> +   (t
> +;; All of the current message has been seen - show the start of
> +;; the next open message.
> +(notmuch-show-next-open-message)
> +nil)))
>  
>  (defun notmuch-show-advance-and-archive ()
>"Advance through thread and archive.
> @@ -1201,44 +1219,40 @@ from each message), kills the buffer, and displays 
> the next
>  thread from the search from which this thread was originally
>  shown."
>(interactive)
> -  (if (notmuch-show-advance)
> -  (notmuch-show-archive-thread)))
> +  (when (notmuch-show

[RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[David Edmondson]

The advance/rewind functions had become complex, which made it hard to
determine how they are expected to behave. Re-implement them simply in
order to poll user-experience and expectation.
---

Switched back to using `previous-single-char-property-change' now that
Aaron explained it. Fix a bug rewinding when the start of the current
message is visible.

 emacs/notmuch-show.el |  132 +++--
 1 files changed, 73 insertions(+), 59 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 46525aa..e914ce1 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1156,38 +1156,56 @@ Some useful entries are:
 ;; Commands typically bound to keys.

 (defun notmuch-show-advance ()
-  "Advance through thread.
+  "Advance through the current thread.

-If the current message in the thread is not yet fully visible,
-scroll by a near screenful to read more of the message.
+Scroll the current message if the end of it is not visible,
+otherwise move to the next message.

-Otherwise, (the end of the current message is already within the
-current window), advance to the next open message."
+Return `t' if we are at the end of the last message, otherwise
+`nil'."
   (interactive)
-  (let* ((end-of-this-message (notmuch-show-message-bottom))
-(visible-end-of-this-message (1- end-of-this-message))
-(ret nil))
-(while (invisible-p visible-end-of-this-message)
-  (setq visible-end-of-this-message
-   (previous-single-char-property-change visible-end-of-this-message
- 'invisible)))
-(cond
- ;; Ideally we would test `end-of-this-message' against the result
- ;; of `window-end', but that doesn't account for the fact that
- ;; the end of the message might be hidden.
- ((and visible-end-of-this-message
-  (> visible-end-of-this-message (window-end)))
-  ;; The bottom of this message is not visible - scroll.
-  (scroll-up nil))
-
- ((not (= end-of-this-message (point-max)))
-  ;; This is not the last message - move to the next visible one.
-  (notmuch-show-next-open-message))
-
- (t
-  ;; This is the last message - change the return value
-  (setq ret t)))
-ret))
+  (cond
+   ((eobp)
+;; We are at the end of the buffer - move to the next thread.
+t)
+
+   ;; Ideally we would simply do:
+   ;; 
+   ;;  ((> (notmuch-show-message-bottom) (window-end))
+   ;; 
+   ;; here, but that fails if the trailing text in the buffer is
+   ;; invisible (`window-end' returns the last _visible_ character,
+   ;; which can then be smaller than `notmuch-show-message-bottom').
+   ;;
+   ;; So we need to find the last visible character of the message. We
+   ;; do this by searching backwards from
+   ;; `notmuch-show-message-bottom' for changes in the `invisible'
+   ;; property until we find a non-invisible character. When we find
+   ;; such a character we test to see whether it is visible in the
+   ;; window.
+   ;;
+   ;; Properties change between characters - the return value of
+   ;; `previous-single-char-property-change' points to the first
+   ;; character _inside_ the region with the `invisible' property
+   ;; set. To allow for this we step backwards one character upon
+   ;; finding the start of the invisible region.
+
+   ((> (let ((visible-bottom (notmuch-show-message-bottom)))
+(while (invisible-p visible-bottom)
+  (setq visible-bottom (max (point-min)
+(1- (previous-single-char-property-change
+ visible-bottom 'invisible)
+visible-bottom) (window-end))
+;; The end of this message is not visible - scroll to show more of
+;; it.
+(scroll-up)
+nil)
+
+   (t
+;; All of the current message has been seen - show the start of
+;; the next open message.
+(notmuch-show-next-open-message)
+nil)))

 (defun notmuch-show-advance-and-archive ()
   "Advance through thread and archive.
@@ -1201,44 +1219,40 @@ from each message), kills the buffer, and displays the 
next
 thread from the search from which this thread was originally
 shown."
   (interactive)
-  (if (notmuch-show-advance)
-  (notmuch-show-archive-thread)))
+  (when (notmuch-show-advance)
+(notmuch-show-archive-thread)))

 (defun notmuch-show-rewind ()
-  "Backup through the thread, (reverse scrolling compared to 
\\[notmuch-show-advance-and-archive]).
+  "Move backwards through a thread, the counterpart to 
\\[notmuch-show-advance-and-archive]."

-Specifically, if the beginning of the previous email is fewer
-than `window-height' lines from the current point, move to it
-just like `notmuch-show-previous-message'.
-
-Otherwise, just scroll down a screenful of the current message.
-
-This command does not modify any message tags, (it does not undo
-any effects from previous calls to
-`notmuch-show-advance-and-archive'."
   (interac

Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[Dmitry Kurochkin]

Hi David.

On Fri, 23 Dec 2011 18:41:52 +, David Edmondson  wrote:
> The advance/rewind functions had become complex, which made it hard to
> determine how they are expected to behave. Re-implement them simply in
> order to poll user-experience and expectation.
> ---
> 
> Switched back to using `previous-single-char-property-change' now that
> Aaron explained it. Fix a bug rewinding when the start of the current
> message is visible.
> 

I did not do a proper review.  But here are few comments:

* Revert changes to notmuch-show-advance-and-archive.

* Can we split this in two patches?  One for rewind and another for
  advance.

* Does this patch change the behavior of the functions or is it just
  meant to simplify the code?  If it is the former, it would be really
  nice to have tests for it.

Regards,
  Dmitry

>  emacs/notmuch-show.el |  132 
> +++--
>  1 files changed, 73 insertions(+), 59 deletions(-)
> 
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 46525aa..e914ce1 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -1156,38 +1156,56 @@ Some useful entries are:
>  ;; Commands typically bound to keys.
>  
>  (defun notmuch-show-advance ()
> -  "Advance through thread.
> +  "Advance through the current thread.
>  
> -If the current message in the thread is not yet fully visible,
> -scroll by a near screenful to read more of the message.
> +Scroll the current message if the end of it is not visible,
> +otherwise move to the next message.
>  
> -Otherwise, (the end of the current message is already within the
> -current window), advance to the next open message."
> +Return `t' if we are at the end of the last message, otherwise
> +`nil'."
>(interactive)
> -  (let* ((end-of-this-message (notmuch-show-message-bottom))
> -  (visible-end-of-this-message (1- end-of-this-message))
> -  (ret nil))
> -(while (invisible-p visible-end-of-this-message)
> -  (setq visible-end-of-this-message
> - (previous-single-char-property-change visible-end-of-this-message
> -   'invisible)))
> -(cond
> - ;; Ideally we would test `end-of-this-message' against the result
> - ;; of `window-end', but that doesn't account for the fact that
> - ;; the end of the message might be hidden.
> - ((and visible-end-of-this-message
> -(> visible-end-of-this-message (window-end)))
> -  ;; The bottom of this message is not visible - scroll.
> -  (scroll-up nil))
> -
> - ((not (= end-of-this-message (point-max)))
> -  ;; This is not the last message - move to the next visible one.
> -  (notmuch-show-next-open-message))
> -
> - (t
> -  ;; This is the last message - change the return value
> -  (setq ret t)))
> -ret))
> +  (cond
> +   ((eobp)
> +;; We are at the end of the buffer - move to the next thread.
> +t)
> +
> +   ;; Ideally we would simply do:
> +   ;; 
> +   ;;((> (notmuch-show-message-bottom) (window-end))
> +   ;; 
> +   ;; here, but that fails if the trailing text in the buffer is
> +   ;; invisible (`window-end' returns the last _visible_ character,
> +   ;; which can then be smaller than `notmuch-show-message-bottom').
> +   ;;
> +   ;; So we need to find the last visible character of the message. We
> +   ;; do this by searching backwards from
> +   ;; `notmuch-show-message-bottom' for changes in the `invisible'
> +   ;; property until we find a non-invisible character. When we find
> +   ;; such a character we test to see whether it is visible in the
> +   ;; window.
> +   ;;
> +   ;; Properties change between characters - the return value of
> +   ;; `previous-single-char-property-change' points to the first
> +   ;; character _inside_ the region with the `invisible' property
> +   ;; set. To allow for this we step backwards one character upon
> +   ;; finding the start of the invisible region.
> +
> +   ((> (let ((visible-bottom (notmuch-show-message-bottom)))
> +  (while (invisible-p visible-bottom)
> +(setq visible-bottom (max (point-min)
> +  (1- (previous-single-char-property-change
> +   visible-bottom 'invisible)
> +  visible-bottom) (window-end))
> +;; The end of this message is not visible - scroll to show more of
> +;; it.
> +(scroll-up)
> +nil)
> +
> +   (t
> +;; All of the current message has been seen - show the start of
> +;; the next open message.
> +(notmuch-show-next-open-message)
> +nil)))
>  
>  (defun notmuch-show-advance-and-archive ()
>"Advance through thread and archive.
> @@ -1201,44 +1219,40 @@ from each message), kills the buffer, and displays 
> the next
>  thread from the search from which this thread was originally
>  shown."
>(interactive)
> -  (if (notmuch-show-advance)
> -  (notmuch-show-archive-thread)))
> +  (when (notmuch-show

[RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[David Edmondson]

The advance/rewind functions had become complex, which made it hard to
determine how they are expected to behave. Re-implement them simply in
order to poll user-experience and expectation.
---

Switched back to using `previous-single-char-property-change' now that
Aaron explained it. Fix a bug rewinding when the start of the current
message is visible.

 emacs/notmuch-show.el |  132 +++--
 1 files changed, 73 insertions(+), 59 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 46525aa..e914ce1 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1156,38 +1156,56 @@ Some useful entries are:
 ;; Commands typically bound to keys.
 
 (defun notmuch-show-advance ()
-  "Advance through thread.
+  "Advance through the current thread.
 
-If the current message in the thread is not yet fully visible,
-scroll by a near screenful to read more of the message.
+Scroll the current message if the end of it is not visible,
+otherwise move to the next message.
 
-Otherwise, (the end of the current message is already within the
-current window), advance to the next open message."
+Return `t' if we are at the end of the last message, otherwise
+`nil'."
   (interactive)
-  (let* ((end-of-this-message (notmuch-show-message-bottom))
-(visible-end-of-this-message (1- end-of-this-message))
-(ret nil))
-(while (invisible-p visible-end-of-this-message)
-  (setq visible-end-of-this-message
-   (previous-single-char-property-change visible-end-of-this-message
- 'invisible)))
-(cond
- ;; Ideally we would test `end-of-this-message' against the result
- ;; of `window-end', but that doesn't account for the fact that
- ;; the end of the message might be hidden.
- ((and visible-end-of-this-message
-  (> visible-end-of-this-message (window-end)))
-  ;; The bottom of this message is not visible - scroll.
-  (scroll-up nil))
-
- ((not (= end-of-this-message (point-max)))
-  ;; This is not the last message - move to the next visible one.
-  (notmuch-show-next-open-message))
-
- (t
-  ;; This is the last message - change the return value
-  (setq ret t)))
-ret))
+  (cond
+   ((eobp)
+;; We are at the end of the buffer - move to the next thread.
+t)
+
+   ;; Ideally we would simply do:
+   ;; 
+   ;;  ((> (notmuch-show-message-bottom) (window-end))
+   ;; 
+   ;; here, but that fails if the trailing text in the buffer is
+   ;; invisible (`window-end' returns the last _visible_ character,
+   ;; which can then be smaller than `notmuch-show-message-bottom').
+   ;;
+   ;; So we need to find the last visible character of the message. We
+   ;; do this by searching backwards from
+   ;; `notmuch-show-message-bottom' for changes in the `invisible'
+   ;; property until we find a non-invisible character. When we find
+   ;; such a character we test to see whether it is visible in the
+   ;; window.
+   ;;
+   ;; Properties change between characters - the return value of
+   ;; `previous-single-char-property-change' points to the first
+   ;; character _inside_ the region with the `invisible' property
+   ;; set. To allow for this we step backwards one character upon
+   ;; finding the start of the invisible region.
+
+   ((> (let ((visible-bottom (notmuch-show-message-bottom)))
+(while (invisible-p visible-bottom)
+  (setq visible-bottom (max (point-min)
+(1- (previous-single-char-property-change
+ visible-bottom 'invisible)
+visible-bottom) (window-end))
+;; The end of this message is not visible - scroll to show more of
+;; it.
+(scroll-up)
+nil)
+
+   (t
+;; All of the current message has been seen - show the start of
+;; the next open message.
+(notmuch-show-next-open-message)
+nil)))
 
 (defun notmuch-show-advance-and-archive ()
   "Advance through thread and archive.
@@ -1201,44 +1219,40 @@ from each message), kills the buffer, and displays the 
next
 thread from the search from which this thread was originally
 shown."
   (interactive)
-  (if (notmuch-show-advance)
-  (notmuch-show-archive-thread)))
+  (when (notmuch-show-advance)
+(notmuch-show-archive-thread)))
 
 (defun notmuch-show-rewind ()
-  "Backup through the thread, (reverse scrolling compared to 
\\[notmuch-show-advance-and-archive]).
+  "Move backwards through a thread, the counterpart to 
\\[notmuch-show-advance-and-archive]."
 
-Specifically, if the beginning of the previous email is fewer
-than `window-height' lines from the current point, move to it
-just like `notmuch-show-previous-message'.
-
-Otherwise, just scroll down a screenful of the current message.
-
-This command does not modify any message tags, (it does not undo
-any effects from previous calls to
-`notmuch-show-advance-and-archive'."
   (i