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

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

[Paul Rankin]

1. emacs -Q
2. M-x switch-to-buffer RET test
3. M-x outline-mode
4. insert:
   * one
   * two
   * three
5. position point at "* three"
6. M-x outline-move-subtree-up
   => Wrong type argument: number-or-marker-p, nil
7. position point at "* two"
8. M-x outline-move-subtree-down
   => Wrong type argument: number-or-marker-p, nil

Expected behaviour:

- at 6. subtree "* three" should move above "* two"
- or at 8. subtree "* two" should move below "* three"


In GNU Emacs 24.4.1 (x86_64-apple-darwin14.0.0, NS apple-appkit-1343.14)
 of 2014-10-21 on Paul-Rankins-MacBook-Pro.local
Windowing system distributor `Apple', version 10.3.1343
Configured using:
 `configure --prefix=/usr/local/Cellar/emacs/24.4
 --enable-locallisppath=/usr/local/share/emacs/site-lisp
 --infodir=/usr/local/Cellar/emacs/24.4/share/info/emacs --without-dbus
 --with-gnutls --with-ns --disable-ns-self-contained'

Important settings:
  value of $LANG: en_AU.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Outline

Minor modes in effect:
  tooltip-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
M-x s w i t <tab> b u <tab> <return> t e s t <return> 
M-x o u t l i n e SPC m o d <tab> <return> s-v <up> 
M-x o u t l i n e SPC m o v e <tab> u p <tab> <return> 
<up> <up> M-x o u t l i n e SPC m o v e <tab> d o <tab> 
<return> M-x r e p o r t <tab> <return>

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
You can run the command `switch-to-buffer' with C-x b
Mark set
outline-move-subtree-up: Wrong type argument: number-or-marker-p, nil
call-interactively: Wrong type argument: number-or-marker-p, nil

Load-path shadows:
None found.

Features:
(shadow sort gnus-util mail-extr emacsbug message format-spec rfc822 mml
easymenu mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util help-fns mail-prsvr mail-utils noutline outline easy-mmode
time-date tooltip electric uniquify ediff-hook vc-hooks lisp-float-type
mwheel ns-win tool-bar dnd fontset image regexp-opt fringe
tabulated-list newcomment lisp-mode prog-mode register page menu-bar
rfn-eshadow timer select scroll-bar mouse jit-lock font-lock syntax
facemenu font-core frame cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese hebrew greek romanian slovak
czech european ethiopic indian cyrillic chinese case-table epa-hook
jka-cmpr-hook help simple abbrev minibuffer nadvice loaddefs button
faces cus-face macroexp files text-properties overlay sha1 md5 base64
format env code-pages mule custom widget hashtable-print-readable
backquote make-network-process cocoa ns multi-tty emacs)

Memory information:
((conses 16 73311 8272)
 (symbols 48 17397 0)
 (miscs 40 41 170)
 (strings 32 10116 4888)
 (string-bytes 1 269187)
 (vectors 16 9102)
 (vector-slots 8 374266 16740)
 (floats 8 54 226)
 (intervals 56 204 0)
 (buffers 960 13))

-- 
Paul W. Rankin
http://www.paulwrankin.com

Before printing this email please take a moment to think about the
environment. Just stop and think about it. Think about the last time you
were walking alone in a forest, how you felt at peace, how a wave of
clarity seemed to overcome you and you had to stop and reevaluate your
life, what you're doing with the limited time you have here. "Damn," you
thought, "life is so precious. I should really be doing ______." Are you
doing that now? Why not? Go on, pick up your computer and throw it out
the window! It'll be great, like that scene from Network where everyone
starts yelling "I'M MAD AS HELL AND I'M NOT GOING TO TAKE THIS ANY
MORE." That'll be you, but it will be real. Now's your moment.
-- 
Paul W. Rankin
http://www.paulwrankin.com

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

[Stephen Berman]

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

On Wed, 19 Nov 2014 18:29:01 +1000 Paul Rankin <paul@tilk.co> wrote:

> 1. emacs -Q
> 2. M-x switch-to-buffer RET test
> 3. M-x outline-mode
> 4. insert:
>    * one
>    * two
>    * three
> 5. position point at "* three"
> 6. M-x outline-move-subtree-up
>    => Wrong type argument: number-or-marker-p, nil
> 7. position point at "* two"
> 8. M-x outline-move-subtree-down
>    => Wrong type argument: number-or-marker-p, nil
>
> Expected behaviour:
>
> - at 6. subtree "* three" should move above "* two"
> - or at 8. subtree "* two" should move below "* three"

The following patch (against master) seems to fix this, though it could
probably be done more elegantly.

Steve Berman


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

diff --git a/lisp/outline.el b/lisp/outline.el
index c7cad31..bfed982 100644
--- a/lisp/outline.el
+++ b/lisp/outline.el
@@ -649,17 +649,27 @@ the match data is set appropriately."
 		   'outline-get-last-sibling))
 	(ins-point (make-marker))
 	(cnt (abs arg))
+	;; Make sure we can move forward when needed (bug#19102).
+	(maybe-forward-char (lambda ()
+			      (when (and (eobp) (not (bolp)))
+				(newline))
+			      (unless (bolp)
+				(if (= (char-after) ?\n)
+				    (forward-char 1)))))
+	(empty-last-line (save-excursion
+			   (goto-char (point-max))
+			   (and (bolp) (eolp))))
 	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)
@@ -667,14 +677,19 @@ the match data is set appropriately."
 		 (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)
     (if folded (hide-subtree))
-    (move-marker ins-point nil)))
+    (move-marker ins-point nil)
+    ;; Clean up if necessary.
+    (save-excursion
+      (goto-char (point-max))
+      (when (and (bolp) (eolp) (not empty-last-line))
+	(delete-char -1)))))
 
 (defun outline-end-of-heading ()
   (if (re-search-forward outline-heading-end-regexp nil 'move)

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

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@gmx.net>
> Date: Wed, 19 Nov 2014 14:17:25 +0100
> Cc: 19102@debbugs.gnu.org
> 
> The following patch (against master) seems to fix this, though it could
> probably be done more elegantly.

Thanks.  Can you explain why the problem happened in the first place?

> +	;; Make sure we can move forward when needed (bug#19102).

Please only mention the bug number in comments if the code or the rest
of the comment text do not speak for themselves, and there are some
subtle issues here that can only be understood by reading the bug
discussion.  Otherwise, the bug number will appear in the commit log,
so no need to put it in the code.

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

[Stephen Berman]

On Wed, 19 Nov 2014 17:54:41 +0200 Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Stephen Berman <stephen.berman@gmx.net>
>> Date: Wed, 19 Nov 2014 14:17:25 +0100
>> Cc: 19102@debbugs.gnu.org
>> 
>> The following patch (against master) seems to fix this, though it could
>> probably be done more elegantly.
>
> Thanks.  Can you explain why the problem happened in the first place?

The error occurs when the sexp `(= (char-after) ?\n)' in
outline-move-subtree-down is evaluated at eob.  This can happen either
when calling outline-move-subtree-down on either of the last two
subtrees, or when calling outline-move-subtree-up on the last subtree.
There are two triggers: (i) finding the end point of the subtree to be
moved up or down, and (ii) finding the insertion point to move the
selected subtree down to.  The patch avoids the error by making sure
there is a line after the final subtree, adding it if necessary and then
deleting the added line after the move.

