Hi Philip, Thanks for your comments, I'm responding to them inline and attaching a slightly updated patch based on them below. Philip Kaludercic writes: >> From 2bbd1767594990357f61d4af467093bf6abb117e Mon Sep 17 00:00:00 2001 >> From: Eshel Yaron >> Date: Mon, 15 May 2023 21:04:21 +0300 >> Subject: [PATCH v2] Add customization options for dictionary-search >> >> Allow users to customize 'dictionary-search' via several new >> customization options. >> >> * lisp/net/dictionary.el (dictionary-define-word) >> (dictionary-match-word) >> (dictionary-completing-read-word) >> (dictionary-dictionaries) >> (dictionary-completing-read-dictionary) >> (dictionary-display-definition-in-help-buffer): New functions. >> (dictionary-read-word-prompt) >> (dictionary-display-definition-function) >> (dictionary-read-word-function) >> (dictionary-read-dictionary-function) >> (dictionary-search-interface): New user options. >> (dictionary-search): Use them. >> (dictionary-read-dictionary-default) >> (dictionary-read-word-default): New functions, extracted from >> 'dictionary-search'. > > Shouldn't these Changelog entries be folded together? > > * lisp/net/dictionary.el (dictionary-define-word, dictionary-match-word, > dictionary-completing-read-word, dictionary-dictionaries, > dictionary-completing-read-dictionary, > dictionary-display-definition-in-help-buffer): New functions. > I'm not sure, the way I read the example commit message in CONTRIBUTE is that opening and closing parentheses should appear on the same line, no? Anyway I updated the commit message to be a bit more compact. >> +(defcustom dictionary-display-definition-function nil >> + "Function to use for displaying dictionary definitions. >> +It is called with three string arguments: the word being defined, >> +the dictionary name, and the full definition." >> + :type '(choice (const :tag "Dictionary buffer" nil) >> + (const :tag "Help buffer" >> + dictionary-display-definition-in-help-buffer) >> + (function :tag "Custom function")) >> + :version "30.1") > > What is the reason for having this option fallback to nil? I couldn't > make that out from the patch. If all the other options offer a > concrete-default function (that you could also call in your own > function), then it seems inconsistent to not provide this here as well. > The reason is that, unlike the other options, the default path that `dictionary-search` takes to displaying a definition is highly coupled with how it obtains the definition, making it difficult to extract into a standalone function. That's a refactor I prefer to avoid at this point. So, if you set `dictionary-display-definition-function` to a custom function, we use the new function `dictionary-define-word` to cleanly obtain the definition and let your custom function display it. If you use the default (nil) value, we let `dictionary-search` call the "old" function `dictionary-new-search` that both obtains and displays the definition. >> +(defcustom dictionary-search-interface nil >> + "Controls how `dictionary-search' prompts for words and displays definitions. >> + >> +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." >> + :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))))) >> + (setq dictionary-display-definition-function (nth 0 vals) >> + dictionary-read-word-function (nth 1 vals) >> + dictionary-read-dictionary-function (nth 2 vals))) > > I think you could also make use of seq-setq here? > Done, in the updated patch. >> +(defun dictionary-define-word (word dictionary) >> + "Return the definition of WORD in DICTIONARY, or nil if not found." >> + (dictionary-send-command >> + (format "define %s \"%s\"" dictionary word)) >> + (when (and (= (read (dictionary-read-reply)) 150) >> + (= (read (dictionary-read-reply)) 151)) > > I think a memq would be nice here. > No, `memq` would be appropriate if we wanted to check that the expression `(read (dictionary-read-reply))` evaluates to either 150 or to 151, but here we want to check that it evaluates 150 and then afterwards that it evaluates to 151. >> +(define-button-type 'help-word >> + :supertype 'help-xref >> + 'help-function 'dictionary-search >> + 'help-echo (purecopy "mouse-2, RET: describe this word")) > > Why the purecopy? > Thanks, I guess that's something I copied from the button definitions in help-mode.el. Removed. >> +(defun dictionary-display-definition-in-help-buffer (word dictionary definition) >> + "Display DEFINITION, the definition of WORD in DICTIONARY." >> + (let ((help-buffer-under-preparation t)) >> + (help-setup-xref (list #'dictionary-search word dictionary) >> + (called-interactively-p 'interactive)) >> + (with-help-window (help-buffer) >> + (with-current-buffer (help-buffer) >> + (insert definition) >> + (goto-char (point-min)) >> + (while (re-search-forward (rx "{" >> + (group-n 1 (* (not (any ?})))) >> + "}") >> + nil t) > > Perhaps you could explain what is going on here. Why is this pattern > significant? > We want to buttonize references to other definitions in the *Help* buffer, which appear enclosed in curly braces. I've added a comment explaining this. Updated patch: