bug#19493: [PATCH] Fix trailing ... for outline-toggle-children

bug#19493: [PATCH] Fix trailing ... for outline-toggle-children

[Oleh Krehel]

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

Hello,

Calling `outline-toggle-children' twice for a test.org with these
contents:

    * foo
    bar

results in:

    * foo
    bar...

I attach a patch to fix this.

regards,
Oleh

[-- Attachment #2: 0001-Fix-trailing-.-in-outline.patch --]
[-- Type: text/x-patch, Size: 1153 bytes --]

From a70f169e5f5d224d46b421cb800f24ea75f3709f Mon Sep 17 00:00:00 2001
From: Oleh Krehel <ohwoeowho@gmail.com>
Date: Sat, 3 Jan 2015 00:55:59 +0100
Subject: [PATCH] Fix trailing ... in outline

* lisp/outline.el (outline-next-preface): Don't go backward char when
  at end of buffer.

This will fix the problem of calling `outline-toggle-children' twice
in a row for the file's last heading.
---
 lisp/outline.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/outline.el b/lisp/outline.el
index 11d71fb..260073d 100644
--- a/lisp/outline.el
+++ b/lisp/outline.el
@@ -420,9 +420,10 @@ or else the number of characters matched by `outline-regexp'."
 If there's no following heading line, stop before the newline
 at the end of the buffer."
   (if (re-search-forward (concat "\n\\(?:" outline-regexp "\\)")
-			 nil 'move)
+                         nil 'move)
       (goto-char (match-beginning 0)))
-  (if (and (bolp) (or outline-blank-line (eobp)) (not (bobp)))
+  (if (and (bolp) (or outline-blank-line (eobp))
+           (not (bobp)) (not (eobp)))
       (forward-char -1)))
 
 (defun outline-next-heading ()
-- 
1.9.1

bug#19493: Acknowledgement ([PATCH] Fix trailing ... for outline-toggle-children)

[Oleh Krehel]

Hello,

Is there an update on this?

regards,
Oleh

bug#19493: [PATCH] Fix trailing ... for outline-toggle-children

[Dmitry Gutov]

Hi! This piece of code doesn't make sense to me:

On 01/03/2015 03:04 AM, Oleh Krehel wrote:

-  (if (and (bolp) (or outline-blank-line (eobp)) (not (bobp)))
+  (if (and (bolp) (or outline-blank-line (eobp))
+           (not (bobp)) (not (eobp)))

The second condition already checks for `eobp', so apparently the 
function is supposed to handle the end-of-buffer case.

If that was an error in someone's thinking, the patch should correct 
that as well. But maybe the fix should do something in the caller instead.

bug#19493: [PATCH] Fix trailing ... for outline-toggle-children

[Oleh Krehel]

