Bug: org-archive-subtree-save-file-p logic [9.3.6 (release_9.3.6-399-ge6df03 @ /home/n/.emacs.d/straight/build/org/)]

Bug: org-archive-subtree-save-file-p logic [9.3.6 (release_9.3.6-399-ge6df03 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

The logic for saving the archive buffer in org-archive-subtree 
does not consider the (default) value  of 'from-org for 
org-archive-subtree-save-file-p.

#+begin_src emacs-lisp
;; Save and kill the buffer, if it is not the same
;; buffer and depending on `org-archive-subtree-save-file-p'
(unless (eq this-buffer buffer)
  (when (or (eq org-archive-subtree-save-file-p t)
            (and (boundp 'org-archive-from-agenda)
                 (eq org-archive-subtree-save-file-p 
                 'from-agenda)))
    (save-buffer)))
#+end_src

While patching this, I realized I'm not sure I understand the 
intended logic of each value for org-archive-subtree-save-file-p.
When setting it to 't', the defcustom :tag string claims "Always 
save the archive buffer".
This is not the case if archiving from within the current buffer. 
Perhaps a clearer :tag string?

#+begin_src emacs-lisp
(defcustom org-archive-subtree-save-file-p 'from-org
  ;;...
  (const :tag "Save the archive buffer unless it is the current 
  buffer" t)
  ;;...
  ))
#+end_src

The value 'from-org also still saves the archive buffer when 
archiving from a buffer that is not in Org mode.
I'm not entirely sure of its purpose. If the intent is to allow an 
option that prevents saving only when archiving from the agenda,
I suggest a single option excluding that case and saving for 
other, non-nil values:

#+begin_src emacs-lisp
(defcustom org-archive-subtree-save-file-p 'unless-agenda
  "Conditionally save the archive file after archiving a subtree.
The value 'unless-agenda prevents saving from the agenda-view.
Other non-nil values save the archive buffer unless it is the 
current buffer."
  :group 'org-archive
  :package-version '(Org . "9.4")
  :type '(choice
          (const :tag "Do not save archive buffer when archiving 
          from an agenda view" unless-agenda)
          (const :tag "Save the archive buffer unless it is the 
          current buffer" t)
          (const :tag "Do not save the archive buffer")))
#+end_src

Then the saving logic in org-archive-subtree becomes:

#+begin_src emacs-lisp
(when org-archive-subtree-save-file-p
  (unless (or (eq buffer this-buffer)
              (and (eq org-archive-subtree-save-file-p 
              'unless-agenda)
                   ;;bound when called from org-agenda.el
                   (boundp 'org-archive-from-agenda)))
    (save-buffer)))
#+end_src

If this is the intended logic, I think it could be cleaned up even 
more so that it reads better and I'm happy to submit a patch.
If not, I'm interested to hear the actual case.

Thanks


Emacs  : GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, X 
toolkit, cairo version 1.17.3, Xaw3d scroll bars)
 of 2020-02-23
Package: Org mode version 9.3.6 (release_9.3.6-399-ge6df03 @ 
/home/n/.emacs.d/straight/build/org/)

Re: Bug: org-archive-subtree-save-file-p logic [9.3.6 (release_9.3.6-399-ge6df03 @ /home/n/.emacs.d/straight/build/org/)]

[Kyle Meyer]

No Wayman <iarchivedmywholelife@gmail.com> writes:

> The logic for saving the archive buffer in org-archive-subtree 
> does not consider the (default) value  of 'from-org for 
> org-archive-subtree-save-file-p.

Hmm, indeed, the change from 3d0282ef8 (New option
`org-archive-subtree-save-file-p', 2020-01-31) looks incomplete.

> While patching this, I realized I'm not sure I understand the 
> intended logic of each value for org-archive-subtree-save-file-p.

From digging a bit, I think the history went like this:

  * All the way back in 4be4c5623 (version 4.12a, 2008-01-31),
    org-archive-subtree saved the archive buffer, except when it was the
    current buffer.

  [ The next two aren't relevant for the save part, but I'm noting
    because we should cleaned them up. ]

  * As of bf09955fe (Bugfix for `org-archive-subtree', 2008-03-01), the
    buffer was killed after being saved.

  * 343f3c478 (Keep archive buffer after archiving something to it,
    2009-10-28) reverted the killing, though it left behind a stale
    comment saying the buffer will be killed.

  * 63f6e851b (Do not save target buffer after archiving subtree,
    2017-11-25) stopped saving the buffer for performance reasons.

  * Some users preferred the older behavior.  An associated Debian bug
    was opened.

    https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=887332
    https://yhetil.org/orgmode/f74fce15-c2ea-048e-b2ef-7ad40e717c89@arfer.net/

    In response, Bastien committed 3d0282ef8 to offer more control over
    when saving happens.

> When setting it to 't', the defcustom :tag string claims "Always 
> save the archive buffer".
> This is not the case if archiving from within the current buffer. 
> Perhaps a clearer :tag string?
>
> #+begin_src emacs-lisp
> (defcustom org-archive-subtree-save-file-p 'from-org
>   ;;...
>   (const :tag "Save the archive buffer unless it is the current 
>   buffer" t)
>   ;;...
>   ))
> #+end_src

Yeah, I'd say that's an improvement, though the same "unless it is the
current buffer" would be true for from-org as well, were it wired up.

> The value 'from-org also still saves the archive buffer when 
> archiving from a buffer that is not in Org mode.

I'm confused by this statement.  To me it looks like save-buffer will
_never_ be called when org-archive-subtree-save-file-p is from-org:

    (unless (eq this-buffer buffer)
      (when (or (eq org-archive-subtree-save-file-p t)
                (and (boundp 'org-archive-from-agenda)
                     (eq org-archive-subtree-save-file-p 'from-agenda)))
        (save-buffer)))

> I'm not entirely sure of its purpose. If the intent is to allow an 
> option that prevents saving only when archiving from the agenda,
> I suggest a single option excluding that case and saving for 
> other, non-nil values:

Based on the history above, I believe the main purpose is to give users
a way to reverse the "no saving" behavior made in 63f6e851b (Do not save
target buffer after archiving subtree, 2017-11-25).  I'm _guessing_
that, on top of that, the idea adding a from-agenda value was that some
users may want to save only when archiving from the agenda, because in
that case they're a bit removed from the buffer and might not think to
save it, or something along those lines.

> #+begin_src emacs-lisp
> (defcustom org-archive-subtree-save-file-p 'unless-agenda
>   "Conditionally save the archive file after archiving a subtree.
> The value 'unless-agenda prevents saving from the agenda-view.
> Other non-nil values save the archive buffer unless it is the 
> current buffer."
>   :group 'org-archive
>   :package-version '(Org . "9.4")
>   :type '(choice
>           (const :tag "Do not save archive buffer when archiving 
>           from an agenda view" unless-agenda)
>           (const :tag "Save the archive buffer unless it is the 
>           current buffer" t)
>           (const :tag "Do not save the archive buffer")))
> #+end_src
>
> Then the saving logic in org-archive-subtree becomes:
>
> #+begin_src emacs-lisp
> (when org-archive-subtree-save-file-p
>   (unless (or (eq buffer this-buffer)
>               (and (eq org-archive-subtree-save-file-p 
>               'unless-agenda)
>                    ;;bound when called from org-agenda.el
>                    (boundp 'org-archive-from-agenda)))
>     (save-buffer)))
> #+end_src

Assuming what I said above is true, I think what you propose here loses
the ability to save only when archiving from the agenda.  And more
importantly, users would not be able to give a blanket "don't save" in
order to retain the behavior introduced by 63f6e851b (Do not save target
buffer after archiving subtree, 2017-11-25).

Re: Bug: org-archive-subtree-save-file-p logic [9.3.6 (release_9.3.6-399-ge6df03 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

Kyle Meyer <kyle@kyleam.com> writes:

> Based on the history above, I believe the main purpose is to 
> give users
> a way to reverse the "no saving" behavior made in 63f6e851b (Do 
> not save
> target buffer after archiving subtree, 2017-11-25).  I'm 
> _guessing_
> that, on top of that, the idea adding a from-agenda value was 
> that some
> users may want to save only when archiving from the agenda, 
> because in
> that case they're a bit removed from the buffer and might not 
> think to
> save it, or something along those lines.

> Assuming what I said above is true, I think what you propose 
> here loses
> the ability to save only when archiving from the agenda.  And 
> more
> importantly, users would not be able to give a blanket "don't 
> save" in
> order to retain the behavior introduced by 63f6e851b (Do not 
> save target
> buffer after archiving subtree, 2017-11-25).


Thanks for the explanation, Kyle. I understand the intent of the 
variable better now.
What do you think of something like this?

#+begin_src emacs-lisp
(defcustom org-archive-subtree-save-file-p 'unless-agenda
  "Conditionally save the archive file after archiving a subtree.
The value 'unless-agenda prevents saving from the agenda-view.
The value 'only-agenda saves only when the archive is initiated 
from the agenda-view.
The value t saves in all cases where the archive target buffer is 
not the current buffer.
The value nil prevents saving in all cases."
  :group 'org-archive
  :package-version '(Org . "9.4")
  :type '(choice
          (const :tag "Do not save archive buffer when archiving 
          from an agenda view" unless-agenda)
          (const :tag "Only save archive buffer when archiving 
          from an agenda view"   only-agenda)
          (const :tag "Save the archive buffer unless it is the 
          current buffer" t)
          (const :tag "Do not save the archive buffer")))
#+end_src

#+begin_src emacs-lisp
;;don't save when target buffer is current buffer
(unless (eq buffer this-buffer)
  ;;t always saves
  (when (or (eq org-archive-subtree-save-file-p t)
            ;;'unless-agenda saves unless archive initiated from 
            agenda
            (and (eq org-archive-subtree-save-file-p 
            'unless-agenda)
                 (not (boundp 'org-archive-from-agenda)))
            ;;'only-agenda saves iff archive initiated from agenda
            (and (eq org-archive-subtree-save-file-p 'only-agenda)
                 (boundp 'org-archive-from-agenda)))
    (save-buffer)))
#+end_src

This should result in the following scenarios:

| org-archive-subtree-save-file-p | called-from-agenda? | buffer 
  saved? |
|---------------------------------+---------------------+---------------|
| t                               | nil                 | t 
  |
| t                               | t                   | t 
  |
| only-agenda                     | nil                 | nil 
  |
| only-agenda                     | t                   | t 
  |
| unless-agenda                   | nil                 | t 
  |
| unless-agenda                   | t                   | nil 
  |
| nil                             | nil                 | nil 
  |
| nil                             | t                   | nil 
  |

Re: Bug: org-archive-subtree-save-file-p logic [9.3.6 (release_9.3.6-399-ge6df03 @ /home/n/.emacs.d/straight/build/org/)]

[Kyle Meyer]

No Wayman <iarchivedmywholelife@gmail.com> writes:

> What do you think of something like this?

Thanks for the suggestion.  The code is somewhat oddly formatted, at
least on my end.  Could you send a proper git-format-patch output to
this thread (either via git-send-email or as an attachment)?

> #+begin_src emacs-lisp
> (defcustom org-archive-subtree-save-file-p 'unless-agenda
>   "Conditionally save the archive file after archiving a subtree.
> The value 'unless-agenda prevents saving from the agenda-view.

"an agenda view" sounds better to me and matches the original text from
3d0282ef8.

> The value 'only-agenda saves only when the archive is initiated 
> from the agenda-view.

nitpick: Please write symbols as `foo' rather than 'foo.

Also, if you're going to expand the docstring (not a bad thing), I think
it'd make sense to slim down the :tag text a bit.
org-archive-save-context-info provides a nice example of formatting the
docstring, including the list of values.

> The value t saves in all cases where the archive target buffer is 
> not the current buffer.
> The value nil prevents saving in all cases."
>   :group 'org-archive
>   :package-version '(Org . "9.4")
>   :type '(choice
>           (const :tag "Do not save archive buffer when archiving 
>           from an agenda view" unless-agenda)
>           (const :tag "Only save archive buffer when archiving 
>           from an agenda view"   only-agenda)

As I mentioned above, I have a slight preference for sticking with
3d0282ef8's names: from-org and from-agenda.  I suppose the main
argument against from-org is that it's not clear from the name alone
that it's referring to a non-agenda Org buffer because "org" is of
course a bit overloaded.  Considered alongside from-agenda, I don't
think it's too bad though.

>           (const :tag "Save the archive buffer unless it is the 
>           current buffer" t)

This current buffer bit also applies to unless-agenda/from-org.  Perhaps
it'd make sense to just mention the current buffer behavior in the main
docstring, given it applies to all options (even though for
only-agenda/from-agenda, it's never the case that the archive buffer is
the current buffer).

In summary

  * I'd prefer to make a more minimal change on top of 3d0282ef8,
    keeping the names chosen there.  Functionally that comes down to
    adjusting the condition that guards the save-buffer call to consider
    from-org.

  * I think it'd be good to expand the docstring (along the lines of
    what you suggested) as well as trim and clarify the tag text a bit.

Thoughts?

Re: Bug: org-archive-subtree-save-file-p logic [9.3.6 (release_9.3.6-399-ge6df03 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

Kyle Meyer <kyle@kyleam.com> writes:

> Thanks for the suggestion.  The code is somewhat oddly 
> formatted, at
> least on my end.  Could you send a proper git-format-patch 
> output to
> this thread (either via git-send-email or as an attachment)?

Apologies, I pasted that from an Org buffer without reformatting.
Attaching a proper patch with your suggested revisions here.

> I suppose the main argument against from-org is that it's not 
> clear
> from the name alone that it's referring to a non-agenda Org 
> buffer
> because "org" is of course a bit overloaded. Considered 
> alongside
> from-agenda, I don't think it's too bad though.
>

I agree. 'from-org is pretty vague. That, combined with the old 
saving logic was
part of the reason for my initial confusion.
I don't mind keeping it if you feel it's satisfactory, though.
I'm more after the proper functionality.

> This current buffer bit also applies to unless-agenda/from-org. 
> Perhaps
> it'd make sense to just mention the current buffer behavior in 
> the main
> docstring, given it applies to all options (even though for
> only-agenda/from-agenda, it's never the case that the archive 
> buffer is
> the current buffer).

I've dropped the mention of the current-buffer case in the 
docstring altogether.

> In summary
>
>   * I'd prefer to make a more minimal change on top of 
>   3d0282ef8,
>     keeping the names chosen there.  Functionally that comes 
>     down to
>     adjusting the condition that guards the save-buffer call to 
>     consider
>     from-org.
>
>   * I think it'd be good to expand the docstring (along the 
>   lines of
>     what you suggested) as well as trim and clarify the tag text 
>     a bit.

Took a look at `org-archive-save-context-info' as you suggested. 
The new docstring is similar to that.



===File 
/home/n/.emacs.d/straight/repos/org/0001-org-archive.el-fix-org-archive-subtree-save-file-p.patch===
From 289d3ff93c9f7f56ee54d98fd7d6294c4472a37b Mon Sep 17 00:00:00 
2001
From: Nicholas Vollmer <iarchivedmywholelife@gmail.com>
Subject: [PATCH] org-archive.el: fix 
org-archive-subtree-save-file-p

Consider case of 'from-org setting in saving logic.
Improve docstring.
Remove dead code comment.
---
 lisp/org-archive.el | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/lisp/org-archive.el b/lisp/org-archive.el
index 10a5eb501..5e11c3743 100644
--- a/lisp/org-archive.el
+++ b/lisp/org-archive.el
@@ -92,14 +92,20 @@ When a string, a %s formatter will be replaced 
by the file name."
 	  (const :tag "Always" t)))

 (defcustom org-archive-subtree-save-file-p 'from-org
-  "Non-nil means save the archive file after archiving a 
   subtree."
+  "Conditionally save the archive file after archiving a subtree.
+This variable can be any of the following symbols:
+
+t              saves in all cases.
+`from-org'     prevents saving from an agenda-view.
+`from-agenda'  saves only when the archive is initiated from an 
agenda-view.
+nil            prevents saving in all cases."
   :group 'org-archive
   :package-version '(Org . "9.4")
   :type '(choice
-	  (const :tag "Always save the archive buffer" t)
-	  (const :tag "Save target buffer when archiving from an agenda 
     view" from-agenda)
-	  (const :tag "Save target buffer when archiving from an org 
     buffer" from-org)
-	  (const :tag "Do not save the archive buffer")))
+         (const :tag "from-org"    from-org)
+         (const :tag "from-agenda" from-agenda)
+         (const :tag "t" t)
+         (const :tag "nil")))

 (defcustom org-archive-save-context-info '(time file olpath 
 category todo itags)
   "Parts of context info that should be stored as properties when 
   archiving.
@@ -373,14 +379,13 @@ direct children of this heading."
 		     value))))
 	      ;; Save and kill the buffer, if it is not the same
 	      ;; buffer and depending on 
 `org-archive-subtree-save-file-p'
-	      (unless (eq this-buffer buffer)
-		(when (or (eq org-archive-subtree-save-file-p t)
-			  (and (boundp 'org-archive-from-agenda)
-			       (eq org-archive-subtree-save-file-p 'from-agenda)))
-		  (save-buffer)))
-	      ;; (unless (or (not org-archive-subtree-save-file-p)
-	      ;; 		  (eq this-buffer buffer))
-	      ;; 	(save-buffer))
+              (unless (eq this-buffer buffer)
+                (when (or (eq org-archive-subtree-save-file-p t)
+                          (and (boundp 'org-archive-from-agenda)
+                               (eq 
org-archive-subtree-save-file-p 'from-agenda))
+                          (and (not (boundp 
'org-archive-from-agenda))
+                               (eq 
org-archive-subtree-save-file-p 'from-org)))
+                  (save-buffer)))
 	      (widen))))
 	;; Here we are back in the original buffer.  Everything seems
 	;; to have worked.  So now run hooks, cut the tree and finish
--
2.26.0

============================================================

Re: Bug: org-archive-subtree-save-file-p logic [9.3.6 (release_9.3.6-399-ge6df03 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

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


No Wayman <iarchivedmywholelife@gmail.com> writes:

> Attaching a proper patch with your suggested revisions here.

Sorry, I'm not used to submitting patches through email.
I see it inlined on my end, but it does not appear as if it was 
attached.
Trying once more:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix-org-archive-subtree-save-file-p --]
[-- Type: text/x-patch, Size: 2984 bytes --]

From 289d3ff93c9f7f56ee54d98fd7d6294c4472a37b Mon Sep 17 00:00:00 2001
From: Nicholas Vollmer <iarchivedmywholelife@gmail.com>
Date: Tue, 7 Apr 2020 14:39:29 -0400
Subject: [PATCH] org-archive.el: fix org-archive-subtree-save-file-p

Consider case of 'from-org setting in saving logic.
Improve docstring.
Remove dead code comment.
---
 lisp/org-archive.el | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/lisp/org-archive.el b/lisp/org-archive.el
index 10a5eb501..5e11c3743 100644
--- a/lisp/org-archive.el
+++ b/lisp/org-archive.el
@@ -92,14 +92,20 @@ When a string, a %s formatter will be replaced by the file name."
 	  (const :tag "Always" t)))
 
 (defcustom org-archive-subtree-save-file-p 'from-org
-  "Non-nil means save the archive file after archiving a subtree."
+  "Conditionally save the archive file after archiving a subtree.
+This variable can be any of the following symbols:
+
+t              saves in all cases.
+`from-org'     prevents saving from an agenda-view.
+`from-agenda'  saves only when the archive is initiated from an agenda-view.
+nil            prevents saving in all cases."
   :group 'org-archive
   :package-version '(Org . "9.4")
   :type '(choice
-	  (const :tag "Always save the archive buffer" t)
-	  (const :tag "Save target buffer when archiving from an agenda view" from-agenda)
-	  (const :tag "Save target buffer when archiving from an org buffer" from-org)
-	  (const :tag "Do not save the archive buffer")))
+         (const :tag "from-org"    from-org)
+         (const :tag "from-agenda" from-agenda)
+         (const :tag "t" t)
+         (const :tag "nil")))
 
 (defcustom org-archive-save-context-info '(time file olpath category todo itags)
   "Parts of context info that should be stored as properties when archiving.
@@ -373,14 +379,13 @@ direct children of this heading."
 		     value))))
 	      ;; Save and kill the buffer, if it is not the same
 	      ;; buffer and depending on `org-archive-subtree-save-file-p'
-	      (unless (eq this-buffer buffer)
-		(when (or (eq org-archive-subtree-save-file-p t)
-			  (and (boundp 'org-archive-from-agenda)
-			       (eq org-archive-subtree-save-file-p 'from-agenda)))
-		  (save-buffer)))
-	      ;; (unless (or (not org-archive-subtree-save-file-p)
-	      ;; 		  (eq this-buffer buffer))
-	      ;; 	(save-buffer))
+              (unless (eq this-buffer buffer)
+                (when (or (eq org-archive-subtree-save-file-p t)
+                          (and (boundp 'org-archive-from-agenda)
+                               (eq org-archive-subtree-save-file-p 'from-agenda))
+                          (and (not (boundp 'org-archive-from-agenda))
+                               (eq org-archive-subtree-save-file-p 'from-org)))
+                  (save-buffer)))
 	      (widen))))
 	;; Here we are back in the original buffer.  Everything seems
 	;; to have worked.  So now run hooks, cut the tree and finish
-- 
2.26.0

Re: Bug: org-archive-subtree-save-file-p logic [9.3.6 (release_9.3.6-399-ge6df03 @ /home/n/.emacs.d/straight/build/org/)]

[Kyle Meyer]

No Wayman <iarchivedmywholelife@gmail.com> writes:

> Subject: [PATCH] org-archive.el: fix org-archive-subtree-save-file-p

I've applied your patch with a few tweaks in 374cb4b41.

Note that you're approaching the "tiny change" limit.  In order to be
able to make future contributions, please consider the signing the FSF
copyright papers.  More information is at
<https://orgmode.org/worg/org-contribute.html>.

Thanks for the report and the fix!