>> +	;; Make sure we can move forward when needed (bug#19102).
>
> Please only mention the bug number in comments if the code or the rest
> of the comment text do not speak for themselves, and there are some
> subtle issues here that can only be understood by reading the bug
> discussion.  Otherwise, the bug number will appear in the commit log,
> so no need to put it in the code.

That occurred to me when I made the ChangeLog entry in my local tree; if
the patch is accepted, I'll remove the bug number from the comment.

Steve Berman

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

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@gmx.net>
> Cc: paul@tilk.co,  19102@debbugs.gnu.org
> Date: Wed, 19 Nov 2014 18:09:52 +0100
> 
> On Wed, 19 Nov 2014 17:54:41 +0200 Eli Zaretskii <eliz@gnu.org> wrote:
> > Thanks.  Can you explain why the problem happened in the first place?
> 
> The error occurs when the sexp `(= (char-after) ?\n)' in
> outline-move-subtree-down is evaluated at eob.

Wouldn't using eolp instead of the comparison solve that problem more
easily?

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

[Stephen Berman]

On Wed, 19 Nov 2014 20:56:12 +0200 Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Stephen Berman <stephen.berman@gmx.net>
>> Cc: paul@tilk.co,  19102@debbugs.gnu.org
>> Date: Wed, 19 Nov 2014 18:09:52 +0100
>> 
>> On Wed, 19 Nov 2014 17:54:41 +0200 Eli Zaretskii <eliz@gnu.org> wrote:
>> > Thanks.  Can you explain why the problem happened in the first place?
>> 
>> The error occurs when the sexp `(= (char-after) ?\n)' in
>> outline-move-subtree-down is evaluated at eob.
>
> Wouldn't using eolp instead of the comparison solve that problem more
> easily?

Well, that eliminates the wrong-type-argument error in the current code,
but it instead signals "End of buffer" and still fails to move the
subtree in the cases I listed.  But using eolp in place of the
comparison in my patch is fine, and cleaner, so thanks.  Do you see any
other problems with the patch or more room for improvement?

Steve Berman

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

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@gmx.net>
> Cc: paul@tilk.co,  19102@debbugs.gnu.org
> Date: Wed, 19 Nov 2014 21:14:42 +0100
> 
> > Wouldn't using eolp instead of the comparison solve that problem more
> > easily?
> 
> Well, that eliminates the wrong-type-argument error in the current code,
> but it instead signals "End of buffer"

From forward-char?  If so, you could avoid the call if eobp.  Or wrap
the call in condition-case and ignore errors.

> Do you see any other problems with the patch or more room for
> improvement?

I was worried by the complexity of maybe-forward-char, but maybe now
it is much simpler.

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

[Stephen Berman]

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

On Wed, 19 Nov 2014 22:32:06 +0200 Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Stephen Berman <stephen.berman@gmx.net>
>> Cc: paul@tilk.co,  19102@debbugs.gnu.org
>> Date: Wed, 19 Nov 2014 21:14:42 +0100
>> 
>> > Wouldn't using eolp instead of the comparison solve that problem more
>> > easily?
>> 
>> Well, that eliminates the wrong-type-argument error in the current code,
>> but it instead signals "End of buffer"
>
> From forward-char?  If so, you could avoid the call if eobp.  Or wrap
> the call in condition-case and ignore errors.

Those solutions work only if there's an empty line after the last
subtree.  If there isn't, I can't see any way other than my patch.

>> Do you see any other problems with the patch or more room for
>> improvement?
>
> I was worried by the complexity of maybe-forward-char, but maybe now
> it is much simpler.

I simplified it just a bit more, but that's all I can think of.  I also
had overlooked a side-effect of the error signaled when trying to move
the last subtree down: it leaves a dangling newline; this is avoided
using with-demoted-errors.  The revised patch is appended.

Steve Berman


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

diff --git a/lisp/outline.el b/lisp/outline.el
index c7cad31..a34ff12 100644
--- a/lisp/outline.el
+++ b/lisp/outline.el
@@ -649,32 +649,46 @@ 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 ()
+			      (and (eobp) (not (bolp)) (newline))
+			      (and (eolp) (not (bolp)) (forward-char 1))))
+	(empty-last-line (save-excursion
+			   (goto-char (point-max))
+			   (and (bolp) (eolp))))
 	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")))
+		 (with-demoted-errors "%s"
+		   (message "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)
     (if folded (hide-subtree))
-    (move-marker ins-point nil)))
+    (move-marker ins-point nil)
+    ;; If we added a newline to move forward, delete it.
+    (save-excursion
+      (goto-char (point-max))
+      (when (and (bolp) (eolp) (not empty-last-line))
+	(delete-char -1)))))
 
 (defun outline-end-of-heading ()
   (if (re-search-forward outline-heading-end-regexp nil 'move)

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

[Paul Rankin]

Stephen Berman <stephen.berman@gmx.net> writes:

> On Wed, 19 Nov 2014 22:32:06 +0200 Eli Zaretskii <eliz@gnu.org> wrote:
>
>>> From: Stephen Berman <stephen.berman@gmx.net>
>>> Cc: paul@tilk.co,  19102@debbugs.gnu.org
>>> Date: Wed, 19 Nov 2014 21:14:42 +0100
>>> 
>>> > Wouldn't using eolp instead of the comparison solve that problem more
>>> > easily?
>>> 
>>> Well, that eliminates the wrong-type-argument error in the current code,
>>> but it instead signals "End of buffer"
>>
>> From forward-char?  If so, you could avoid the call if eobp.  Or wrap
>> the call in condition-case and ignore errors.
>
> Those solutions work only if there's an empty line after the last
> subtree.  If there isn't, I can't see any way other than my patch.
>
>>> Do you see any other problems with the patch or more room for
>>> improvement?
>>
>> I was worried by the complexity of maybe-forward-char, but maybe now
>> it is much simpler.
>
> I simplified it just a bit more, but that's all I can think of.  I also
> had overlooked a side-effect of the error signaled when trying to move
> the last subtree down: it leaves a dangling newline; this is avoided
> using with-demoted-errors.  The revised patch is appended.
>
> Steve Berman

Sorry if there is some very obvious reason I'm missing, but can't we
just change

    (= (char-after) ?\n)

to

    (eq (char-after) ?\n)

...?

-- 
Paul W. Rankin
http://www.paulwrankin.com

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

[Stephen Berman]

On Thu, 20 Nov 2014 16:46:22 +1000 Paul Rankin <paul@tilk.co> wrote:

> Stephen Berman <stephen.berman@gmx.net> writes:
>
>> On Wed, 19 Nov 2014 22:32:06 +0200 Eli Zaretskii <eliz@gnu.org> wrote:
>>
>>>> From: Stephen Berman <stephen.berman@gmx.net>
>>>> Cc: paul@tilk.co,  19102@debbugs.gnu.org
>>>> Date: Wed, 19 Nov 2014 21:14:42 +0100
>>>> 
>>>> > Wouldn't using eolp instead of the comparison solve that problem more
>>>> > easily?
>>>> 
>>>> Well, that eliminates the wrong-type-argument error in the current code,
>>>> but it instead signals "End of buffer"
>>>
>>> From forward-char?  If so, you could avoid the call if eobp.  Or wrap
>>> the call in condition-case and ignore errors.
>>
>> Those solutions work only if there's an empty line after the last
>> subtree.  If there isn't, I can't see any way other than my patch.
>>
>>>> Do you see any other problems with the patch or more room for
>>>> improvement?
>>>
>>> I was worried by the complexity of maybe-forward-char, but maybe now
>>> it is much simpler.
>>
>> I simplified it just a bit more, but that's all I can think of.  I also
>> had overlooked a side-effect of the error signaled when trying to move
>> the last subtree down: it leaves a dangling newline; this is avoided
>> using with-demoted-errors.  The revised patch is appended.
>>
>> Steve Berman
>
> Sorry if there is some very obvious reason I'm missing, but can't we
> just change
>
>     (= (char-after) ?\n)
>
> to
>
>     (eq (char-after) ?\n)
>
> ...?

This has the same effect as Eli's suggestion of not calling forward-char
at eob or wrapping it in condition-case: outline-move-subtree-up/down
works if there's an empty line after the last subtree, but if not, it
puts the last two subtree headers on the same line.

Steve Berman

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

[Paul Rankin]

Stephen Berman <stephen.berman@gmx.net> writes:

>> Sorry if there is some very obvious reason I'm missing, but can't we
>> just change
>>
>>     (= (char-after) ?\n)
>>
>> to
>>
>>     (eq (char-after) ?\n)
>>
>> ...?
>
> This has the same effect as Eli's suggestion of not calling forward-char
> at eob or wrapping it in condition-case: outline-move-subtree-up/down
> works if there's an empty line after the last subtree, but if not, it
> puts the last two subtree headers on the same line.
>
> Steve Berman

Sorry, I spoke far too soon there.

-- 
Paul W. Rankin
http://www.paulwrankin.com

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

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@gmx.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  19102@debbugs.gnu.org
> Date: Thu, 20 Nov 2014 11:08:13 +0100
> 
> outline-move-subtree-up/down works if there's an empty line after
> the last subtree, but if not, it puts the last two subtree headers
> on the same line.

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
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?

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

[Stephen Berman]

On Thu, 20 Nov 2014 18:21:03 +0200 Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Stephen Berman <stephen.berman@gmx.net>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  19102@debbugs.gnu.org
>> Date: Thu, 20 Nov 2014 11:08:13 +0100
>> 
>> outline-move-subtree-up/down works if there's an empty line after
>> the last subtree, but if not, it puts the last two subtree headers
>> on the same line.
>
> 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.

> 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?  But can C-g really take effect here?
There is no place in the function where execution halts to wait for user
feedback.  Maybe with an outline containing an enormous number of
subtrees the calculations could take long enough to make C-g interrupt
it, though I tried and failed to do it moving over 500 subtrees.  But 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.

Steve Berman

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

[Eli Zaretskii]

> 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?

> > 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 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.

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

[Stephen Berman]

[-- 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)

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

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@gmx.net>
> Cc: paul@tilk.co,  19102@debbugs.gnu.org
> Date: Fri, 21 Nov 2014 18:31:19 +0100
> 
> >> 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?

C-g sets a "quit" flag.  When Lisp evaluation takes place and the quit
flag is set, the Lisp interpreter throws to top-level, thus
interrupting whatever function was running.

> 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)?

I don't see his assignment.  IMO, this patch is borderline wrt being
"tiny"; I'll let Stefan judge.

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

[Stephen Berman]

On Fri, 21 Nov 2014 21:56:07 +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 18:31:19 +0100
>> 
>> >> 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?
>
> C-g sets a "quit" flag.  When Lisp evaluation takes place and the quit
> flag is set, the Lisp interpreter throws to top-level, thus
> interrupting whatever function was running.

Thanks for the explanation.

>> 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)?
>
> I don't see his assignment.  IMO, this patch is borderline wrt being
> "tiny"; I'll let Stefan judge.

Ok.

Steve Berman

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

[Paul Rankin]

Eli Zaretskii <eliz@gnu.org> writes:

>> 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)?
>
> I don't see his assignment.  IMO, this patch is borderline wrt being
> "tiny"; I'll let Stefan judge.

If it doesn't qualify as tiny, I'm happy to say Stephen took some
unofficial inspiration from some errant suggestion from me and wrote the
code all by himself, ergo copyright is all his :)

-- 
Paul W. Rankin
http://www.paulwrankin.com

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

[Stefan Monnier]

> 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.

I don't understand why there'd be a difference between file and
non-file buffers.  Outline-mode doesn't care about files at all.

The buffer can end without a newline both in the file and in the
non-file case, regardless of the require-final-newline setting.

OTOH, we can add a final newline pretty much any time we feel like it
(better not do it gratuitously, but in the present case I can't think
of any reason why we'd go out of our way just to avoid adding a final
newline).

Having failed to follow the thread until now, I don't know what
should be done, but I hope this comment of mine can help someone
else decide.


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: Eli Zaretskii <eliz@gnu.org>, paul@tilk.co, 19102@debbugs.gnu.org
> Date: Sat, 22 Nov 2014 11:32:21 -0500
> 
> Having failed to follow the thread until now, I don't know what
> should be done

Please turn your attention to this at least:

  http://debbugs.gnu.org/cgi/bugreport.cgi?bug=19102#62

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

[Stephen Berman]

On Sat, 22 Nov 2014 11:32:21 -0500 Stefan Monnier <monnier@IRO.UMontreal.CA> wrote:

>> 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.
>
> I don't understand why there'd be a difference between file and
> non-file buffers.  Outline-mode doesn't care about files at all.
>
> The buffer can end without a newline both in the file and in the
> non-file case, regardless of the require-final-newline setting.

But by default, when a file in outline-mode is saved, if it doesn't end
in a new line one is added, so on revisiting the file the user would
have to delete the final newline for there to be none.  Otherwise, the
only problematic case is a buffer in outline-mode that hasn't been
written to a file yet, since here the user has to add a final newline to
avoid errors.  Here's a minimal recipe to show the problem:

0. emacs -Q
1. C-x b test RET
2. evaluate this sexp:
   (progn
     (switch-to-buffer (generate-new-buffer "test"))
     (insert "* one\n* two\n")
     (outline-mode))
3. Typing `C-c C-v' (outline-move-subtree-down) with point on either
   line, or typing `C-c C-^' (outline-move-subtree-up) with point on the
   second line, raises the error "Wrong type argument:
   number-or-marker-p, nil".  (This is the error of the OP of this bug.)
4. You can avoid this error by replacing both occurrences of `=' with
   `eq' in outline-move-subtree-down, and now `C-c C-v' and `C-c C-^'
   work as expected.
5. Now delete the final newline, so that eob is after the "o" in line 2.
=> Now typing either `C-c C-v' with point on line one or `C-c C-^' with
   point line two result in the buffer containing the single line
   "* two* one".

