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

[Xavier Maillard]

On Thu, 22 Dec 2011 09:29:55 -0500, Aaron Ecay  wrote:

> Would the problem you had with previous-s-c-prop-change be fixed by the
> patch to the original function I sent in the thread starting at
> id:"m2y5u5cykp.fsf at kcals.intra.maillard.im" ?

AFAIK it does.

/Xavier

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

[David Edmondson]

On Thu, 22 Dec 2011 09:29:55 -0500, Aaron Ecay  wrote:
> Would the problem you had with previous-s-c-prop-change be fixed by the
> patch to the original function I sent in the thread starting at
> id:"m2y5u5cykp.fsf at kcals.intra.maillard.im" ?

I think so, yes. Re-writing the new version I posted to use
`previous-single-char-property-value' in the way that you describe:

   ((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.
(scroll-up)
nil)

seems to work in quick testing.

I'd vote to integrate your change if it seems correct to everyone else,
then I'll argue for a re-write separately.
-- 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 v2] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[Xavier Maillard]

On Thu, 22 Dec 2011 09:29:55 -0500, Aaron Ecay  wrote:

> Would the problem you had with previous-s-c-prop-change be fixed by the
> patch to the original function I sent in the thread starting at
> id:"m2y5u5cykp@kcals.intra.maillard.im" ?

AFAIK it does.

/Xavier
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[RFC][PATCH v2] 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 who they are expected to behave. Re-implement them simply in
order to poll user-experience and expectation.
---

Re-introduce the detection of invisible trailers. Using
`previous-single-char-property-change' just didn't work in my testing,
so back to the original approach. Dmitry: can you explain why you
needed to change it?

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

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 46525aa..258814e 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1156,38 +1156,51 @@ 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)
+;; If we are at the end of the buffer then 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 checking the invisibility of the
+   ;; characters from `notmuch-show-message-bottom' towards the start
+   ;; of the message. When we find a non-invisible character, we test
+   ;; to see whether it is visible in the window (i.e. less than
+   ;; `window-end').
+
+   ((let* ((visible-bottom (notmuch-show-message-bottom))
+  (visible-bottom 
+   (save-excursion
+ (goto-char visible-bottom)
+ (while (invisible-p (point))
+   (backward-char))
+ (point
+  (> visible-bottom (window-end)))
+;; The end of this message is not visible - scroll.
+(scroll-up)
+nil)
+
+   (t
+;; Show the start of the next message.
+(notmuch-show-next-open-message)
+nil)))

 (defun notmuch-show-advance-and-archive ()
   "Advance through thread and archive.
@@ -1201,44 +1214,39 @@ 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'."
   (interactive)
-  (let ((start-of-message (notmuch-show-message-top))
-   (start-of-window (window-start)))
+  (let ((start-of-message (notmuch-show-message-top)))
 (cond
-  ;; Either this message is properly aligned with the start of the
-  ;; window or the start of this message is not visible on the
-  ;; screen - scroll.
- ((or (= start-of-message star

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

[Aaron Ecay]

David,

Would the problem you had with previous-s-c-prop-change be fixed by the
patch to the original function I sent in the thread starting at
id:"m2y5u5cykp.fsf at kcals.intra.maillard.im" ?

-- 
Aaron Ecay

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

[David Edmondson]

On Thu, 22 Dec 2011 09:29:55 -0500, Aaron Ecay  wrote:
> Would the problem you had with previous-s-c-prop-change be fixed by the
> patch to the original function I sent in the thread starting at
> id:"m2y5u5cykp@kcals.intra.maillard.im" ?

I think so, yes. Re-writing the new version I posted to use
`previous-single-char-property-value' in the way that you describe:

   ((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.
(scroll-up)
nil)

seems to work in quick testing.

I'd vote to integrate your change if it seems correct to everyone else,
then I'll argue for a re-write separately.


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

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

[Aaron Ecay]

David,

Would the problem you had with previous-s-c-prop-change be fixed by the
patch to the original function I sent in the thread starting at
id:"m2y5u5cykp@kcals.intra.maillard.im" ?

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

[RFC][PATCH v2] 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 who they are expected to behave. Re-implement them simply in
order to poll user-experience and expectation.
---

Re-introduce the detection of invisible trailers. Using
`previous-single-char-property-change' just didn't work in my testing,
so back to the original approach. Dmitry: can you explain why you
needed to change it?

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

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 46525aa..258814e 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1156,38 +1156,51 @@ 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)
+;; If we are at the end of the buffer then 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 checking the invisibility of the
+   ;; characters from `notmuch-show-message-bottom' towards the start
+   ;; of the message. When we find a non-invisible character, we test
+   ;; to see whether it is visible in the window (i.e. less than
+   ;; `window-end').
+
+   ((let* ((visible-bottom (notmuch-show-message-bottom))
+  (visible-bottom 
+   (save-excursion
+ (goto-char visible-bottom)
+ (while (invisible-p (point))
+   (backward-char))
+ (point
+  (> visible-bottom (window-end)))
+;; The end of this message is not visible - scroll.
+(scroll-up)
+nil)
+
+   (t
+;; Show the start of the next message.
+(notmuch-show-next-open-message)
+nil)))
 
 (defun notmuch-show-advance-and-archive ()
   "Advance through thread and archive.
@@ -1201,44 +1214,39 @@ 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'."
   (interactive)
-  (let ((start-of-message (notmuch-show-message-top))
-   (start-of-window (window-start)))
+  (let ((start-of-message (notmuch-show-message-top)))
 (cond
-  ;; Either this message is properly aligned with the start of the
-  ;; window or the start of this message is not visible on the
-  ;; screen - scroll.
- ((or (= start-of-messag