bug#74511: 31.0.50; `dictionary-search-interface' overrides the user's customization of `dictionary-read-word-function'

[Eshel Yaron]

Hi,

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Steven Allen <steven@stebalien.com>
>> Cc: 74511@debbugs.gnu.org
>> Date: Sun, 24 Nov 2024 12:36:00 -0800
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> Date: Sun, 24 Nov 2024 10:56:08 -0800
>> >> From:  Steven Allen via "Bug reports for GNU Emacs,
>> >>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> >> 
>> >> 
>> >> If the user customizes `dictionary-read-word-function' then loads the
>> >> dictionary package, the value of `dictionary-read-word-function' will be
>> >> overridden when `dictionary-search-interface' is loaded. To reproduce:
>> >> 
>> >> 1. Run emacs with `emacs -Q`.
>> >> 2. Open a scratch buffer and evaluate the following:
>> >> 
>> >>     (setopt dictionary-read-word-function 
>> >> 'dictionary-completing-read-word)
>> >>     (require 'dictionary)
>> >>     (message "%S" dictionary-read-word-function)
>> >> 
>> >> I'd expect `dictionary-read-word-function' to be
>> >> `dictionary-completing-read-word' but instead it gets reset back to the
>> >> default, `dictionary-read-dictionary-default'. I can work around this by
>> >> calling `setopt' after loading the `dictionary' package, but ideally
>> >> that wouldn't be necessary.
>> >
>> > You are supposed to customize dictionary-search-interface if you want
>> > this mode of operation.
>> 
>> In addition to dictionary-search-interface, there are three separately
>> customizable variables:
>> 
>> - dictionary-display-definition-function
>> - dictionary-read-word-function
>> - dictionary-read-dictionary-function
>> 
>> I'd expect dictionary-search-interface to override the defaults for
>> these variables, but I wouldn't expect it to completely clobber any user
>> customizations of said variables.
>> 
>> If these variables cannot be customized, I'd expect them to be "defvar"
>> not "defcustom". Although that would be rather unfortunate because,
>> while I want to enable minibuffer completion for words/dictionaries, I'd
>> also like to use the real dictionary mode (not the help buffer).
>> 
>> Adding ":initialize 'custom-initialize-changed" to
>> dictionary-search-interface would make it possible to specify either
>> dictionary-search-interface or any of the other variables (but not both
>> at the same time).
>
> Eshel, would you please look into this?  IMO, we should at least
> improve the documentation to clarify these subtleties.  Bonus points
> for allowing users to customize the other options without clobbering
> them when the package is loaded or dictionary-search-interface is
> customized.

Thank you Steven, for reporting this issue, and Eli, for pinging me.

The patch below tries to do both: 

- it adds a few words to the docstring of dictionary-search-interface
  which explain that this option can only be meaningfully set after
  loading dictionary.el, and
- it avoids clobbering existing settings for the other options during
  initialization.  It still overrides existing settings for the other
  options if you set dictionary-search-interface after dictionary.el,
  because that's the intended behavior when customizing this option.

WDYT?


diff --git a/lisp/net/dictionary.el b/lisp/net/dictionary.el
index 58c2e9771ba..0f9daa2d07d 100644
--- a/lisp/net/dictionary.el
+++ b/lisp/net/dictionary.el
@@ -301,21 +301,30 @@ dictionary-search-interface
 When set to `help', `dictionary-search' displays definitions in a *Help* 
buffer,
 and provides completion for word selection based on dictionary matches.
 
-Otherwise, `dictionary-search' displays definitions in a *Dictionary* buffer."
+Otherwise, `dictionary-search' displays definitions in a *Dictionary* buffer.
+
+This option only takes affect when you customize it after loading the
+`dictionary' library, so you may want to use `with-eval-after-load' to
+set it in your Emacs initialization file."
   :type '(choice (const :tag "Dictionary buffer" nil)
                  (const :tag "Help buffer" help))
   :set (lambda (symbol value)
-         (let ((vals (pcase value
-                       ('help '(dictionary-display-definition-in-help-buffer
-                                dictionary-completing-read-word
-                                dictionary-completing-read-dictionary))
-                       (_     '(nil
-                                dictionary-read-word-default
-                                dictionary-read-dictionary-default)))))
-           (seq-setq (dictionary-display-definition-function
-                      dictionary-read-word-function
-                      dictionary-read-dictionary-function)
-                     vals))
+         ;; If the symbol of this option is not bound, then we are
+         ;; initializing it, and the user hasn't set it in init.el.
+         ;; Avoid overriding individual customization of other options
+         ;; in this case.
+         (when (boundp symbol)
+           (let ((vals (pcase value
+                         ('help '(dictionary-display-definition-in-help-buffer
+                                  dictionary-completing-read-word
+                                  dictionary-completing-read-dictionary))
+                         (_     '(nil
+                                  dictionary-read-word-default
+                                  dictionary-read-dictionary-default)))))
+             (seq-setq (dictionary-display-definition-function
+                        dictionary-read-word-function
+                        dictionary-read-dictionary-function)
+                       vals)))
          (set-default-toplevel-value symbol value))
   :version "30.1")