Eli Zaretskii writes: >> From: "H. Dieter Wilhelm" >> Date: Fri, 06 Jan 2023 20:03:23 +0100 >> >> I attached a patch for the current master branch build from git >> format-patch. And spliced the code to implement the linking of symbols >> in info manuals to the help documentation into lisp/info-xref.el. > > Thanks. No, thank you. It's a pleasure to read your comments. > I think this should be in info.el, or maybe in a separate > info-SOMETHING.el file. info-xref.el is for a certain job, of > interest primarily to Emacs maintainers, that is different from this > one, and I'm not sure conflating them is TRT. Understood, it's now located in info.el. >> +;; This library provides links of symbols (functions, variables, > > The "This library" part is a remnant of the previous life of this > code, and should be reworded to refer to specific command(s). Done. (I'm sorry, I should have reviewed first this code from 2020.) >> +;; In any case all symbol names must be known to Emacs, i.e. their >> +;; names are found in the variable `obarray'. > > I think a more useful way of saying this is > > In any case, the symbol must be known to Emacs, which means it is > either a built-in, or its Lisp package is loaded in the current > Emacs session, or the symbol is auto-loaded. Absolutely, done >> +;; Inform is checking if the Info documents are relevant Elisp and > ^^^^^^ > This should be adapted to the "new life" of Inform as part of Emacs. Done >> +;; Emacs related files to avoid false positives. Please see the >> +;; customization variable `inform-none-emacs-or-elisp-documents'. > ^^^^^^ > And this. Done >> +;;; Change Log: >> + >> ... >> +;; (`font-lock-function-name-face') > > Not sure if it makes sense to keep this change log. Agreed and removed >> +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; >> +;; Does the following belong to customize.el? >> .... >> +;;; Ideas: >> +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; > > Please review this part and decide which portions should be kept, > perhaps after a suitable rewording, and which should be removed. I removed above blather and kept it as private notes. >> +(require 'button) >> +(require 'cl-lib) >> +(require 'help-mode) ;redundant? > > If this is added to an existing file, there should be a ^L and some > heading-style command before it. Sure, please let me learn about the application of "page markers" first. >> +;; activate inform without manually loading it. Is there a better way? >> +;; ;;;###autoload (require 'info-xref) >> >> +;; this is spawning lisp/info-xref.el's definition to 'info! This >> +;; group is sorted now in 'docs and 'info! -FIXME- > > Comments should start with a capital letter. Done >> +;;;###autoload >> +(defcustom info-xref-make-xref-flag t >> + "Non-nil means create symbol links in info buffers." >> + :type '(choice (const :tag "Create links" t) >> + (const :tag "Do not link" nil)) >> + :group 'info-xref) > > I think we frown on autoloading defcustoms. Removed it, I think above is redundant when the code resides in info.el, anyway. > Also, every new defcustom should have a :version tag. Done >> +;; Info-director-list must be initialised > ^^^^^^^^ > Typo. Also, comments should be complete sentences, and end with a > period (here and elsewhere in the patch). Done (I should have run checkdoc first, uuups there are lots of warnings from the other code in info.el.) >> +(info-initialize) > > Why do you need to call this? and why on top level? Because I have no better idea to build info-emacs-info-dir-content (maybe dynamically?). With the code below I force the initialisation of Info-directory-list. It is used for checking if the current info document is relevant to Emacs. ;; We need to initalise Info-directory-list first. (info-initialize) ;; Before declaring the following variable: (defvar info-emacs-info-dir-content (mapcar 'file-name-nondirectory ;'file-name-sans-extension (directory-files (car ;; search for the main Emacs' info/ directory (cl-member "[^.]emacs" Info-directory-list :test 'string-match-p)) ;; don't list "." and ".." t "[^.]$")) "List of file names in Emacs' own info/ directory.") >> +;; Turn into regexp list necessary? Stefan >> +;; Switch to alist with explanation of file name? >> +(defcustom info-xref-none-emacs-or-elisp-documents >> + '("aarm2012" ; Stefan: Ada manual, Elpa archive >> + "arm2012" ; Stefan: Ada manual >> + "sicp" ; T.V: Structure and Interpretation of Computer Programs, >> + ; Melpa archive >> + ) >> + "List of all none GNU-Emacs or Elisp documentation. >> +Or other documents not to be checked for linking to their help >> +documentation. The list must contains only the base name of the >> +files (without their file name extension \".info\")." >> + :type '(repeat string) >> + :group 'info-xref) > > Not sure what is this about, and what do the names above signify. This was a discussion on gmane.emacs.devel (please see jwvtv0qv5av.fsf-monnier+emacs@gnu.org) on how to avoid trying to link info documents which don't belong to Emacs. (Core-Utils, Ada documentation from Melpa, etc.) > There are also typos: "none GNU-Emacs", "must contains". Done. >> +(defun Info-xref-make-xrefs (&optional buffer) >> + "Parse and hyperlink documentation cross-references in the given BUFFER. > > The doc string should tell what happens if BUFFER is omitted or nil. Thank you, I'll take care about it. >> + (while (re-search-forward Info-xref-symbol-regexp nil t) >> + (let* ((data (match-string 8)) >> + (sym (intern-soft data))) >> + (if sym >> + (cond >> + ((match-string 3) ; `variable' &c >> + (and (or (boundp sym) ; `variable' doesn't ensure >> + ; it's actually bound >> + (get sym 'variable-documentation)) >> + (Info-xref-button 8 'Info-xref-variable sym))) >> + ((match-string 4) ; `function' &c >> + (and (fboundp sym) ; similarly >> + (Info-xref-button 8 'Info-xref-function sym))) >> + ((match-string 5) ; `face' >> + (and (facep sym) >> + (Info-xref-button 8 'Info-xref-face sym))) >> + ((match-string 6)) ; nothing for `symbol' >> + ((match-string 7) >> + (Info-xref-button 8 'Info-xref-function-def sym)) >> + ((cl-some (lambda (x) (funcall (nth 1 x) sym)) >> + describe-symbol-backends) >> + (Info-xref-button 8 'Info-xref-symbol sym))))))) > > Can this be rewritten so as to avoid the need for error-prone updates > of the sub-expression numbers every time Info-xref-symbol-regexp is > modified? I'll try later, took note. > Finally, this needs additions to the user manual and NEWS. Alright, I took a note for working on this. Thank you :-)