So once the error in step 3 is fixed, you still have to make sure
there's a final newline.

> OTOH, we can add a final newline pretty much any time we feel like it
> (better not do it gratuitously, but in the present case I can't think
> of any reason why we'd go out of our way just to avoid adding a final
> newline).

Does "we" refer to the outline.el code?  If so, the proposed patch does
just the opposite: it makes sure there's a final newline.

Does this make things clearer?

Steve Berman

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

[Stefan Monnier]

> But by default, when a file in outline-mode is saved, if it doesn't end

Who cares.  A file-buffer may never be saved.  And the
require-final-newline option may be set differently, so even if saved we
can't assume that there's always a final newline.

> Does "we" refer to the outline.el code?  If so, the proposed patch does
> just the opposite: it makes sure there's a final newline.

Sounds like we're in violent agreement, then.


        Stefan

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

[Stephen Berman]

On Sun, 23 Nov 2014 23:07:49 -0500 Stefan Monnier <monnier@IRO.UMontreal.CA> wrote:

>> Does "we" refer to the outline.el code?  If so, the proposed patch does
>> just the opposite: it makes sure there's a final newline.
>
> Sounds like we're in violent agreement, then.

Ok, so I take it you accept the patch, so again the question, does it
qualify as a tiny change?  If not, and if Paul Rankin is unable or
doesn't want to sign a copyright assignment, what's the appropriate way
to give him credit?

Steve Berman

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

[Paul Rankin]

Stephen Berman <stephen.berman@gmx.net> writes:

> On Sun, 23 Nov 2014 23:07:49 -0500 Stefan Monnier <monnier@IRO.UMontreal.CA> wrote:
>
>>> Does "we" refer to the outline.el code?  If so, the proposed patch does
>>> just the opposite: it makes sure there's a final newline.
>>
>> Sounds like we're in violent agreement, then.
>
> Ok, so I take it you accept the patch, so again the question, does it
> qualify as a tiny change?  If not, and if Paul Rankin is unable or
> doesn't want to sign a copyright assignment, what's the appropriate way
> to give him credit?

I'm happy to do the copyright assignment biz if needed, I just think
Stephen you did 99% of the work. Whatever is the right netiquette here,
I go with that.

-- 
Paul W. Rankin
http://www.paulwrankin.com

Before printing this email please take a moment to think about the
environment. Just stop and think about it. Think about the last time you
were walking alone in a forest, how you felt at peace, how a wave of
clarity seemed to overcome you and you had to stop and reevaluate your
life, what you're doing with the limited time you have here. "Damn," you
thought, "life is so precious. I should really be doing ______." Are you
doing that now? Why not? Go on, pick up your computer and throw it out
the window! It'll be great, like that scene from Network where everyone
starts yelling "I'M MAD AS HELL AND I'M NOT GOING TO TAKE THIS ANY
MORE." That'll be you, but it will be real. Now's your moment.

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

[Stephen Berman]

On Wed, 26 Nov 2014 12:34:52 +1000 Paul Rankin <paul@tilk.co> wrote:

> Stephen Berman <stephen.berman@gmx.net> writes:
>
>> On Sun, 23 Nov 2014 23:07:49 -0500 Stefan Monnier <monnier@IRO.UMontreal.CA> wrote:
>>
>>>> Does "we" refer to the outline.el code?  If so, the proposed patch does
>>>> just the opposite: it makes sure there's a final newline.
>>>
>>> Sounds like we're in violent agreement, then.
>>
>> Ok, so I take it you accept the patch, so again the question, does it
>> qualify as a tiny change?  If not, and if Paul Rankin is unable or
>> doesn't want to sign a copyright assignment, what's the appropriate way
>> to give him credit?
>
> I'm happy to do the copyright assignment biz if needed, I just think

You could ask Stefan for the form to fill out to initiate the copyright
assignment process just in case, or for any future contribution to Emacs
you might want to make.

> Stephen you did 99% of the work. Whatever is the right netiquette here,
> I go with that.

Let's see what Stefan says.  Anyway, the important thing is to get the
bug fixed!

Steve Berman

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

[Paul Rankin]

Stephen Berman <stephen.berman@gmx.net> writes:

> On Wed, 19 Nov 2014 22:32:06 +0200 Eli Zaretskii <eliz@gnu.org> wrote:
>
>>> From: Stephen Berman <stephen.berman@gmx.net>
>>> Cc: paul@tilk.co,  19102@debbugs.gnu.org
>>> Date: Wed, 19 Nov 2014 21:14:42 +0100
>>> 
>>> > Wouldn't using eolp instead of the comparison solve that problem more
>>> > easily?
>>> 
>>> Well, that eliminates the wrong-type-argument error in the current code,
>>> but it instead signals "End of buffer"
>>
>> From forward-char?  If so, you could avoid the call if eobp.  Or wrap
>> the call in condition-case and ignore errors.
>
> Those solutions work only if there's an empty line after the last
> subtree.  If there isn't, I can't see any way other than my patch.
>
>>> Do you see any other problems with the patch or more room for
>>> improvement?
>>
>> I was worried by the complexity of maybe-forward-char, but maybe now
>> it is much simpler.
>
> I simplified it just a bit more, but that's all I can think of.  I also
> had overlooked a side-effect of the error signaled when trying to move
> the last subtree down: it leaves a dangling newline; this is avoided
> using with-demoted-errors.  The revised patch is appended.
>
> Steve Berman

