Setting org-todo-keywords through directory-local variables

Setting org-todo-keywords through directory-local variables

[Kévin Le Gouguec]

Hello,

I'd like to set org-todo-keywords and org-todo-keyword-faces through
directory-local variables, to get rid of duplicate #+SEQ_TODO lines in
my Org files[1].

Right now I see two obstacles for this to work:

(1) org-set-regexps-and-options, which sets up a bunch of TODO-related
    machinery, insists on using (default-value 'org-todo-keywords),

(2) this function is called in the major mode function, which IIUC means
    that directory-local values have not been applied yet.

The first obstacle looks like it can be easily removed[2]; the second
obstacle looks more substantial.  It is trivially side-stepped by
sticking (hack-local-variables) at the top of org-mode; to my untrained
eye, it looks like TRT would rather be for Org to add
org-set-regexps-and-options to hack-local-variables-hook.

This sounds like a risky change though: I imagine that a lot of what
happens in the major mode function depends on what
org-set-regexps-and-options sets up, and would therefore need to be
moved to this hook as well.  Figuring which parts should be moved seems
like a non-trivial task that might introduce some regressions…


Can anyone confirm that this would (in principle) be the way forward, or
tell me if I am missing something[3]?


Thank you for your time.


[1] For example:

#+begin_src elisp
((org-mode
  . ((org-todo-keywords
      . ((sequence "REPORT" "REPORTED" "WAITING" "FIXED")
         (sequence "CANCELED")))
     (org-todo-keyword-faces
      . (("REPORT" . org-todo)
         ("REPORTED" . warning)
         ("WAITING" . warning)
         ("FIXED" . org-done)
         ("CANCELED" . shadow))))))
#+end_src

I'd like that so much that I went through the trouble of writing
safe-local-variable predicates for these variables:

#+begin_src elisp
(put 'org-todo-keywords
     'safe-local-variable
     (lambda (x)
       (cl-every
        (lambda (seq)
          (and (memq (car seq) '(sequence type))
               (cl-every (lambda (i) (stringp i)) (cdr seq))))
        x)))

(put 'org-todo-keyword-faces
     'safe-local-variable
     (lambda (x)
       (cl-every
        (lambda (pair)
          (pcase pair
            (`(,keyword . ,face)
             (and (stringp keyword)
                  (or (facep face) (listp face))))))
        x)))
#end_src

[2] I tried to go through org.el's history, but I could not find a
rationale for using default-value.

[3] Alternatively, maybe the answer is as simple as "Org documents
should be self-sufficient; keywords should be explicitly set in
#+SEQ_TODO lines"; that wouldn't sound right though, since
org-todo-keywords is customizable.

Re: Setting org-todo-keywords through directory-local variables

[Kévin Le Gouguec]

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

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> Can anyone confirm that this would (in principle) be the way forward, or
> tell me if I am missing something[3]?

I went ahead and cooked up a proof-of-concept patch, which

(1) adds safe-local-variable properties to org-todo-keywords and
    org-todo-keyword-faces,

(2) stops applying default-value to org-todo-keywords,

(3) delays regexps/font-lock setups until after file- and dir-local
    variables have been set.

While this patch contains a few things that make me weary[1], it solves
my use-case, and passes the current test suite with Emacs 26.3 and 28.

Does this look sound overall?  Does anyone have any idea what kind of
breakage might be slipping through the test suite?

Thank you for your time.


[1] - It's hard to feel confident that moving org-regexps-and-options
      and org-set-font-lock-defaults like this will not introduce
      regressions.

    - Also, these safe-local-variable validation functions could
      probably use some unit tests.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: dir-local-todo-keywords.patch --]
[-- Type: text/x-patch, Size: 3275 bytes --]

diff --git a/lisp/org-faces.el b/lisp/org-faces.el
index d78b606ec..544563276 100644
--- a/lisp/org-faces.el
+++ b/lisp/org-faces.el
@@ -291,7 +291,15 @@ determines if it is a foreground or a background color."
 	   (string :tag "Keyword")
 	   (choice :tag "Face   "
 		   (string :tag "Color")
-		   (sexp :tag "Face")))))
+		   (sexp :tag "Face"))))
+  :safe (lambda (x)
+          (cl-every
+           (lambda (pair)
+             (pcase pair
+               (`(,keyword . ,face)
+                (and (stringp keyword)
+                     (or (facep face) (listp face))))))
+           x)))
 
 (defface org-priority '((t :inherit font-lock-keyword-face))
   "Face used for priority cookies."
diff --git a/lisp/org.el b/lisp/org.el
index e577dc661..7f4672058 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -1945,7 +1945,13 @@ taken from the (otherwise obsolete) variable `org-todo-interpretation'."
 					 org-todo-interpretation-widgets))
 		      widget))
 		   (repeat
-		    (string :tag "Keyword"))))))
+		    (string :tag "Keyword")))))
+  :safe (lambda (x)
+          (cl-every
+           (lambda (seq)
+             (and (memq (car seq) '(sequence type))
+                  (cl-every (lambda (i) (stringp i)) (cdr seq))))
+           x)))
 
 (defvar-local org-todo-keywords-1 nil
   "All TODO and DONE keywords active in a buffer.")
@@ -4358,10 +4364,9 @@ related expressions."
 				     (cons 'sequence (split-string value)))
 				   (append (cdr (assoc "TODO" alist))
 					   (cdr (assoc "SEQ_TODO" alist)))))
-		   (let ((d (default-value 'org-todo-keywords)))
-		     (if (not (stringp (car d))) d
-		       ;; XXX: Backward compatibility code.
-		       (list (cons org-todo-interpretation d)))))))
+		   (if (not (stringp (car org-todo-keywords))) org-todo-keywords
+		     ;; XXX: Backward compatibility code.
+		     (list (cons org-todo-interpretation org-todo-keywords))))))
 	  (dolist (sequence todo-sequences)
 	    (let* ((sequence (or (run-hook-with-args-until-success
 				  'org-todo-setup-filter-hook sequence)
@@ -4801,8 +4806,6 @@ The following commands are available:
      (vconcat (mapcar (lambda (c) (make-glyph-code c 'org-ellipsis))
 		      org-ellipsis)))
     (setq buffer-display-table org-display-table))
-  (org-set-regexps-and-options)
-  (org-set-font-lock-defaults)
   (when (and org-tag-faces (not org-tags-special-faces-re))
     ;; tag faces set outside customize.... force initialization.
     (org-set-tag-faces 'org-tag-faces org-tag-faces))
@@ -4909,7 +4912,16 @@ The following commands are available:
   ;; Try to set `org-hide' face correctly.
   (let ((foreground (org-find-invisible-foreground)))
     (when foreground
-      (set-face-foreground 'org-hide foreground))))
+      (set-face-foreground 'org-hide foreground)))
+
+  ;; For file-visiting buffers, delay some setup until after
+  ;; file-local and directory-local variables have been set.
+  (if (buffer-file-name)
+      (progn
+	(add-hook 'hack-local-variables-hook 'org-set-regexps-and-options 1 t)
+	(add-hook 'hack-local-variables-hook 'org-set-font-lock-defaults 1 t))
+    (org-set-regexps-and-options)
+    (org-set-font-lock-defaults)))
 
 ;; Update `customize-package-emacs-version-alist'
 (add-to-list 'customize-package-emacs-version-alist

Re: Setting org-todo-keywords through directory-local variables

[Nicolas Goaziou]

Hello,

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:
>
>> Can anyone confirm that this would (in principle) be the way forward, or
>> tell me if I am missing something[3]?
>
> I went ahead and cooked up a proof-of-concept patch, which
>
> (1) adds safe-local-variable properties to org-todo-keywords and
>     org-todo-keyword-faces,
>
> (2) stops applying default-value to org-todo-keywords,
>
> (3) delays regexps/font-lock setups until after file- and dir-local
>     variables have been set.
>
> While this patch contains a few things that make me weary[1], it solves
> my use-case, and passes the current test suite with Emacs 26.3 and 28.
>
> Does this look sound overall?  Does anyone have any idea what kind of
> breakage might be slipping through the test suite?

This looks hackish. Also, Org needs the STARTUP part early on, so you
cannot really delay it.

I /think/ the rest can be delayed, but I admit I'm not too sure either.
I think the expected way to do this is to add a SETUPFILE.

Regards,

-- 
Nicolas Goaziou

Re: Setting org-todo-keywords through directory-local variables

[Kévin Le Gouguec]

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

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> This looks hackish.

Any bit in particular?  AFAICT hack-local-variables-hook is the expected
way to perform plumbing that might be affected by file/directory-local
variables.  It is used by e.g. shell-script-mode, cc-mode, markdown-mode
and AUCTeX, to name a few.  The docstring says:

> Major modes can use this to examine user-specified local variables
> in order to initialize other data structure based on them.

I think the buffer-file-name bit looks dodgy; I mainly did that to get
the unit tests to pass on this POC.

>                     Also, Org needs the STARTUP part early on, so you
> cannot really delay it.
>
> I /think/ the rest can be delayed, but I admit I'm not too sure either.

Right.  Now that I've looked at other major modes (especially
AUCTeX[1]), it seems a more conventional approach would be to

- keep the calls to org-set-regexps-and-options and
  org-set-font-lock-defaults where they are now,

- in hack-local-variables-hook, *if* file-local-variables-alist contains
  Org variables that affect those functions, and call them again to
  refresh regexps and fontification.

IIUC this would pretty much guarantee that things can only break for
weirdos like me who try to use directory-local variables.

> I think the expected way to do this is to add a SETUPFILE.

Thanks for the pointer!  Unless I'm misreading (info "(org) In-buffer
Settings"), I could move my SEQ_TODO settings there, but that wouldn't
work for org-todo-keyword-faces, right?


In light of your comments, and based on what I've seen in AUCTeX, I'm
attaching what I believe to be a less intrusive patch.  WDYT?


Thank you for taking the time to review this.  I'm not opposed to using
SETUPFILE (if I can handle org-todo-keyword-faces there); I'm just
wondering if this could be one more opportunity to have Org play nice
with other Emacs facilities (directory-local variables).


[1] https://git.savannah.gnu.org/cgit/auctex.git/tree/font-latex.el?h=release_12_2#n1435



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: dir-local-todo-keywords.patch --]
[-- Type: text/x-patch, Size: 2850 bytes --]

diff --git a/lisp/org-faces.el b/lisp/org-faces.el
index d78b606ec..fc834f37d 100644
--- a/lisp/org-faces.el
+++ b/lisp/org-faces.el
@@ -291,7 +291,15 @@ determines if it is a foreground or a background color."
 	   (string :tag "Keyword")
 	   (choice :tag "Face   "
 		   (string :tag "Color")
-		   (sexp :tag "Face")))))
+		   (sexp :tag "Face"))))
+  :safe (lambda (x)
+          (cl-every
+           (lambda (pair)
+	     (let ((keyword (car pair))
+		   (face (cdr pair)))
+	       (and (stringp keyword)
+		    (or (facep face) (listp face)))))
+           x)))
 
 (defface org-priority '((t :inherit font-lock-keyword-face))
   "Face used for priority cookies."
diff --git a/lisp/org.el b/lisp/org.el
index e577dc661..da38beb45 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -1945,7 +1945,13 @@ taken from the (otherwise obsolete) variable `org-todo-interpretation'."
 					 org-todo-interpretation-widgets))
 		      widget))
 		   (repeat
-		    (string :tag "Keyword"))))))
+		    (string :tag "Keyword")))))
+  :safe (lambda (x)
+          (cl-every
+           (lambda (seq)
+             (and (memq (car seq) '(sequence type))
+                  (cl-every (lambda (i) (stringp i)) (cdr seq))))
+           x)))
 
 (defvar-local org-todo-keywords-1 nil
   "All TODO and DONE keywords active in a buffer.")
