bug#44647: 27.1.50; `bibtex-contline-indentation' doesn't work as file local variable

bug#44647: 27.1.50; `bibtex-contline-indentation' doesn't work as file local variable

[Teemu Likonen]

[-- Attachment #1.1: Type: text/plain, Size: 393 bytes --]

Variable bibtex-contline-indentation does not work as file local
variable. The reason is that bibtex-mode command initializes variable
fill-prefix before the possible file local variable is available. It
gets always the global value of bibtex-contline-indentation.

This can be fixed by locally let-binding fill-prefix every time in the
relevant filling function. Patch for that is attached.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Make-bibtex-contline-indentation-work-as-file-local-.patch --]
[-- Type: text/x-diff, Size: 2057 bytes --]

From a776cae4fcd34987e30b6eab3df45bd2ae66fbd9 Mon Sep 17 00:00:00 2001
From: Teemu Likonen <tlikonen@iki.fi>
Date: Sat, 14 Nov 2020 22:53:18 +0200
Subject: [PATCH] Make `bibtex-contline-indentation' work as file local
 variable

* lisp/textmodes/bibtex.el (bibtex-mode): Don't make fill-prefix a
buffer local variable. It would use the global value, not possible
file local variable.
* lisp/textmodes/bibtex.el (bibtex-fill-field-bounds): Let-bind
fill-prefix variable locally (and dynamically) for use with filling
bibtex fields. The variable is initialized every time by using the
current value of bibtex-contline-indentation variable.
---
 lisp/textmodes/bibtex.el | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lisp/textmodes/bibtex.el b/lisp/textmodes/bibtex.el
index fcf63ed5ec..151dca6fab 100644
--- a/lisp/textmodes/bibtex.el
+++ b/lisp/textmodes/bibtex.el
@@ -3442,8 +3442,6 @@ bibtex-mode
   (set (make-local-variable 'defun-prompt-regexp) "^[ \t]*@[[:alnum:]]+[ \t]*")
   (set (make-local-variable 'outline-regexp) "[ \t]*@")
   (set (make-local-variable 'fill-paragraph-function) #'bibtex-fill-field)
-  (set (make-local-variable 'fill-prefix)
-       (make-string (+ bibtex-entry-offset bibtex-contline-indentation) ?\s))
   (set (make-local-variable 'font-lock-defaults)
        '(bibtex-font-lock-keywords
          nil t ((?$ . "\"")
@@ -4902,7 +4900,10 @@ bibtex-fill-field-bounds
   "Fill BibTeX field delimited by BOUNDS.
 If JUSTIFY is non-nil justify as well.
 If optional arg MOVE is non-nil move point to end of field."
-  (let ((end-field (copy-marker (bibtex-end-of-field bounds))))
+  (let ((end-field (copy-marker (bibtex-end-of-field bounds)))
+        (fill-prefix (make-string (+ bibtex-entry-offset
+                                     bibtex-contline-indentation)
+                                  ?\s)))
     (if (not justify)
         (goto-char (bibtex-start-of-text-in-field bounds))
       (goto-char (bibtex-start-of-field bounds))
-- 
2.20.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 251 bytes --]

bug#44647: 27.1.50; `bibtex-contline-indentation' doesn't work as file local variable

[Teemu Likonen]

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

Hello Roland!

I found and fixed a bug in Emacs bibtex.el file. Here's my description:

* 2020-11-14 23:18:16+02, Teemu Likonen wrote:
> Variable bibtex-contline-indentation does not work as file local
> variable. The reason is that bibtex-mode command initializes variable
> fill-prefix before the possible file local variable is available. It
> gets always the global value of bibtex-contline-indentation.
>
> This can be fixed by locally let-binding fill-prefix every time in the
> relevant filling function. Patch for that is attached.

See: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=44647

What do you think of the patch which I attached to the bug report?

-- 
/// Teemu Likonen - .-.. https://www.iki.fi/tlikonen/
// OpenPGP: 4E1055DC84E9DFF613D78557719D69D324539450

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 251 bytes --]

bug#44647: 27.1.50; `bibtex-contline-indentation' doesn't work as file local variable

[Teemu Likonen]

