emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
@ 2022-06-11  7:50 Ignacio Casso
  2022-06-11 13:32 ` Ihor Radchenko
  0 siblings, 1 reply; 22+ messages in thread
From: Ignacio Casso @ 2022-06-11  7:50 UTC (permalink / raw)
  To: ignaciocasso; +Cc: emacs-orgmode, Max Nikulin, Tim Cross

Hello,

The bug I reported about this to the Emacs devel mailing list
(https://debbugs.gnu.org/cgi/bugreport.cgi?bug=54399) has now been
closed, and some documentation has been updated in commit 071722e41120.

Basically, the problem is that in order for

  (let ((custom-variable local-value) ...)
      ...
      (defcustom custom-variable standard-value ...)
      ...)

to work as expected with dynamic binding, `defcustom' has to set the
variable with `set-default-toplevel-value'. This way, the code inside
the `let' form uses local-value and the default value is set to
standard-value. If `defcustom' uses `set-default' instead, it overwrites
the local value, so the let-binding is useless, and after the let form
is evaluated the variable is void.

The problem was that `set-default-toplevel-value' is used by default,
but the documentation said that the default was `set-default'. So either
whoever wrote the `org-capture-templates' setter was misguided by the
documentation and used `set-default', or more likely, that setter was
written before the introduction of `set-default-toplevel-value' in
custom.el, and was never updated.

The first thing we should do is finding all uses of `set-default' in the
org `defcustom' setters and replace them with
`set-default-toplevel-value'. This would fix this bug and similar ones
when using dynamic binding, and would not affect any other use case.

Then we should decide if we want to use autoload cookies for custom
variables to make this work also with lexical binding. Otherwise, code
like the snippet above would produce an error in Emacs 29, and in Emacs
27 the let binding would be ignored (although at least the custom setter
would work). I have no opinion regarding this last point since I don't
remember what were the disadvantages of using autoload cookies for
custom variables.

I've prepared a patch for the first point, which I attach at the end of
this email. All changes fall in one of the following cases:

- `set-default' -> `set-default-toplevel-value' (as explained)

- `set' -> `set-default-toplevel-value'

  The same, but in this cases there was another bug: If a buffer set the
  custom variable locally before the feature was autoloaded, the setter
  of the variable would not set the standard value as the default for
  other buffers, and would overwrite the buffer-local value.

- :set 'set-default -> nothing, since it would be already the default

I don't really know what most of the variables whose setter I have
changed do or whether it makes any sense to use them inside a let
binding, but I have made the change for all of them nevertheless, since
it can not harm and could potentially fix a bug. Feel free to restrict
those changes only to those variables where it makes sense, such as
`org-capture-templates', if you want to keep the patch small and simple.

Best regards,

Ignacio

[2. patch --- text/x-diff; 0001-using-set-default-toplevel-value-in-defcustom-setter.patch]
From b20e23013329fab574a4d05eb5be8cde1d43dad1 Mon Sep 17 00:00:00 2001
From: Ignacio Casso <ignaciocasso@hotmail.com>
Date: Fri, 10 Jun 2022 12:39:43 +0200
Subject: [PATCH] using `set-default-toplevel-value' in `defcustom' setters

---
 lisp/ob-lilypond.el  |  2 +-
 lisp/ob-shell.el     |  2 +-
 lisp/org-capture.el  |  2 +-
 lisp/org-clock.el    |  2 +-
 lisp/org-duration.el |  2 +-
 lisp/org-faces.el    |  2 +-
 lisp/org-footnote.el |  2 +-
 lisp/org-list.el     |  4 ++--
 lisp/org.el          | 23 +++++++++++------------
 lisp/ox-odt.el       |  2 +-
 10 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/lisp/ob-lilypond.el b/lisp/ob-lilypond.el