@@ -4358,10 +4364,9 @@ related expressions."
 				     (cons 'sequence (split-string value)))
 				   (append (cdr (assoc "TODO" alist))
 					   (cdr (assoc "SEQ_TODO" alist)))))
-		   (let ((d (default-value 'org-todo-keywords)))
-		     (if (not (stringp (car d))) d
-		       ;; XXX: Backward compatibility code.
-		       (list (cons org-todo-interpretation d)))))))
+		   (if (not (stringp (car org-todo-keywords))) org-todo-keywords
+		     ;; XXX: Backward compatibility code.
+		     (list (cons org-todo-interpretation org-todo-keywords))))))
 	  (dolist (sequence todo-sequences)
 	    (let* ((sequence (or (run-hook-with-args-until-success
 				  'org-todo-setup-filter-hook sequence)
@@ -4909,7 +4914,18 @@ The following commands are available:
   ;; Try to set `org-hide' face correctly.
   (let ((foreground (org-find-invisible-foreground)))
     (when foreground
-      (set-face-foreground 'org-hide foreground))))
+      (set-face-foreground 'org-hide foreground)))
+
+  (add-hook 'hack-local-variables-hook #'org--process-local-variables nil t))
+
+(defun org--process-local-variables ()
+  "Refresh settings affected by file-local or directory-local variables."
+  (when
+      (let ((local-vars (mapcar #'car file-local-variables-alist)))
+	(or (memq 'org-todo-keywords local-vars)
+	    (memq 'org-todo-keyword-faces local-vars)))
+    (org-set-regexps-and-options)
+    (org-set-font-lock-defaults)))
 
 ;; Update `customize-package-emacs-version-alist'
 (add-to-list 'customize-package-emacs-version-alist

Re: Setting org-todo-keywords through directory-local variables

[Kévin Le Gouguec]

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

Hello,

I would like to re-submit this idea, now that I am reasonably sure that
my implementation will not impact users who do not use the feature.

(I understand that Org 9.4 is on the way.  I am not asking for this
feature to be included right now; I would like to get some feedback
first, then move on to documenting it.)

* Motivation

To recap: AFAIK it is not possible to define both org-todo-keywords and
org-todo-keyword-faces per-directory.  The former can be set with
#+SETUPFILE, but the latter simply can't be set locally, unless I'm
mistaken.

I'd like to specify, for all Org files in a directory, which keywords to
use and how they must look.  Setting both org-todo-* variables in
.dir-locals.el would be ideal IMO: the definitions for keywords and
their faces would be right next to each other.

This cannot work right now because (1) of a stray call to default-value
(2) Org computes the TODO machinery (regexps and font-lock) when setting
up the major mode, before directory-local variables are in effect.

* Prior art

AUCTeX[1] and markdown-mode[2] both solve this using
hack-local-variables-hook.  This seems to be the expected way for modes
to (re)compute things that may be affected by file or directory-local
variables.

[1] http://git.savannah.gnu.org/cgit/auctex.git/tree/font-latex.el?h=release_12_2#n1331
[2] https://github.com/jrblevin/markdown-mode/blob/v2.4/markdown-mode.el#L9403

The idea is to register a function that will check whether the user
overloaded variables we care about; if that's the case, then we
recompute what we need to.

* Patch

The attached patch:

- does not change org-mode's default setup logic,

- adds a function to hack-local-variables-hook that will look for
  org-todo-* variables, and recompute org-set-regexps-and-options and
  org-set-font-lock-defaults if needed,

- adds :safe predicates for these variables,

- adds unit tests.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Allow-users-to-configure-TODO-keywords-from-dir-loca.patch --]
[-- Type: text/x-patch, Size: 6926 bytes --]

From 148c5fa45e1fb8d58ecc86bb266d0fa33b8994a6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Wed, 27 May 2020 22:53:56 +0200
Subject: [PATCH] Allow users to configure TODO keywords from dir-locals.el

This uses the same method as AUCTeX and markdown-mode to refresh
fontification based on file-local and directory-local variables:

http://git.savannah.gnu.org/cgit/auctex.git/tree/font-latex.el?h=release_12_2#n1331
https://github.com/jrblevin/markdown-mode/blob/v2.4/markdown-mode.el#L9403

* lisp/org-faces.el (org-todo-keyword-faces): Add safe-local-variable
predicate.

* lisp/org.el (org-todo-keywords): Add safe-local-variable predicate.
(org-set-regexps-and-options): Use buffer-local value of
org-todo-keywords.
(org-mode): Register a function to reset regexps and font-lock once
file-local variables have been applied.
(org--process-local-variables): Recompute regexps and font-lock if the
user set relevant variables.

* testing/examples/dir-locals/.dir-locals.el:
* testing/examples/dir-locals/todo.org: Support files for new tests.

* testing/lisp/test-org.el (test-org/dir-local-todo-keyword-faces):
(test-org/dir-local-todo-cycling): New tests.
---
 lisp/org-faces.el                          | 10 ++++++-
 lisp/org.el                                | 28 +++++++++++++++----
 testing/examples/dir-locals/.dir-locals.el | 11 ++++++++
 testing/examples/dir-locals/todo.org       |  8 ++++++
 testing/lisp/test-org.el                   | 32 ++++++++++++++++++++++
 5 files changed, 82 insertions(+), 7 deletions(-)
 create mode 100644 testing/examples/dir-locals/.dir-locals.el
 create mode 100644 testing/examples/dir-locals/todo.org

diff --git a/lisp/org-faces.el b/lisp/org-faces.el
index d78b606ec..fc834f37d 100644
--- a/lisp/org-faces.el
+++ b/lisp/org-faces.el
@@ -291,7 +291,15 @@ determines if it is a foreground or a background color."
 	   (string :tag "Keyword")
 	   (choice :tag "Face   "
 		   (string :tag "Color")
-		   (sexp :tag "Face")))))
+		   (sexp :tag "Face"))))
+  :safe (lambda (x)
+          (cl-every
+           (lambda (pair)
+	     (let ((keyword (car pair))
+		   (face (cdr pair)))
+	       (and (stringp keyword)
+		    (or (facep face) (listp face)))))
+           x)))
 
 (defface org-priority '((t :inherit font-lock-keyword-face))
   "Face used for priority cookies."
diff --git a/lisp/org.el b/lisp/org.el
index 4d46b4173..c0183dbff 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -1945,7 +1945,13 @@ taken from the (otherwise obsolete) variable `org-todo-interpretation'."
 					 org-todo-interpretation-widgets))
 		      widget))
 		   (repeat
-		    (string :tag "Keyword"))))))
+		    (string :tag "Keyword")))))
+  :safe (lambda (x)
+          (cl-every
+           (lambda (seq)
+             (and (memq (car seq) '(sequence type))
+                  (cl-every (lambda (i) (stringp i)) (cdr seq))))
+           x)))
 
 (defvar-local org-todo-keywords-1 nil
   "All TODO and DONE keywords active in a buffer.")
@@ -4358,10 +4364,9 @@ related expressions."
 				     (cons 'sequence (split-string value)))
 				   (append (cdr (assoc "TODO" alist))
 					   (cdr (assoc "SEQ_TODO" alist)))))
-		   (let ((d (default-value 'org-todo-keywords)))
-		     (if (not (stringp (car d))) d
-		       ;; XXX: Backward compatibility code.
-		       (list (cons org-todo-interpretation d)))))))
+		   (if (not (stringp (car org-todo-keywords))) org-todo-keywords
+		     ;; XXX: Backward compatibility code.
+		     (list (cons org-todo-interpretation org-todo-keywords))))))
 	  (dolist (sequence todo-sequences)
 	    (let* ((sequence (or (run-hook-with-args-until-success
 				  'org-todo-setup-filter-hook sequence)
@@ -4908,7 +4913,18 @@ The following commands are available:
   ;; Try to set `org-hide' face correctly.
   (let ((foreground (org-find-invisible-foreground)))
     (when foreground
-      (set-face-foreground 'org-hide foreground))))
+      (set-face-foreground 'org-hide foreground)))
+
+  (add-hook 'hack-local-variables-hook #'org--process-local-variables nil t))
+
+(defun org--process-local-variables ()
+  "Refresh settings affected by file-local or directory-local variables."
+  (when
+      (let ((local-vars (mapcar #'car file-local-variables-alist)))
+	(or (memq 'org-todo-keywords local-vars)
+	    (memq 'org-todo-keyword-faces local-vars)))
+    (org-set-regexps-and-options)
+    (org-set-font-lock-defaults)))
 
 ;; Update `customize-package-emacs-version-alist'
 (add-to-list 'customize-package-emacs-version-alist
diff --git a/testing/examples/dir-locals/.dir-locals.el b/testing/examples/dir-locals/.dir-locals.el
new file mode 100644
index 000000000..677eaca10
--- /dev/null
+++ b/testing/examples/dir-locals/.dir-locals.el
@@ -0,0 +1,11 @@
+((org-mode
+  . ((org-todo-keywords
+      . ((sequence "TODO" "|" "DONE")
+         (sequence "REPORT" "BUG" "KNOWNCAUSE" "|" "FIXED")
+         (sequence "|" "CANCELED")))
+     (org-todo-keyword-faces
+      . (("REPORT" . org-todo)
+         ("BUG" . warning)
+         ("KNOWNCAUSE" . warning)
+         ("FIXED" . org-done)
+         ("CANCELED" . shadow))))))
diff --git a/testing/examples/dir-locals/todo.org b/testing/examples/dir-locals/todo.org
new file mode 100644
index 000000000..cd06b5ebd
--- /dev/null
+++ b/testing/examples/dir-locals/todo.org
@@ -0,0 +1,8 @@
+#+Title: headings with TODO keywords set in .dir-locals.el
+* TODO heading
+* DONE heading
+* REPORT heading
+* BUG heading
+* KNOWNCAUSE heading
+* FIXED heading
+* CANCELED heading
diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index 375e1a718..2adcb2681 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -7158,6 +7158,38 @@ CLOCK: [2012-03-29 Thu 10:00]--[2012-03-29 Thu 16:40] =>  6:40"
 	(org-todo "DONE")
 	(buffer-string))))))
 
+(ert-deftest test-org/dir-local-todo-keyword-faces ()
+  "Make sure TODO faces honor dir-local variables."
+  (org-test-in-example-file
+      (expand-file-name "dir-locals/todo.org" org-test-example-dir)
+    (font-lock-ensure (point-min) (point-max))
+    (dolist (expected-face '(org-todo
+			     org-done
+			     org-todo
+			     warning
+			     warning
+			     org-done
+			     shadow))
+      (should (equal (get-text-property (+ 2 (point)) 'face)
+		     expected-face))
+      (next-line))))
+
+(ert-deftest test-org/dir-local-todo-cycling ()
+  "Make sure TODO cycling honors dir-local variables."
+  (org-test-in-example-file
+      (expand-file-name "dir-locals/todo.org" org-test-example-dir)
+    (dolist (expected-heading '("* DONE heading"
+				"* heading"
+				"* BUG heading"
+				"* KNOWNCAUSE heading"
+				"* FIXED heading"
+				"* heading"
+				"* heading"))
+      (org-todo)
+      (should (string= (buffer-substring (point) (line-end-position))
+		       expected-heading))
+      (next-line))
+    (revert-buffer t t)))
 \f
 ;;; Timestamps API
 
-- 
2.27.0


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


Am I on the right track with this patch, or are there problems I haven't
thought of?

Thank you for your time.

Re: Setting org-todo-keywords through directory-local variables

[Bastien]

Hi Kévin,

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> I would like to re-submit this idea, now that I am reasonably sure that
> my implementation will not impact users who do not use the feature.

FWIW it looks fine to me, thanks for hacking this together.

We are in feature freeze for 9.4 so let's wait until you can commit
this to master.

Best,

-- 
 Bastien

Re: Setting org-todo-keywords through directory-local variables

[Ihor Radchenko]

[sending to Org ML in-reply to the relevant thread]

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=57003
28.1.90; Can local variables be loaded before loading major mode?

> … reminded me of a patch I submitted to the Org ML… some time ago 😣
> (sorry for not following up) to set TODO keywords via .dir-locals.el:
>
> https://list.orgmode.org/87a70stkmv.fsf@gmail.com/

Your patch is not listed on https://updates.orgmode.org/
It is also not in my records (I am only following patches closely since
the beginning of this year).
So, it slipped through the cracks.
I am bumping it herein.

At least, the :safe marking is something we can merge right away.

> My rationale with this patch was that AUCTeX and markdown-mode both use
> hack-local-variables-hook successfully to (re)compute stuff from
> dir/file-locals; I figured Org…
>
> * should bite the bullet, at some point: it'd just be really neat for
>   Emacs users used to this feature,

Maybe. That's why this emacs-devel thread.

> * could do so piecemeal, adding support for variables one at a time as
>   people chime in the ML to express a need.

> E.g. my patch only added support for org-todo-keywords and
> org-todo-keyword-faces, but it laid the foundation for adding support
> for other variables later.

I'd prefer to solve it once and for all. I tried early loading of
file-local variables in the past, but had to revert the commit because
of major issues. See
https://list.orgmode.org/87r11wkmew.fsf@ucl.ac.uk/T/#mab6359ed2107d5515c6bb6b266551f0c5049ceca

Maybe the hook approach can work better. But I'd prefer to discuss all
the possible caveats first.

> Also to try to reduce the risk of breakage, it went for "compute Org
> settings normally; then selectively recompute some if relevant variables
> are found in dir/file-locals".  That way "regular" Org users who rely
> rather on SETUPFILEs wouldn't be impacted, only "early adopters" of
> dir/file-locals might shoot themselves in the foot.

I am not sure what is the problem with SETUPFILE.
We can simply load it in the hook. Though the priority of SETUPFILE vs.
local variables should be discussed. Probably, local variables should
take precedence to keep things consistent with the rest of Emacs.

> (Also it had tests 😊)

Tests are always welcome :)

> Anyhoo.  Not even sure the patch applies after two years, but the
> general approach might be worth looking into?

Sure.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Setting org-todo-keywords through directory-local variables

[Kévin Le Gouguec]

Ihor Radchenko <yantar92@posteo.net> writes:

> [sending to Org ML in-reply to the relevant thread]

[Thanks!]

> Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:
>
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=57003
> 28.1.90; Can local variables be loaded before loading major mode?
>
>> … reminded me of a patch I submitted to the Org ML… some time ago 😣
>> (sorry for not following up) to set TODO keywords via .dir-locals.el:
>>
>> https://list.orgmode.org/87a70stkmv.fsf@gmail.com/
>
> Your patch is not listed on https://updates.orgmode.org/
> It is also not in my records (I am only following patches closely since
> the beginning of this year).
> So, it slipped through the cracks.

(Right, that's entirely on me, I posted it knowing that an Org release
was pending and figuring I'd come back later…  well, better late than
never)

>> * could do so piecemeal, adding support for variables one at a time as
>>   people chime in the ML to express a need.
>
>> E.g. my patch only added support for org-todo-keywords and
>> org-todo-keyword-faces, but it laid the foundation for adding support
>> for other variables later.
>
> I'd prefer to solve it once and for all. I tried early loading of
> file-local variables in the past, but had to revert the commit because
> of major issues. See
> https://list.orgmode.org/87r11wkmew.fsf@ucl.ac.uk/T/#mab6359ed2107d5515c6bb6b266551f0c5049ceca
>
> Maybe the hook approach can work better. But I'd prefer to discuss all
> the possible caveats first.

My reasoning for keeping the current initialization code untouched and
_re_computing stuff in hack-local-variables-hook hinged on…

* refactoring being fraught; since Org already has a "blessed" way to do
  more or less what file/dir-locals do (SETUPFILEs), I figured it wasn't
  worth rocking everyone's boat for the benefit of the few,

* the prior art in other markup modes (markdown-handle-local-variables,
  font-latex-after-hacking-local-variables).

>> Also to try to reduce the risk of breakage, it went for "compute Org
>> settings normally; then selectively recompute some if relevant variables
>> are found in dir/file-locals".  That way "regular" Org users who rely
>> rather on SETUPFILEs wouldn't be impacted, only "early adopters" of
>> dir/file-locals might shoot themselves in the foot.
>
> I am not sure what is the problem with SETUPFILE.
> We can simply load it in the hook. 

I wasn't suggesting there's a problem with SETUPFILEs; my point was that
I considered two categories of users:

* those who are happy with SETUPFILEs: my implementation goal was to
  "guarantee" that my patch would have zero impact on them,

* those who want to play with dir/file-locals (👋): conversely, I wanted
  to make sure that only them would get to "pick up the pieces" when
  something would inevitably break.

This patch might have been my first foray into Org's init code, so it
felt too risky to go with any approach other than "keep the
implementation for the established features _exactly_ _as_ _now_; stuff
all the experimental stuff in hack-local-variables-hook".

>                                    Though the priority of SETUPFILE vs.
> local variables should be discussed. Probably, local variables should
> take precedence to keep things consistent with the rest of Emacs.

No strong opinions there.  I'm not even sure how my patch handled that?
🤔

> +(defun org--process-local-variables ()
> +  "Refresh settings affected by file-local or directory-local variables."
> +  (when
> +      (let ((local-vars (mapcar #'car file-local-variables-alist)))
> +	(or (memq 'org-todo-keywords local-vars)
> +	    (memq 'org-todo-keyword-faces local-vars)))
> +    (org-set-regexps-and-options)
> +    (org-set-font-lock-defaults)))

IIUC the logic goes org-set-regexps-and-options ⇒ org-collect-keywords ⇒
org--collect-keywords-1, and that's where SETUPFILE is processed?  So
currently the SETUPFILE would have priority 🙃 (unless I'm misreading
the code)

Re: Setting org-todo-keywords through directory-local variables

[Ihor Radchenko]

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

>> Maybe the hook approach can work better. But I'd prefer to discuss all
>> the possible caveats first.
>
> My reasoning for keeping the current initialization code untouched and
> _re_computing stuff in hack-local-variables-hook hinged on…

I would avoid re-computing staff. Some variables define Org parser setup
and re-computing is expensive when we need to reset the parser state. In
particular, it will make parser cache persistence useless.

> This patch might have been my first foray into Org's init code, so it
> felt too risky to go with any approach other than "keep the
> implementation for the established features _exactly_ _as_ _now_; stuff
> all the experimental stuff in hack-local-variables-hook".

I'd say that it is too early to consider local variable hooks.
Especially given that Emacs devs just suggested a better approach and
discouraged using hack-local-variables-hook. See
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=57003#108

What we can do wrt this patch is extract the part that marks some
variables as :safe. It will be a useful addition in any case.

For handling local variables, let's wait for the discussion with Emacs
devs to resolve.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Setting org-todo-keywords through directory-local variables

[Nicolas Goaziou]

Hello,

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> I'd like to set org-todo-keywords and org-todo-keyword-faces through
> directory-local variables, to get rid of duplicate #+SEQ_TODO lines in
> my Org files[1].
>
> Right now I see two obstacles for this to work:
>
> (1) org-set-regexps-and-options, which sets up a bunch of TODO-related
>     machinery, insists on using (default-value 'org-todo-keywords),
>
> (2) this function is called in the major mode function, which IIUC means
>     that directory-local values have not been applied yet.
>
> The first obstacle looks like it can be easily removed[2]; the second
> obstacle looks more substantial.  It is trivially side-stepped by
> sticking (hack-local-variables) at the top of org-mode; to my untrained
> eye, it looks like TRT would rather be for Org to add
> org-set-regexps-and-options to hack-local-variables-hook.
>
> This sounds like a risky change though: I imagine that a lot of what
> happens in the major mode function depends on what
> org-set-regexps-and-options sets up, and would therefore need to be
> moved to this hook as well.  Figuring which parts should be moved seems
> like a non-trivial task that might introduce some regressions…
>
>
> Can anyone confirm that this would (in principle) be the way forward, or
> tell me if I am missing something[3]?

Did you consider using SETUPFILE?

Regards,

-- 
Nicolas Goaziou