> Hi! This piece of code doesn't make sense to me:
>
> On 01/03/2015 03:04 AM, Oleh Krehel wrote:
>
> -  (if (and (bolp) (or outline-blank-line (eobp)) (not (bobp)))
> +  (if (and (bolp) (or outline-blank-line (eobp))
> +           (not (bobp)) (not (eobp)))
>
> The second condition already checks for `eobp', so apparently the function
> is supposed to handle the end-of-buffer case.
>
> If that was an error in someone's thinking, the patch should correct that as
> well. But maybe the fix should do something in the caller instead.

You're right, I hadn't noticed before.
Alternatively, `show-entry' can be patched like this:

    (defun show-entry ()
      "Show the body directly following this heading.
    Show the heading too, if it is currently invisible."
      (interactive)
      (save-excursion
        (outline-back-to-heading t)
        (outline-flag-region
         (1- (point))
         (progn
           (outline-next-preface)
           (min (1+ (point))
                (point-max))) nil)))

The point is just not to have a single trailing hidden char, which
results from passing different bounds to `outline-flag-region' when
hiding and unhiding.

Oleh

bug#19493: [PATCH] Fix trailing ... for outline-toggle-children

[Dmitry Gutov]

On 01/16/2015 04:12 PM, Oleh Krehel wrote:

> You're right, I hadn't noticed before.
> Alternatively, `show-entry' can be patched like this:

That doesn't seem to be the full solution either. For instance, if the 
current heading has subheadings, with or without your patch "..." will 
remain at the end of the last of them.

Maybe the fix has to touch `show-children' as well. I'd hope to see a 
one-liner change that would fix both cases, though. :)

> The point is just not to have a single trailing hidden char, which
> results from passing different bounds to `outline-flag-region' when
> hiding and unhiding.

How come we pass different bounds? `show-entry' does pass a lower FROM 
bound than `hide-entry', but that should have this effect, should it? 
It's the TO bound that should make the difference.

bug#19493: [PATCH] Fix trailing ... for outline-toggle-children

[Oleh Krehel]

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

I attach the updated patch.

On 19 January 2015 at 05:14, Dmitry Gutov <dgutov@yandex.ru> wrote:

> That doesn't seem to be the full solution either. For instance, if the
> current heading has subheadings, with or without your patch "..." will
> remain at the end of the last of them.

If you're using `outline-toggle-children', it's supposed to leave the
children hidden.

The patch basically just fixes `outline-show-entry', which was is called by
`outline-toggle-children' in my original issue.

Oleh

[-- Attachment #2: 0001-lisp-outline.el-outline-show-entry-Fix-one-invisible.patch --]
[-- Type: text/x-patch, Size: 1183 bytes --]

From 78ada3f9fcbd6fa066038325174561bf6a95e493 Mon Sep 17 00:00:00 2001
From: Oleh Krehel <ohwoeowho@gmail.com>
Date: Sat, 7 Feb 2015 18:54:07 +0100
Subject: [PATCH] lisp/outline.el (outline-show-entry): Fix one invisible char

* lisp/outline.el (outline-show-entry): Previously, when called for
the last outline in a file, a single invisible char was left.
Add a check for this condition.
---
 lisp/outline.el | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lisp/outline.el b/lisp/outline.el
index ae31b80..059ca62 100644
--- a/lisp/outline.el
+++ b/lisp/outline.el
@@ -777,7 +777,12 @@ Show the heading too, if it is currently invisible."
   (save-excursion
     (outline-back-to-heading t)
     (outline-flag-region (1- (point))
-                         (progn (outline-next-preface) (point)) nil)))
+                         (progn
+                           (outline-next-preface)
+                           (if (= 1 (- (point-max) (point)))
+                               (point-max)
+                             (point)))
+                         nil)))
 
 (define-obsolete-function-alias
     'show-entry 'outline-show-entry "25.1")
-- 
1.8.4

bug#19493: [PATCH] Fix trailing ... for outline-toggle-children

[Dmitry Gutov]

On 02/07/2015 09:07 PM, Oleh Krehel wrote:

> If you're using `outline-toggle-children', it's supposed to leave the
> children hidden.

Fair enough.

> The patch basically just fixes `outline-show-entry', which was is called by
> `outline-toggle-children' in my original issue.

I still don't like particularly like it (why 1 and not another number? 
doesn't that last character after point make any difference?), but 
please feel free to install and then follow up on any regressions if 
they come up.

In truth, I don't use outline-mode with any regularity, and nobody else 
has expressed interest in this bug, so you're best equipped to evaluate 
this fix.

bug#19493: [PATCH] Fix trailing ... for outline-toggle-children

[Lars Ingebrigtsen]

Oleh Krehel <o.krehel@tue.nl> writes:

>      (outline-flag-region (1- (point))
> -                         (progn (outline-next-preface) (point)) nil)))
> +                         (progn
> +                           (outline-next-preface)
> +                           (if (= 1 (- (point-max) (point)))
> +                               (point-max)
> +                             (point)))
> +                         nil)))

This looks like it was applied, so I'm closing the bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no