I needed to rewrite this for functionality with my own package, so I
rewrote it very arrogantly and this is what I came up with...

(defun fountain-outline-shift-down (&optional arg)
  (interactive "p")
  (outline-back-to-heading)
  (let* ((move-func
          (if (< 0 arg)
              'outline-get-next-sibling
            'outline-get-last-sibling))
         (end-point-func
          '(lambda ()
             (outline-end-of-subtree)
             (if (and (eolp)
                      (not (bolp))
                      (eobp))
                 (open-line 1))
             (unless (eobp)
               (forward-char 1))
             (point)))
         (beg (point))
         (end
          (save-excursion
            (funcall end-point-func)))
         (folded
          (save-excursion
            (outline-end-of-heading)
            (outline-invisible-p)))
         (insert-point (make-marker))
         (i (abs arg)))
    (goto-char beg)
    (while (< 0 i)
      (or (funcall move-func)
          (progn (goto-char beg)
                 (message "Cannot move past superior level")))
      (setq i (1- i)))
    (if (< 0 arg)                       ; what does this bit do??
        (funcall end-point-func))
    (move-marker insert-point (point))
    (insert (delete-and-extract-region beg end))
    (goto-char insert-point)
    (if folded
        (hide-subtree))
    (set-marker insert-point nil)))

The only thing I'm not sure about is the line marked above. This is not
intended for Emacs, just wanna see if I'm on the right track :)

-- 
Paul W. Rankin
http://www.paulwrankin.com

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

[Stephen Berman]

On Thu, 20 Nov 2014 17:22:50 +1000 Paul Rankin <paul@tilk.co> wrote:

> I needed to rewrite this for functionality with my own package, so I
> rewrote it very arrogantly and this is what I came up with...
>
> (defun fountain-outline-shift-down (&optional arg)
>   (interactive "p")
>   (outline-back-to-heading)
>   (let* ((move-func
>           (if (< 0 arg)
>               'outline-get-next-sibling
>             'outline-get-last-sibling))
>          (end-point-func
>           '(lambda ()
>              (outline-end-of-subtree)
>              (if (and (eolp)
>                       (not (bolp))
>                       (eobp))
>                  (open-line 1))
>              (unless (eobp)
>                (forward-char 1))
>              (point)))
>          (beg (point))
>          (end
>           (save-excursion
>             (funcall end-point-func)))
>          (folded
>           (save-excursion
>             (outline-end-of-heading)
>             (outline-invisible-p)))
>          (insert-point (make-marker))
>          (i (abs arg)))
>     (goto-char beg)
>     (while (< 0 i)
>       (or (funcall move-func)
>           (progn (goto-char beg)
>                  (message "Cannot move past superior level")))
>       (setq i (1- i)))
>     (if (< 0 arg)                       ; what does this bit do??
>         (funcall end-point-func))
>     (move-marker insert-point (point))
>     (insert (delete-and-extract-region beg end))
>     (goto-char insert-point)
>     (if folded
>         (hide-subtree))
>     (set-marker insert-point nil)))

This basically does the same thing as the patch I proposed -- adding a
newline if necessary to ensure forward movement -- except that if there
was no empty line after the last subtree, your code leaves the added
newline dangling.  If you add that bit then there is effectively no
difference between your version and mine.  However, yours is slightly
shorter and slightly cleaner, since it avoids a couple of setq's of
let-bound variables, so maybe it's the better fix (after adding the line
deletion bit; also, it's not necessary and AFAIK stylistically
discouraged to quote a lambda form; and finally, I'm not sure if
open-line is more or less appropriate here than newline -- maybe both
are too heavyweight and (insert "\n") or even (terpri) would suffice?

> The only thing I'm not sure about is the line marked above. This is not
> intended for Emacs, just wanna see if I'm on the right track :)

The line you marked tests whether we're moving the subtree down
(positive arg) and if so ensures we find the insertion point after it.
Or are you asking about something else?

Steve Berman

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

[Paul Rankin]

Stephen Berman <stephen.berman@gmx.net> writes:

> This basically does the same thing as the patch I proposed -- adding a
> newline if necessary to ensure forward movement -- except that if there
> was no empty line after the last subtree, your code leaves the added
> newline dangling.  If you add that bit then there is effectively no
> difference between your version and mine.

For my package's purposes I think it might be better to have that
dangling newline created it's not already there, but yeah, not for
outline-mode. (This may be a faux pas on my part.)

However, how about this for the dangling line code?

--8<---------------cut here---------------start------------->8---
(unless empty-last-line
  (save-excursion
    (goto-char (point-max))
    (if (and (bolp) (eolp))
        (delete-char -1)))))
--8<---------------cut here---------------end--------------->8---

That way it doesn't execute if it doesn't need to. Or am I over-thinking
it?

> However, yours is slightly shorter and slightly cleaner, since it
> avoids a couple of setq's of let-bound variables, so maybe it's the
> better fix (after adding the line deletion bit; also, it's not
> necessary and AFAIK stylistically discouraged to quote a lambda form;
> and finally, I'm not sure if open-line is more or less appropriate
> here than newline -- maybe both are too heavyweight and (insert "\n")
> or even (terpri) would suffice?

Awesome, thanks. I'm not proposing any of my code go into Emacs, so go
with whatever you think is best.

>> The only thing I'm not sure about is the line marked above. This is not
>> intended for Emacs, just wanna see if I'm on the right track :)
>
> The line you marked tests whether we're moving the subtree down
> (positive arg) and if so ensures we find the insertion point after it.
> Or are you asking about something else?

Thanks. Yes that was all, please do not be under any impression that I
have any idea what I'm doing.

I did find some problems with saving match data though, so just for the
heck of it, I've pasted the function I ended up going with for my
package (it probably looks like I rewrote things from your patch just to
be contrary, but it's just to fit with the internal style of the
package):

--8<---------------cut here---------------start------------->8---
(defun fountain-outline-shift-down (&optional n)
  (interactive "p")
  (outline-back-to-heading)
  (let* ((move-func
          (if (< 0 n)
              'outline-get-next-sibling
            'outline-get-last-sibling))
         (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)))
         (insert-point (make-marker))
         (i (abs n)))
    (goto-char beg)
    (while (< 0 i)
      (or (funcall move-func)
          (progn (goto-char beg)
                 (message "Cannot shift past higher level")))
      (setq i (1- i)))
    (if (< 0 n)
        (funcall end-point-func))
    (move-marker insert-point (point))
    (insert (delete-and-extract-region beg end))
    (goto-char insert-point)
    (if folded
        (hide-subtree))
    (set-marker insert-point nil)))
--8<---------------cut here---------------end--------------->8---



-- 
Paul W. Rankin
http://www.paulwrankin.com

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

[Stephen Berman]

On Thu, 20 Nov 2014 23:43:10 +1000 Paul Rankin <paul@tilk.co> wrote:

> Stephen Berman <stephen.berman@gmx.net> writes:
>
>> This basically does the same thing as the patch I proposed -- adding a
>> newline if necessary to ensure forward movement -- except that if there
>> was no empty line after the last subtree, your code leaves the added
>> newline dangling.  If you add that bit then there is effectively no
>> difference between your version and mine.
>
> For my package's purposes I think it might be better to have that
> dangling newline created it's not already there, but yeah, not for
> outline-mode. (This may be a faux pas on my part.)

Actually, the faux pas seems to have been mine, see my latest reply to
Eli Zaretskii.

> However, how about this for the dangling line code?
>
> (unless empty-last-line
>   (save-excursion
>     (goto-char (point-max))
>     (if (and (bolp) (eolp))
>         (delete-char -1)))))
>
>
> That way it doesn't execute if it doesn't need to. Or am I over-thinking
> it?

I guess that could save a few CPU cycles, but I guess the issue is moot
now.

>> However, yours is slightly shorter and slightly cleaner, since it
>> avoids a couple of setq's of let-bound variables, so maybe it's the
>> better fix (after adding the line deletion bit; also, it's not
>> necessary and AFAIK stylistically discouraged to quote a lambda form;
>> and finally, I'm not sure if open-line is more or less appropriate
>> here than newline -- maybe both are too heavyweight and (insert "\n")
>> or even (terpri) would suffice?
>
> Awesome, thanks. I'm not proposing any of my code go into Emacs, so go
> with whatever you think is best.
>
>>> The only thing I'm not sure about is the line marked above. This is not
>>> intended for Emacs, just wanna see if I'm on the right track :)
>>
>> The line you marked tests whether we're moving the subtree down
>> (positive arg) and if so ensures we find the insertion point after it.
>> Or are you asking about something else?
>
> Thanks. Yes that was all, please do not be under any impression that I
> have any idea what I'm doing.

Well, you seem to have at least as much an idea about this as I do (you
probably shouldn't take that as a compliment ;-).

> I did find some problems with saving match data though, so just for the

Yes, the match data should be saved.

> heck of it, I've pasted the function I ended up going with for my
> package (it probably looks like I rewrote things from your patch just to
> be contrary, but it's just to fit with the internal style of the
> package):

> (defun fountain-outline-shift-down (&optional n)
>   (interactive "p")
>   (outline-back-to-heading)
>   (let* ((move-func
>           (if (< 0 n)
>               'outline-get-next-sibling
>             'outline-get-last-sibling))
>          (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)))
>          (insert-point (make-marker))
>          (i (abs n)))
>     (goto-char beg)
>     (while (< 0 i)
>       (or (funcall move-func)
>           (progn (goto-char beg)
>                  (message "Cannot shift past higher level")))
>       (setq i (1- i)))
>     (if (< 0 n)
>         (funcall end-point-func))
>     (move-marker insert-point (point))
>     (insert (delete-and-extract-region beg end))
>     (goto-char insert-point)
>     (if folded
>         (hide-subtree))
>     (set-marker insert-point nil)))

As far as I'm concerned it's fine to commit your fix (though probably
without the unrelated more or less stylistic differences); but since I'm
not the maintainer of outline.el nor a core Emacs maintainer, it's up to
(at least one of) them.

Steve Berman

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

[Stefan Monnier]

> +	(maybe-forward-char (lambda ()
> +			      (and (eobp) (not (bolp)) (newline))
> +			      (and (eolp) (not (bolp)) (forward-char 1))))

If we add a newline, we know don't want to do a `forward-char'.  So:

   			      (if (and (eobp) (not (bolp))) (newline)
   			        (and (eolp) (not (bolp)) (forward-char 1)))))

Also, I don't understand the (not (bolp)) test in the second line: the
code it replaces only tested (= (char-after) ?\n).  So I think it'd be
better to use:

   			      (if (and (eobp) (not (bolp))) (newline)
   			        (if (eq (char-after) ?\n) (forward-char 1)))))

