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: 2690 bytes --]

On Fri, 21 Nov 2014 12:42:56 +0200 Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Stephen Berman <stephen.berman@gmx.net>
>> Cc: paul@tilk.co,  19102@debbugs.gnu.org
>> Date: Fri, 21 Nov 2014 11:32:20 +0100
>> 
>> > Does it make sense to fix outline-move-subtree-up/down so that they
>> > also work when there's no empty line after the last subtree?  That's
>> 
>> It didn't occur to me before, but your question prompted me to check and
>> I see outline-mode derives from text-mode, which sets
>> require-final-newline to mode-require-final-newline, so I guess the
>> assumption is indeed that files in outline-mode should end in a newline.
>
> Does doing that solve the problem without the need to delete the added
> line?

Yes.  As long as there is a final newline, the only change the current
code needs is to replace `=' with `eq'.  So given that
require-final-newline is true for files in outline-mode, the only case
that requires adding a final newline is a non-file outline-mode buffer
that lacks a final newline.

>> > one thing that looks inelegant in your patch, and might also cause
>> > some (minor) trouble: what if the user types C-g before this function
>> > finishes?
>> 
>> We could avoid the trouble by wrapping the newline call in
>> unwind-protect, couldn't we?
>
> Yes, but it's better to avoid that in the first place.
>
>> But can C-g really take effect here?
>> There is no place in the function where execution halts to wait for user
>> feedback.
>
> C-g sets a flag that is checked by evaluation.

I don't understand how this could result in C-g taking effect before the
function finishes; could you elaborate?

>> I guess those are idle speculations, it does seem that just keeping a
>> final newline added by the function is the simplest (and perhaps best)
>> fix, so I guess we should commit some version of that fix and see if
>> anyone complains.
>
> Go for it, and thanks.

I think that, once the non-file buffer case is taken into account (and
not doing means moving a subtree can corrupt the outline by putting two
headers on the same line), the cleanest fix is basically the one Paul
Rankin proposed in his last post.  I've attached it as a diff against
emacs-24, where I assume the fix should be committed (I added a comment
and tweaked the function Paul posted to avoid irrelevant changes to the
current code, and also restricted the error handling by making it a
user-error and having it signal only when the user attempts to move over
a higher outline level, avoiding an inappropriate message at bob or
eob).  Does this patch qualify as a tiny change, or does Paul have a
copyright assignment on file (I don't have access to the file)?

Steve Berman


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

diff --git a/lisp/outline.el b/lisp/outline.el
index c7cad31..3243e15 100644
--- a/lisp/outline.el
+++ b/lisp/outline.el
@@ -645,31 +645,38 @@ 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))
-	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))
-    (if (= (char-after) ?\n) (forward-char 1))
-    (setq end (point))
-    ;; Find insertion point, with error handling
+  (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 (and (eobp) (eolp) (not (bolp)))
+			       (insert-char ?\n))
+			   (unless (eobp)
+			     (forward-char 1))
+			   (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)
       (or (funcall movfunc)
-	  (progn (goto-char beg)
-		 (error "Cannot move past superior level")))
+	  (unless (or (bobp) (eobp))
+	    (goto-char beg)
+	    (user-error "Cannot shift past higher level")))
       (setq cnt (1- cnt)))
     (if (> arg 0)
-	;; Moving forward - still need to move over subtree
-	(progn (outline-end-of-subtree)
-	       (if (= (char-after) ?\n) (forward-char 1))))
+	;; Moving forward - still need to move over subtree.
+	(funcall end-point-func))
     (move-marker ins-point (point))
     (insert (delete-and-extract-region beg end))
     (goto-char ins-point)