Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.
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
Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.
On Mon, 26 Dec 2011 10:49:46 +, David Edmondson d...@dme.org 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
Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.
On Wed, 28 Dec 2011 15:22:45 +, David Edmondson d...@dme.org 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.
On Wed, 28 Dec 2011 13:04:02 -0500, Aaron Ecay aarone...@gmail.com 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
Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.
On Fri, 23 Dec 2011 23:01:33 +0400, Dmitry Kurochkin dmitry.kuroch...@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) * 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
Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.
On Sat, 24 Dec 2011 20:06:35 -0500, Austin Clements amdra...@mit.edu 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.
On Sun, 25 Dec 2011 23:11:27 -0500, Aaron Ecay aarone...@gmail.com 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.
Hi David. On Mon, 26 Dec 2011 10:46:13 +, David Edmondson d...@dme.org wrote: On Fri, 23 Dec 2011 23:01:33 +0400, Dmitry Kurochkin dmitry.kuroch...@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 ___ 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.
On Mon, 26 Dec 2011 22:00:21 +, David Edmondson d...@dme.org 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
Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.
On Mon, 26 Dec 2011 14:24:11 -0800, Jameson Graef Rollins jroll...@finestructure.net wrote: On Mon, 26 Dec 2011 22:00:21 +, David Edmondson d...@dme.org 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
Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.
On Sat, 24 Dec 2011 20:06:35 -0500, Austin Clements amdra...@mit.edu 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 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) +
Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.
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 this thread was originally shown. (interactive) - (if (notmuch-show-advance) -
Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.
Hi David. On Fri, 23 Dec 2011 18:41:52 +, David Edmondson d...@dme.org 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-advance) +(notmuch-show-archive-thread))) (defun notmuch-show-rewind () - Backup through the
