[PATCH] emacs: Disambiguate point placement after hiding message

[PATCH] emacs: Disambiguate point placement after hiding message

[Austin Clements]

Currently, if point is in the middle of a message when the user
collapses it, Emacs then displays the cursor on the header line of the
next message, even though point is still on the collapsed message and
even though, if you explicitly move point to the same visual location,
it will be on the next message.  As a result, following actions like
re-expanding the message or modifying tags apply to the collapsed
message, even though, visually, it looks like they will apply to the
message following the collapsed message.

This patch addresses this by explicitly moving point when a message is
collapsed so it is visually unambiguous that the point is still on the
collapsed message.
---
 emacs/notmuch-show.el |   18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 5751d98..94b4178 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1363,7 +1363,23 @@ effects."
 ;; components.
 
 (defun notmuch-show-message-visible (props visible-p)
-  (overlay-put (plist-get props :message-overlay) 'invisible (not visible-p))
+  (let ((ov (plist-get props :message-overlay)))
+    (overlay-put ov 'invisible (not visible-p))
+    (when (not visible-p)
+      ;; If point was contained in the overlay, move it to a sensible
+      ;; spot that is visible and still on the same message.
+      ;; Strangely, the Emacs event loop doesn't move the point out of
+      ;; the invisible region for us like it normally does (perhaps
+      ;; because it doesn't know which way to go), so if we don't do
+      ;; this, it's visually ambiguous which message an action will
+      ;; apply to.
+      (let ((start (overlay-start ov))
+	    (end (overlay-end ov)))
+	(dolist (win (get-buffer-window-list nil nil t))
+	  (with-selected-window win
+	    (when (and (<= start (point)) (< (point) end))
+	      (goto-char (1- start))
+	      (beginning-of-visual-line)))))))
   (notmuch-show-set-prop :message-visible visible-p props))
 
 (defun notmuch-show-headers-visible (props visible-p)
-- 
1.7.10.4

[PATCH v2] emacs: Disambiguate point placement after hiding message

[Austin Clements]

Currently, if point is in the middle of a message when the user
collapses it, Emacs then displays the cursor on the header line of the
next message, even though point is still on the collapsed message and
even though, if you explicitly move point to the same visual location,
it will be on the next message.  As a result, following actions like
re-expanding the message or modifying tags apply to the collapsed
message, even though, visually, it looks like they will apply to the
message following the collapsed message.

This patch addresses this by explicitly moving point when a message is
collapsed so it is visually unambiguous that the point is still on the
collapsed message.
---

v2 should fix the strange behavior observed in v1.  The added code is
essentially identical to v1, but v2 adds it to
notmuch-show-toggle-message---which is only used
interactively---rather than the core notmuch-show-message-visible
function.

 emacs/notmuch-show.el |   28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 5751d98..6ab926c 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1789,12 +1789,30 @@ See `notmuch-tag' for information on the format of TAG-CHANGES."
   (force-window-update))
 
 (defun notmuch-show-toggle-message ()
-  "Toggle the visibility of the current message."
+  "Toggle the visibility of the current message.
+
+If this hides the current message, it will also move point to
+make it obvious it's still on the current message."
   (interactive)
-  (let ((props (notmuch-show-get-message-properties)))
-    (notmuch-show-message-visible
-     props
-     (not (plist-get props :message-visible))))
+  (let* ((props (notmuch-show-get-message-properties))
+	 (visible-p (not (plist-get props :message-visible))))
+    (notmuch-show-message-visible props visible-p)
+    (when (not visible-p)
+      (let ((ov (plist-get props :message-overlay)))
+	;; If point was contained in the overlay, move it to a
+	;; sensible spot that is visible and still on the same
+	;; message.  Strangely, the Emacs event loop doesn't move the
+	;; point out of the invisible region for us like it normally
+	;; does (perhaps because it doesn't know which way to go), so
+	;; if we don't do this, it's visually ambiguous which message
+	;; an action will apply to.
+	(let ((start (overlay-start ov))
+	      (end (overlay-end ov)))
+	  (dolist (win (get-buffer-window-list nil nil t))
+	    (with-selected-window win
+	      (when (and (<= start (point)) (< (point) end))
+		(goto-char (1- start))
+		(beginning-of-visual-line))))))))
   (force-window-update))
 
 (defun notmuch-show-open-or-close-all ()
-- 
1.7.10.4

Re: [PATCH v2] emacs: Disambiguate point placement after hiding message

[Tomi Ollila]

On Wed, Jan 09 2013, Austin Clements <amdragon@MIT.EDU> wrote:

> Currently, if point is in the middle of a message when the user
> collapses it, Emacs then displays the cursor on the header line of the
> next message, even though point is still on the collapsed message and
> even though, if you explicitly move point to the same visual location,
> it will be on the next message.  As a result, following actions like
> re-expanding the message or modifying tags apply to the collapsed
> message, even though, visually, it looks like they will apply to the
> message following the collapsed message.
>
> This patch addresses this by explicitly moving point when a message is
> collapsed so it is visually unambiguous that the point is still on the
> collapsed message.
> ---
>
> v2 should fix the strange behavior observed in v1.  The added code is
> essentially identical to v1, but v2 adds it to
> notmuch-show-toggle-message---which is only used
> interactively---rather than the core notmuch-show-message-visible
> function.
>
>  emacs/notmuch-show.el |   28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 5751d98..6ab926c 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -1789,12 +1789,30 @@ See `notmuch-tag' for information on the format of TAG-CHANGES."
>    (force-window-update))
>  
>  (defun notmuch-show-toggle-message ()
> -  "Toggle the visibility of the current message."
> +  "Toggle the visibility of the current message.
> +
> +If this hides the current message, it will also move point to
> +make it obvious it's still on the current message."
>    (interactive)
> -  (let ((props (notmuch-show-get-message-properties)))
> -    (notmuch-show-message-visible
> -     props
> -     (not (plist-get props :message-visible))))
> +  (let* ((props (notmuch-show-get-message-properties))
> +	 (visible-p (not (plist-get props :message-visible))))
> +    (notmuch-show-message-visible props visible-p)
> +    (when (not visible-p)
> +      (let ((ov (plist-get props :message-overlay)))
> +	;; If point was contained in the overlay, move it to a
> +	;; sensible spot that is visible and still on the same
> +	;; message.  Strangely, the Emacs event loop doesn't move the
> +	;; point out of the invisible region for us like it normally
> +	;; does (perhaps because it doesn't know which way to go), so
> +	;; if we don't do this, it's visually ambiguous which message
> +	;; an action will apply to.
> +	(let ((start (overlay-start ov))
> +	      (end (overlay-end ov)))
> +	  (dolist (win (get-buffer-window-list nil nil t))
> +	    (with-selected-window win
> +	      (when (and (<= start (point)) (< (point) end))
> +		(goto-char (1- start))
> +		(beginning-of-visual-line))))))))

Soo. the problem with this is still the behaviour of 
(beginning-of-visual-line), which "leaks" to previous message
if (point) is in the first header line. Interestingly 
(beginning-of-line) does not -- and this is supposed to move more
than beginning-of-visual-line...

But this and the defadvice in
id:1357855176-31653-1-git-send-email-amdragon@mit.edu
could fix this problem -- if the "sketchy" approach were used...

Austin: have you got further with the "alternate" approach
you mentioned in the other mail ?

In the meanwhile I played a bit how those overlays are 
positioned -- basically moved those to one character position
closer to the beginning of buffer -- so that overlays start
with "\n" and end just before "\n". My naive attempts just
to move brought some interesting side effects (line counts
in button change, header line coloring doesn't go to the
end of file and cursor is sometimes positioned interesting
-- but the cursor no longer leak to previous (or next)
message.

For reference, The changes I made attached. I don't bother
to make proper patch until/unless we know this is definite
way to proceed (and this definitely have some implementation
issues, too)...

From bd3571c578aa45a23a120fc82b89f7e1649617fd Mon Sep 17 00:00:00 2001
From: Tomi Ollila <tomi.ollila@iki.fi>
Date: Wed, 23 Jan 2013 11:34:20 +0300
Subject: [PATCH] these email headers copied just to make git am happy

---
diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 1864dd1..7ee6d1d 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -870,7 +870,7 @@ message at DEPTH in the current thread."
 (defun notmuch-show-create-part-overlays (msg beg end hide)
   "Add an overlay to the part between BEG and END"
   (let* ((button (button-at beg))
-	 (part-beg (and button (1+ (button-end button)))))
+	 (part-beg (and button (button-end button))))
 
     ;; If the part contains no text we do not make it toggleable. We
     ;; also need to check that the button is a genuine part button not
@@ -898,7 +898,7 @@ If HIDE is non-nil then initially hide this part."
     ;; Ensure that the part ends with a carriage return.
     (unless (bolp)
       (insert "\n"))
-    (notmuch-show-create-part-overlays msg beg (point) hide)))
+    (notmuch-show-create-part-overlays msg beg (1- (point)) hide)))
 
 (defun notmuch-show-insert-body (msg body depth)
   "Insert the body BODY at depth DEPTH in the current thread."
@@ -946,8 +946,8 @@ If HIDE is non-nil then initially hide this part."
       (when (not (string= notmuch-show-previous-subject
 			  bare-subject))
 	(forward-line 1))
-      (setq headers-start (point-marker)))
-    (setq headers-end (point-marker))
+      (setq headers-start (copy-marker (1- (point)))))
+    (setq headers-end (copy-marker (1- (point))))
 
     (setq notmuch-show-previous-subject bare-subject)
 
@@ -958,7 +958,7 @@ If HIDE is non-nil then initially hide this part."
     ;; Ensure that the body ends with a newline.
     (unless (bolp)
       (insert "\n"))
-    (setq content-end (point-marker))
+    (setq content-end (copy-marker (1- (point))))
 
     ;; Indent according to the depth in the thread.
     (if notmuch-show-indent-content
diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
index d6db4fa..49e2dde 100644
--- a/emacs/notmuch-wash.el
+++ b/emacs/notmuch-wash.el
@@ -167,11 +167,12 @@ that PREFIX should not include a newline."
       (if prefix
 	  (insert-before-markers prefix))
       (let ((button-beg (point)))
-	(insert-before-markers (notmuch-wash-button-label overlay) "\n")
-	(let ((button (make-button button-beg (1- (point))
+	(insert-before-markers (notmuch-wash-button-label overlay))
+	(let ((button (make-button button-beg (point)
 				   'overlay overlay
 				   :type button-type)))
-	  (overlay-put overlay 'notmuch-wash-button button))))))
+	  (overlay-put overlay 'notmuch-wash-button button))
+	(insert "\n"))))) ;; after marker(s)
 
 (defun notmuch-wash-excerpt-citations (msg depth)
   "Excerpt citations and up to one signature."
@@ -183,7 +184,7 @@ that PREFIX should not include a newline."
 	     (msg-end (point-max))
 	     (msg-lines (count-lines msg-start msg-end)))
 	(notmuch-wash-region-to-button
-	 msg msg-start msg-end "original")))
+	 msg msg-start (1- msg-end) "original")))
   (while (and (< (point) (point-max))
 	      (re-search-forward notmuch-wash-citation-regexp nil t))
     (let* ((cite-start (match-beginning 0))
@@ -199,7 +200,7 @@ that PREFIX should not include a newline."
 	  (goto-char cite-end)
 	  (forward-line (- notmuch-wash-citation-lines-suffix))
 	  (notmuch-wash-region-to-button
-	   msg hidden-start (point-marker)
+	   msg hidden-start (copy-marker (1- (point)))
 	   "citation")))))
   (if (and (not (eobp))
 	   (re-search-forward notmuch-wash-signature-regexp nil t))
@@ -207,10 +208,8 @@ that PREFIX should not include a newline."
 	     (sig-end (match-end 0))
 	     (sig-lines (count-lines sig-start (point-max))))
 	(if (<= sig-lines notmuch-wash-signature-lines-max)
-	    (let ((sig-start-marker (make-marker))
-		  (sig-end-marker (make-marker)))
-	      (set-marker sig-start-marker sig-start)
-	      (set-marker sig-end-marker (point-max))
+	    (let ((sig-start-marker (copy-marker sig-start))
+		  (sig-end-marker (copy-marker (1- (point-max)))))
 	      (overlay-put (make-overlay sig-start-marker sig-end-marker) 'face 'message-cited-text)
 	      (notmuch-wash-region-to-button
 	       msg sig-start-marker sig-end-marker