index df128441a..dc33ebc17 100644
--- a/lisp/ob-lilypond.el
+++ b/lisp/ob-lilypond.el
@@ -107,7 +107,7 @@ you can leave the string empty on this case."
   :package-version '(Org . "8.2.7")
   :set
   (lambda (symbol value)
-    (set symbol value)
+    (set-default-toplevel-value symbol value)
     (setq
      org-babel-lilypond-ly-command   (nth 0 value)
      org-babel-lilypond-pdf-command  (nth 1 value)
diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index c25941a44..4454e3b5d 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -68,7 +68,7 @@ outside the Customize interface."
   :group 'org-babel
   :type '(repeat (string :tag "Shell name: "))
   :set (lambda (symbol value)
-	 (set-default symbol value)
+	 (set-default-toplevel-value symbol value)
 	 (org-babel-shell-initialize)))
 
 (defcustom org-babel-shell-results-defaults-to-output t
diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index 773234967..948eb8bc6 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -376,7 +376,7 @@ When you need to insert a literal percent sign in the template,
 you can escape ambiguous cases with a backward slash, e.g., \\%i."
   :group 'org-capture
   :package-version '(Org . "9.5")
-  :set (lambda (s v) (set s (org-capture-upgrade-templates v)))
+  :set (lambda (s v) (set-default-toplevel-value s (org-capture-upgrade-templates v)))
   :type
   (let ((file-variants '(choice :tag "Filename       "
 				(file :tag "Literal")
diff --git a/lisp/org-clock.el b/lisp/org-clock.el
index e0c40ae23..b94c79baa 100644
--- a/lisp/org-clock.el
+++ b/lisp/org-clock.el
@@ -493,7 +493,7 @@ This variable only has effect if set with \\[customize]."
          (if value
              (add-hook 'kill-emacs-query-functions #'org-clock-kill-emacs-query)
            (remove-hook 'kill-emacs-query-functions #'org-clock-kill-emacs-query))
-         (set symbol value))
+         (set-default-toplevel-value symbol value))
   :type 'boolean
   :package-version '(Org . "9.5"))
 
diff --git a/lisp/org-duration.el b/lisp/org-duration.el
index b242a6f2c..338ea11a9 100644
--- a/lisp/org-duration.el
+++ b/lisp/org-duration.el
@@ -98,7 +98,7 @@ sure to call the following command:
   :version "26.1"
   :package-version '(Org . "9.1")
   :set (lambda (var val)
-         (set-default var val)
+         (set-default-toplevel-value var val)
          ;; Avoid recursive load at startup.
 	 (when (featurep 'org-duration)
            (org-duration-set-regexps)))
diff --git a/lisp/org-faces.el b/lisp/org-faces.el
index f919a6b47..5fb6c3e07 100644
--- a/lisp/org-faces.el
+++ b/lisp/org-faces.el
@@ -338,7 +338,7 @@ determines if it is a foreground or a background color."
 
 (defvar org-tags-special-faces-re nil)
 (defun org-set-tag-faces (var value)
-  (set var value)
+  (set-default-toplevel-value var value)
   (if (not value)
       (setq org-tags-special-faces-re nil)
     (setq org-tags-special-faces-re
diff --git a/lisp/org-footnote.el b/lisp/org-footnote.el
index 0a5f994a4..8e0ac0da2 100644
--- a/lisp/org-footnote.el
+++ b/lisp/org-footnote.el
@@ -110,7 +110,7 @@ you will need to run the following command after the change:
   :group 'org-footnote
   :initialize 'custom-initialize-default
   :set (lambda (var val)
-	 (set var val)
+	 (set-default-toplevel-value var val)
 	 (when (fboundp 'org-element-cache-reset)
 	   (org-element-cache-reset 'all)))
   :type '(choice
diff --git a/lisp/org-list.el b/lisp/org-list.el
index 515763036..af560cff4 100644
--- a/lisp/org-list.el
+++ b/lisp/org-list.el
@@ -235,7 +235,7 @@ interface or run the following code after updating it:
   :type '(choice (const :tag "dot like in \"2.\"" ?.)
 		 (const :tag "paren like in \"2)\"" ?\))
 		 (const :tag "both" t))
-  :set (lambda (var val) (set var val)
+  :set (lambda (var val) (set-default-toplevel-value var val)
 	 (when (featurep 'org-element) (org-element-update-syntax))))
 
 (defcustom org-list-allow-alphabetical nil
@@ -253,7 +253,7 @@ interface or run the following code after updating it:
   :group 'org-plain-lists
   :version "24.1"
   :type 'boolean
-  :set (lambda (var val) (set var val)
+  :set (lambda (var val) (set-default-toplevel-value var val)
 	 (when (featurep 'org-element) (org-element-update-syntax))))
 
 (defcustom org-list-two-spaces-after-bullet-regexp nil
diff --git a/lisp/org.el b/lisp/org.el
index ac94fb614..c57ccf319 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -231,7 +231,7 @@ Stars are put in group 1 and the trimmed body in group 2.")
 ;;;###autoload
 (defun org-babel-do-load-languages (sym value)
   "Load the languages defined in `org-babel-load-languages'."
-  (set-default sym value)
+  (set-default-toplevel-value sym value)
   (dolist (pair org-babel-load-languages)
     (let ((active (cdr pair)) (lang (symbol-name (car pair))))
       (if active
@@ -706,7 +706,7 @@ defined in org-duration.el.")
 
 (defun org-set-modules (var value)
   "Set VAR to VALUE and call `org-load-modules-maybe' with the force flag."
-  (set var value)
+  (set-default-toplevel-value var value)
   (when (featurep 'org)
     (org-load-modules-maybe 'force)
     (org-element-cache-reset 'all)))
@@ -837,7 +837,7 @@ depends on, if any."
   :package-version '(Org . "9.0")
   :initialize 'custom-initialize-set
   :set (lambda (var val)
-	 (if (not (featurep 'ox)) (set-default var val)
+	 (if (not (featurep 'ox)) (set-default-toplevel-value var val)
 	   ;; Any back-end not required anymore (not present in VAL and not
 	   ;; a parent of any back-end in the new value) is removed from the
 	   ;; list of registered back-ends.
@@ -862,7 +862,7 @@ depends on, if any."
 			  backend))
 		((not (memq backend new-list)) (push backend new-list))))
 	     ;; Set VAR to that list with fixed dependencies.
-	     (set-default var new-list))))
+	     (set-default-toplevel-value var new-list))))
   :type '(set :greedy t
 	      (const :tag "   ascii       Export buffer to ASCII format" ascii)
 	      (const :tag "   beamer      Export buffer to Beamer presentation" beamer)
@@ -1815,9 +1815,9 @@ are followed by a letter in parenthesis, like TODO(t)."
   :group 'org-todo
   :set (lambda (var val)
 	 (cond
-	  ((eq var t) (set var 'auto))
-	  ((eq var 'prefix) (set var nil))
-	  (t (set var val))))
+	  ((eq var t) (set-default-toplevel-value var 'auto))
+	  ((eq var 'prefix) (set-default-toplevel-value var nil))
+	  (t (set-default-toplevel-value var val))))
   :type '(choice
 	  (const :tag "Never" nil)
 	  (const :tag "Automatically, when key letter have been defined" auto)
@@ -1899,7 +1899,7 @@ be blocked if any prior sibling is not yet done.
 Finally, if the parent is blocked because of ordered siblings of its own,
 the child will also be blocked."
   :set (lambda (var val)
-	 (set var val)
+	 (set-default-toplevel-value var val)
 	 (if val
 	     (add-hook 'org-blocker-hook
 		       'org-block-todo-from-children-or-siblings-or-parent)
@@ -1917,7 +1917,7 @@ This variable needs to be set before org.el is loaded, and you need to
 restart Emacs after a change to make the change effective.  The only way
 to change it while Emacs is running is through the customize interface."
   :set (lambda (var val)
-	 (set var val)
+	 (set-default-toplevel-value var val)
 	 (if val
 	     (add-hook 'org-blocker-hook
 		       'org-block-todo-from-checkboxes)
@@ -2368,7 +2368,6 @@ The formats are defined through the variable `org-time-stamp-custom-formats'.
 To turn this on on a per-file basis, insert anywhere in the file:
    #+STARTUP: customtime"
   :group 'org-time
-  :set 'set-default
   :type 'sexp)
 (make-variable-buffer-local 'org-display-custom-times)
 
@@ -3275,7 +3274,7 @@ header, or they will be appended."
 
 (defun org-set-packages-alist (var val)
   "Set the packages alist and make sure it has 3 elements per entry."
-  (set var (mapcar (lambda (x)
+  (set-default-toplevel-value var (mapcar (lambda (x)
 		     (if (and (consp x) (= (length x) 2))
 			 (list (car x) (nth 1 x) t)
 		       x))
@@ -3548,7 +3547,7 @@ After a match, the match groups contain these elements:
 (defvar org-emphasis-alist) ; defined just below
 (defun org-set-emph-re (var val)
   "Set variable and compute the emphasis regular expression."
-  (set var val)
+  (set-default-toplevel-value var val)
   (when (and (boundp 'org-emphasis-alist)
 	     (boundp 'org-emphasis-regexp-components)
 	     org-emphasis-alist org-emphasis-regexp-components)
diff --git a/lisp/ox-odt.el b/lisp/ox-odt.el
index aa6e90122..9b46f15b5 100644
--- a/lisp/ox-odt.el
+++ b/lisp/ox-odt.el
@@ -404,7 +404,7 @@ with GNU ELPA tar or standard Emacs distribution."
     "Set `org-odt-schema-dir'.
 Also add it to `rng-schema-locating-files'."
     (let ((schema-dir value))
-      (set var
+      (set-default-toplevel-value var
 	   (if (and
 		(file-expand-wildcards
 		 (expand-file-name "od-manifest-schema*.rnc" schema-dir))
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
@ 2022-03-10 12:53 Ignacio Casso
  2022-03-10 16:32 ` Max Nikulin
  0 siblings, 1 reply; 22+ messages in thread
From: Ignacio Casso @ 2022-03-10 12:53 UTC (permalink / raw)
  To: emacs-orgmode

Hello,

I think I've found a bug with org-capture autoload. If the first time
you use org-capture before it's actually loaded is within a let form binding
org-capture-templates, it produces an error saying that the template was
not found.

For example, if we call this:

  (let ((org-capture-templates
         '(("d" "default" entry
            (file+headline org-default-notes-file "Tasks")
            "* %?"))))
    (org-capture nil "d")))

It produces the error:

  (error "No capture template referred to by \"d\" keys")


Furthermore, depending on where that form was evaluated or defined, the
next time you called org-capture may work or produce the following
error:

  org-capture: Symbol’s value as variable is void: org-capture-templates

In particular, if it is evaluated directly in the scratch buffer the
next call to org-capture works, and if it is part of a function
defined in my init file and evaluated in the scratch buffer, it produces
the error.

I have tried this too in Emacs 29 and the behaviour is similar, but
sometimes the error produced is

  (error "Defining as dynamic an already lexical var")

instead of

  (error "No capture template referred to by \"d\" keys")

This again depends on where the form is evaluated or defined.


Best regards,

Ignacio

P.S., I now this has an easy solution: not autoloading org-capture and having
(require 'org-capture) in my config. I'm just reporting the bug, if that
is what this is.

Emacs  : GNU Emacs 27.2 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.20)
 of 2022-01-16
Package: Org mode version 9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)


^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2022-06-14 13:58 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-11  7:50 [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)] Ignacio Casso
2022-06-11 13:32 ` Ihor Radchenko
2022-06-11 17:25   ` Ignacio Casso
2022-06-14  4:02     ` Ihor Radchenko
2022-06-14  7:49       ` Ignacio Casso
2022-06-14 13:58         ` Ihor Radchenko
2022-06-12 12:36   ` Max Nikulin
  -- strict thread matches above, loose matches on Subject: below --
2022-03-10 12:53 Ignacio Casso
2022-03-10 16:32 ` Max Nikulin
2022-03-10 18:00   ` Ignacio Casso
2022-03-11 10:07     ` Max Nikulin
2022-03-11 10:38       ` Ignacio Casso
2022-03-11 19:59         ` Tim Cross
2022-03-14 10:42           ` Ignacio Casso
2022-03-14 14:52           ` Max Nikulin
2022-03-14 18:42             ` Tim Cross
2022-03-14 19:43               ` Ignacio Casso
2022-03-14 22:54                 ` Tim Cross
2022-03-15  9:02                   ` Ignacio Casso
2022-03-15 15:59                     ` Ignacio Casso
2022-03-15 12:04                 ` Max Nikulin
2022-03-15 12:26                   ` Ignacio Casso

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).