And to be closer to the original code, I'd swap the two tests.  And I'd
use `insert' rather than `newline' since I don't want to run abbrev
expansions and things like that.

   			      (if (eq (char-after) ?\n) (forward-char 1)
   			        (if (and (eobp) (not (bolp))) (insert "\n")))))

> +	(empty-last-line (save-excursion
> +			   (goto-char (point-max))
> +			   (and (bolp) (eolp))))

At point-max, we know that (eolp) is non-nil, so you can just write

   	(empty-last-line (save-excursion
   			   (goto-char (point-max))
   			   (bolp))))

> -		 (error "Cannot move past superior level")))
> +		 (with-demoted-errors "%s"
> +		   (message "Cannot move past superior level"))))

This is wrong.  Did you mean maybe to replace `error' with
`user-error' instead, maybe?

> +    ;; If we added a newline to move forward, delete it.
> +    (save-excursion
> +      (goto-char (point-max))
> +      (when (and (bolp) (eolp) (not empty-last-line))
> +	(delete-char -1)))))

Let's not bother.  There's no harm in adding a missing final newline
when we modify such a line-oriented file.


        Stefan

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

[Stephen Berman]

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

On Tue, 25 Nov 2014 21:43:02 -0500 Stefan Monnier <monnier@IRO.UMontreal.CA> wrote:

>> +	(maybe-forward-char (lambda ()
>> +			      (and (eobp) (not (bolp)) (newline))
>> +			      (and (eolp) (not (bolp)) (forward-char 1))))
> 
> If we add a newline, we know don't want to do a `forward-char'.  So:

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.  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).

Here's the version of my patch with your improvements:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: outline-move-subtree-down (1) --]
[-- Type: text/x-emacs-lisp, Size: 1370 bytes --]

(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))
    ;; Find insertion point, with error handling.
    (goto-char beg)
    (while (> cnt 0)
      (or (funcall movfunc)
	  (unless (or (bobp) (eobp))
	    (goto-char beg)
	    (user-error "Cannot move past superior level")))
      (setq cnt (1- cnt)))
    (if (> arg 0)
	;; Moving forward - still need to move over subtree.
	(progn (outline-end-of-subtree)
	       (funcall maybe-forward-char)))
    (move-marker ins-point (point))
    (insert (delete-and-extract-region beg end))
    (goto-char ins-point)
    (if folded (hide-subtree))
    (move-marker ins-point nil)))

[-- Attachment #3: Type: text/plain, Size: 81 bytes --]


And here's Paul's version (tweaked to make it closer to the outline.el
style):


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: outline-move-subtree-down (2) --]
[-- Type: text/x-emacs-lisp, Size: 1412 bytes --]

(defun outline-move-subtree-down (&optional arg)
  "Move the current subtree down past ARG headlines of the same level."
  (interactive "p")
  (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)
	  (unless (or (bobp) (eobp))
	    (goto-char beg)
	    (user-error "Cannot move past superior level")))
      (setq cnt (1- cnt)))
    (if (> arg 0)
	;; 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)
    (if folded (hide-subtree))
    (move-marker ins-point nil)))

[-- Attachment #5: Type: text/plain, Size: 36 bytes --]


Which should we use?

Steve Berman

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

[Stefan Monnier]

> 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.

> 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.


        Stefan

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

[Stephen Berman]

[-- 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)

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

[Stefan Monnier]

> Ok, I've done the latter, appended below.

Both look fine, and the second is small enough to count as "tiny change"
(but we won't be able to accept much more from him before he signs the
copyright paperwork).  Please install, thank you,


        Stefan

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

[Stephen Berman]

On Wed, 26 Nov 2014 17:11:49 -0500 Stefan Monnier <monnier@IRO.UMontreal.CA> wrote:

>> Ok, I've done the latter, appended below.
>
> Both look fine, and the second is small enough to count as "tiny change"
> (but we won't be able to accept much more from him before he signs the
> copyright paperwork).  Please install, thank you,

Ok, but first, just two more questions: I assume the bugfix part should
go into emacs-24, but should the refactoring part go only into master?
Or should I combine them again into one commit (in emacs-24)?

Steve Berman

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

[Stefan Monnier]

> Ok, but first, just two more questions: I assume the bugfix part should
> go into emacs-24,

Yes.

> but should the refactoring part go only into master?
> Or should I combine them again into one commit (in emacs-24)?

Either way is OK.


        Stefan

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

[Stephen Berman]

On Wed, 26 Nov 2014 21:18:17 -0500 Stefan Monnier <monnier@IRO.UMontreal.CA> wrote:

>> Ok, but first, just two more questions: I assume the bugfix part should
>> go into emacs-24,
>
> Yes.
>
>> but should the refactoring part go only into master?
>> Or should I combine them again into one commit (in emacs-24)?
>
> Either way is OK.

I pushed the fix to emacs-24 and am closing this bug.  I think it's
cleaner for the refactoring part to go only into master; I'll do that
when the fix is merged.

Steve Berman

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

[Stefan Monnier]

> I pushed the fix to emacs-24 and am closing this bug.  I think it's
> cleaner for the refactoring part to go only into master; I'll do that
> when the fix is merged.

Sounds good, thank you,


        Stefan