[-- Attachment #1.1: Type: text/plain, Size: 1442 bytes --]

* 2020-11-14 23:27:41+02, Teemu Likonen wrote:
> * 2020-11-14 23:18:16+02, Teemu Likonen wrote:
>> Variable bibtex-contline-indentation does not work as file local
>> variable. The reason is that bibtex-mode command initializes variable
>> fill-prefix before the possible file local variable is available. It
>> gets always the global value of bibtex-contline-indentation.
>>
>> This can be fixed by locally let-binding fill-prefix every time in the
>> relevant filling function. Patch for that is attached.
>
> See: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=44647

I'll add one example. Currently if we want to use file or directory
local variable to define bibtex indentation we must do use variable
bibtex-text-indentation for normal field value indentation and then use
fill-prefix with spaces to define the indentation for continuation
lines:

    ;; .dir-locals.el
    ((bibtex-mode . ((bibtex-text-indentation . 20)
                     (fill-prefix . "                     ")))) ; 21 spaces

Quite obviously more user-friendly way would be this:

    ;; .dir-locals.el
    ((bibtex-mode . ((bibtex-text-indentation . 20)
                     (bibtex-contline-indentation . 21))))

My patch does this. It let-binds fill-prefix every time and initializes
its value by using the (possibly buffer local) value of
bibtex-contline-indentation.

Here is inline version of the patch so that it is easier to read in
debbugs.gnu.org page.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Make-bibtex-contline-indentation-work-as-file-local-.patch --]
[-- Type: text/x-diff, Size: 2057 bytes --]

From a776cae4fcd34987e30b6eab3df45bd2ae66fbd9 Mon Sep 17 00:00:00 2001
From: Teemu Likonen <tlikonen@iki.fi>
Date: Sat, 14 Nov 2020 22:53:18 +0200
Subject: [PATCH] Make `bibtex-contline-indentation' work as file local
 variable

* lisp/textmodes/bibtex.el (bibtex-mode): Don't make fill-prefix a
buffer local variable. It would use the global value, not possible
file local variable.
* lisp/textmodes/bibtex.el (bibtex-fill-field-bounds): Let-bind
fill-prefix variable locally (and dynamically) for use with filling
bibtex fields. The variable is initialized every time by using the
current value of bibtex-contline-indentation variable.
---
 lisp/textmodes/bibtex.el | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lisp/textmodes/bibtex.el b/lisp/textmodes/bibtex.el
index fcf63ed5ec..151dca6fab 100644
--- a/lisp/textmodes/bibtex.el
+++ b/lisp/textmodes/bibtex.el
@@ -3442,8 +3442,6 @@ bibtex-mode
   (set (make-local-variable 'defun-prompt-regexp) "^[ \t]*@[[:alnum:]]+[ \t]*")
   (set (make-local-variable 'outline-regexp) "[ \t]*@")
   (set (make-local-variable 'fill-paragraph-function) #'bibtex-fill-field)
-  (set (make-local-variable 'fill-prefix)
-       (make-string (+ bibtex-entry-offset bibtex-contline-indentation) ?\s))
   (set (make-local-variable 'font-lock-defaults)
        '(bibtex-font-lock-keywords
          nil t ((?$ . "\"")
@@ -4902,7 +4900,10 @@ bibtex-fill-field-bounds
   "Fill BibTeX field delimited by BOUNDS.
 If JUSTIFY is non-nil justify as well.
 If optional arg MOVE is non-nil move point to end of field."
-  (let ((end-field (copy-marker (bibtex-end-of-field bounds))))
+  (let ((end-field (copy-marker (bibtex-end-of-field bounds)))
+        (fill-prefix (make-string (+ bibtex-entry-offset
+                                     bibtex-contline-indentation)
+                                  ?\s)))
     (if (not justify)
         (goto-char (bibtex-start-of-text-in-field bounds))
       (goto-char (bibtex-start-of-field bounds))
-- 
2.20.1


[-- Attachment #1.3: Type: text/plain, Size: 116 bytes --]


-- 
/// Teemu Likonen - .-.. https://www.iki.fi/tlikonen/
// OpenPGP: 4E1055DC84E9DFF613D78557719D69D324539450

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 251 bytes --]

bug#44647: 27.1.50; `bibtex-contline-indentation' doesn't work as file local variable

[Roland Winkler]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 580 bytes --]

On Sat Nov 14 2020 Teemu Likonen wrote:
> This can be fixed by locally let-binding fill-prefix every time in
> the relevant filling function. Patch for that is attached.

Your patch implies that the buffer-local value of fill-prefix is not
what it should be (say, if any other command wants to use it).

How about the rather different patch attached below, partly inspired
by the related comment

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=21764#15

which had somehow escaped my attention.  The issues mentioned in
this comment should likewise be fixed by the attached patch.


[-- Attachment #2: bibtex.diff --]
[-- Type: application/octet-stream, Size: 3715 bytes --]

--- bibtex.el~	2020-11-15 21:13:56.440216563 -0600
+++ bibtex.el	2020-11-15 21:28:47.313180062 -0600
@@ -890,7 +890,8 @@
 (defcustom bibtex-comment-start "@Comment"
   "String starting a BibTeX comment."
   :group 'bibtex
-  :type 'string)
+  :type 'string
+  :safe #'stringp)
 
 (defcustom bibtex-add-entry-hook nil
   "List of functions to call when BibTeX entry has been inserted."
@@ -1219,7 +1220,8 @@
   "Offset for BibTeX entries.
 Added to the value of all other variables which determine columns."
   :group 'bibtex
-  :type 'integer)
+  :type 'integer
+  :safe #'integerp)
 
 (defcustom bibtex-field-indentation 2
   "Starting column for the name part in BibTeX fields."
@@ -1232,13 +1234,15 @@
   "Starting column for the text part in BibTeX fields.
 Should be equal to the space needed for the longest name part."
   :group 'bibtex
-  :type 'integer)
+  :type 'integer
+  :safe #'integerp)
 
 (defcustom bibtex-contline-indentation
   (+ bibtex-text-indentation 1)
   "Starting column for continuation lines of BibTeX fields."
   :group 'bibtex
-  :type 'integer)
+  :type 'integer
+  :safe #'integerp)
 
 (defcustom bibtex-align-at-equal-sign nil
   "If non-nil, align fields at equal sign instead of field text.
@@ -2941,7 +2945,7 @@
                                          (1+ (match-beginning 3)) (1- (match-end 3)))))
                                (unless (assoc key crossref-keys)
                                  (push (list key) crossref-keys))))
-                            ;; We have probably have a non-bibtex file.
+                            ;; We probably have a non-bibtex file.
                             ((not (match-beginning bibtex-type-in-head))
                              (throw 'userkey nil))
                             ;; only keys of known entries
@@ -3435,15 +3439,10 @@
                                    bibtex-parse-keys-timeout t
                                    'bibtex-parse-buffers-stealthily)))
   (set (make-local-variable 'paragraph-start) "[ \f\n\t]*$")
-  (set (make-local-variable 'comment-start) bibtex-comment-start)
-  (set (make-local-variable 'comment-start-skip)
-       (concat (regexp-quote bibtex-comment-start) "\\>[ \t]*"))
   (set (make-local-variable 'comment-column) 0)
   (set (make-local-variable 'defun-prompt-regexp) "^[ \t]*@[[:alnum:]]+[ \t]*")
   (set (make-local-variable 'outline-regexp) "[ \t]*@")
   (set (make-local-variable 'fill-paragraph-function) #'bibtex-fill-field)
-  (set (make-local-variable 'fill-prefix)
-       (make-string (+ bibtex-entry-offset bibtex-contline-indentation) ?\s))
   (set (make-local-variable 'font-lock-defaults)
        '(bibtex-font-lock-keywords
          nil t ((?$ . "\"")
@@ -3462,9 +3461,18 @@
   (set (make-local-variable 'syntax-propertize-function)
        (syntax-propertize-via-font-lock
         bibtex-font-lock-syntactic-keywords))
-  (bibtex-set-dialect nil t)
-  ;; Allow `bibtex-dialect' as a file-local variable.
-  (add-hook 'hack-local-variables-hook #'bibtex-set-dialect nil t))
+  (let ((fun (lambda ()
+               (bibtex-set-dialect)
+               (set (make-local-variable 'comment-start) bibtex-comment-start)
+               (set (make-local-variable 'comment-start-skip)
+                    (concat (regexp-quote bibtex-comment-start) "\\>[ \t]*"))
+               (set (make-local-variable 'fill-prefix)
+                    (make-string (+ bibtex-entry-offset
+                                    bibtex-contline-indentation)
+                                 ?\s)))))
+    (if buffer-file-name
+        (add-hook 'hack-local-variables-hook fun nil t)
+      (funcall fun))))
 
 (defun bibtex-entry-alist (dialect)
   "Return entry-alist for DIALECT."

bug#44647: 27.1.50; `bibtex-contline-indentation' doesn't work as file local variable

[Teemu Likonen]

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

* 2020-11-15 22:09:39-06, Roland Winkler wrote:

> On Sat Nov 14 2020 Teemu Likonen wrote:
>> This can be fixed by locally let-binding fill-prefix every time in
>> the relevant filling function. Patch for that is attached.
>
> Your patch implies that the buffer-local value of fill-prefix is not
> what it should be (say, if any other command wants to use it).

Yes. Bibtex-mode's filling uses fill-prefix and that variable must be
set to correct value before filling. The correct moment may not be when
a bibtex-mode buffer is initialized with buffer-local values. More about
this below.

> How about the rather different patch attached below, partly inspired
> by the related comment
>
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=21764#15
>
> which had somehow escaped my attention.  The issues mentioned in
> this comment should likewise be fixed by the attached patch.

That patch sets buffer-local value for fill-prefix correctly. I'm quite
okay with that but it is not optimal. If user later changes the related
settings, like

    (setq-local bibtex-text-indentation 25
                bibtex-contline-indentation 26)

and then tries to fill the current bibtex entry (C-c C-q) then only
bibtex-text-indentation actually works but bibtex-contline-indentation
doesn't have any effect. The wrong result is caused by the wrong value
of fill-prefix, like this:

    @book{pitkäjohdanto,
      author =               {Oetiker, Tobias and Partl, Hubert and Hyna, Irene and
                      Schlegl, Elisabeth and Hell\-gren, Timo},
      title =                {Pitkänpuoleinen johdanto Latex 2ε:n käyttöön},
      subtitle =             {Eli opi Latex 2ε 133 minuutissa},
      note =                 {Versio 4.17fi, lokakuu 2005},
      date =                 2005,
      url =                  {https://www.ctan.org/pkg/lshort-finnish},
    }

That is why I think fill-prefix should be let-bound every time. User
should be able to trust that bibtex-contline-indentation does the right
thing.

-- 
/// Teemu Likonen - .-.. https://www.iki.fi/tlikonen/
// OpenPGP: 4E1055DC84E9DFF613D78557719D69D324539450

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 251 bytes --]

bug#44647: 27.1.50; `bibtex-contline-indentation' doesn't work as file local variable

[Roland Winkler]

On Mon Nov 16 2020 Teemu Likonen wrote:
> > Your patch implies that the buffer-local value of fill-prefix is not
> > what it should be (say, if any other command wants to use it).
> 
> Yes. Bibtex-mode's filling uses fill-prefix and that variable must
> be set to correct value before filling. The correct moment may not
> be when a bibtex-mode buffer is initialized with buffer-local
> values.

Defcustom supports the keyword :set that can handle the case when a
user variable such as bibtex-contline-indentation is changed which
implies updating another variable such as fill-prefix.  However, I
assume that this is a rather rare case in the present context.

On the other hand, it is a design principle of emacs that variables
such as fill-prefix can be and are used by different commands.  So
they should have their proper value instead of only let-binding them
on the fly.  Such let-bindings are used only when it is the intent
to use different values than the proper buffer-local values.

bug#44647: 27.1.50; `bibtex-contline-indentation' doesn't work as file local variable

[Lars Ingebrigtsen]

Teemu Likonen <tlikonen@iki.fi> writes:

> I found and fixed a bug in Emacs bibtex.el file. Here's my description:

This is the same as bug#44618, so I'm merging these two.  See my
comments there.

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