bug#19102: 24.4; outline-move-subtree-up/down error at last and second-last subtree

[Stephen Berman <stephen.berman@gmx.net>]

[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]

On Wed, 26 Nov 2014 10:54:19 -0500 Stefan Monnier <monnier@IRO.UMontreal.CA> wrote:

>> Oops, you commented on the last version of my patch, which, however, I
>> had already considered superseded by the last version of the command
>> subsequently posted by Paul Rankin, which in effect already addressed
>> your concerns.  Rather than repost the diff for the latter, I'm
>> appending the two corrected versions of outline-move-subtree-down to
>> facilitate comparison and deciding which to use.
>
> Please just send the diff that (you think) should be applied.
> Or two diffs to apply in sequence if you want to distinguish your
> changes from Paul's.

Ok, I've done the latter, appended below.  The first change just fixes
the bug, incorporating your simplification of my patch for when to move
forward and when to add a newline.  The second change is Paul's
refactoring of the code to avoid setq's of let-bound variables, which
I think make the code cleaner and more elegant (the version he posted
also fixed the bug, but I prefer your simplification).

>> The only thing I added to both is a condition for triggering the
>> user-error, since the message doesn't seem appropriate when you try to
>> move a subtree down at eob or up at bob (if you don't think it's worth
>> avoiding the message there, I'll remove the condition).
>
> I don't see why we should avoid the error in those cases: AFAICT, we
> can't do what the user asked, so we should signal an error.

Ok, I removed that part.

Steve Berman


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Fix for bug#19102 --]
[-- Type: text/x-patch, Size: 1572 bytes --]

diff --git a/lisp/outline.el b/lisp/outline.el
index c7cad31..61ee7ff 100644
--- a/lisp/outline.el
+++ b/lisp/outline.el
@@ -649,27 +649,32 @@ the match data is set appropriately."
 		   'outline-get-last-sibling))
 	(ins-point (make-marker))
 	(cnt (abs arg))
+	;; Make sure we can move forward to find the end of the
+	;; subtree and the insertion point.
+	(maybe-forward-char (lambda ()
+			      (if (eq (char-after) ?\n) (forward-char 1)
+				(if (and (eobp) (not (bolp))) (insert "\n")))))
 	beg end folded)
-    ;; Select the tree
+    ;; Select the tree.
     (outline-back-to-heading)
     (setq beg (point))
     (save-match-data
       (save-excursion (outline-end-of-heading)
 		      (setq folded (outline-invisible-p)))
       (outline-end-of-subtree))
-    (if (= (char-after) ?\n) (forward-char 1))
+    (funcall maybe-forward-char)
     (setq end (point))
-    ;; Find insertion point, with error handling
+    ;; Find insertion point, with error handling.
     (goto-char beg)
     (while (> cnt 0)
       (or (funcall movfunc)
 	  (progn (goto-char beg)
-		 (error "Cannot move past superior level")))
+		 (user-error "Cannot move past superior level")))
       (setq cnt (1- cnt)))
     (if (> arg 0)
-	;; Moving forward - still need to move over subtree
+	;; Moving forward - still need to move over subtree.
 	(progn (outline-end-of-subtree)
-	       (if (= (char-after) ?\n) (forward-char 1))))
+	       (funcall maybe-forward-char)))
     (move-marker ins-point (point))
     (insert (delete-and-extract-region beg end))
     (goto-char ins-point)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Refactoring patch --]
[-- Type: text/x-patch, Size: 2218 bytes --]

diff --git a/lisp/outline.el b/lisp/outline.el
index 61ee7ff..bb56341 100644
--- a/lisp/outline.el
+++ b/lisp/outline.el
@@ -645,25 +645,25 @@ the match data is set appropriately."
 (defun outline-move-subtree-down (&optional arg)
   "Move the current subtree down past ARG headlines of the same level."
   (interactive "p")
-  (let ((movfunc (if (> arg 0) 'outline-get-next-sibling
-		   'outline-get-last-sibling))
-	(ins-point (make-marker))
-	(cnt (abs arg))
-	;; Make sure we can move forward to find the end of the
-	;; subtree and the insertion point.
-	(maybe-forward-char (lambda ()
-			      (if (eq (char-after) ?\n) (forward-char 1)
-				(if (and (eobp) (not (bolp))) (insert "\n")))))
-	beg end folded)
-    ;; Select the tree.
-    (outline-back-to-heading)
-    (setq beg (point))
-    (save-match-data
-      (save-excursion (outline-end-of-heading)
-		      (setq folded (outline-invisible-p)))
-      (outline-end-of-subtree))
-    (funcall maybe-forward-char)
-    (setq end (point))
+  (outline-back-to-heading)
+  (let* ((movfunc (if (> arg 0) 'outline-get-next-sibling
+		    'outline-get-last-sibling))
+	 ;; Find the end of the subtree to be moved as well as the point to
+	 ;; move it to, adding a newline if necessary, to ensure these points
+	 ;; are at bol on the line below the subtree.
+         (end-point-func (lambda ()
+			   (outline-end-of-subtree)
+			   (if (eq (char-after) ?\n) (forward-char 1)
+				(if (and (eobp) (not (bolp))) (insert "\n")))
+			   (point)))
+         (beg (point))
+         (folded (save-match-data
+		   (outline-end-of-heading)
+		   (outline-invisible-p)))
+         (end (save-match-data
+		(funcall end-point-func)))
+         (ins-point (make-marker))
+         (cnt (abs arg)))
     ;; Find insertion point, with error handling.
     (goto-char beg)
     (while (> cnt 0)
@@ -673,8 +673,7 @@ the match data is set appropriately."
       (setq cnt (1- cnt)))
     (if (> arg 0)
 	;; Moving forward - still need to move over subtree.
-	(progn (outline-end-of-subtree)
-	       (funcall maybe-forward-char)))
+	(funcall end-point-func))
     (move-marker ins-point (point))
     (insert (delete-and-extract-region beg end))
     (goto-char ins-point)