Re: master 388a874 2/4: Do interactive mode tagging for man.el

Re: master 388a874 2/4: Do interactive mode tagging for man.el

[Eli Zaretskii]

Stefan, did you consider the fact that woman.el defines a different
mode, but uses some commands from man.el?  (I didn't try invoking the
commands you tagged, so maybe there's no problem.)

Re: master 388a874 2/4: Do interactive mode tagging for man.el

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> Stefan, did you consider the fact that woman.el defines a different
> mode, but uses some commands from man.el?  (I didn't try invoking the
> commands you tagged, so maybe there's no problem.)

Thanks for catching this.

Yes, completion for these commands in Woman is indeed now broken when
filtering is enabled.  Sorry about that.

The problem here is that `woman-mode' uses `man-mode' but does not
inherit that mode.  Instead, it *calls* `Man-mode' from `woman-mode',
with some extra `fset' shenanigans that I don't see a trivial fix for.
(This unusual arrangement should probably be fixed at some point...)

So if we want to keep the tagging, I see two possible workarounds:

1) Tag the man-mode commands with both `Man-mode' and `woman-mode'.

2) Introduce a new mode, `man-shared-mode', inheriting from
   special-mode, that both `Man-mode' and `woman-mode' can inherit
   from.  Then tag the commands using that.

Both options have their pros and cons.

WDYT?

Re: master 388a874 2/4: Do interactive mode tagging for man.el

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Fri, 19 Feb 2021 20:24:49 -0600
> Cc: emacs-devel@gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Stefan, did you consider the fact that woman.el defines a different
> > mode, but uses some commands from man.el?  (I didn't try invoking the
> > commands you tagged, so maybe there's no problem.)
> 
> Thanks for catching this.

So I think the procedure for tagging should include a mandatory
grepping of all the Lisp files to see if the mode or its features are
referenced or used anywhere else.

> So if we want to keep the tagging, I see two possible workarounds:
> 
> 1) Tag the man-mode commands with both `Man-mode' and `woman-mode'.
> 
> 2) Introduce a new mode, `man-shared-mode', inheriting from
>    special-mode, that both `Man-mode' and `woman-mode' can inherit
>    from.  Then tag the commands using that.
> 
> Both options have their pros and cons.

There's a 3rd option:

 3) Put a 'completion-predicate' property on the relevant woman.el
    commands, so that command-completion-default-include-p does TRT
    with them even though the modes don't match.

I will withhold my opinion for now, and let Lars and others speak up.
This is basically the first time we bump into the issues that were
discussed here already, but sounded theoretical back then.  We now
need to fix such an issue with the tagging in practice, and we might
as well invest some thinking before we decide which way to go.
Because whatever we decide to do now, and the considerations to go
into that decision, will be most probably taken as the canonical
solution for such situations.

Re: master 388a874 2/4: Do interactive mode tagging for man.el

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> So I think the procedure for tagging should include a mandatory
> grepping of all the Lisp files to see if the mode or its features are
> referenced or used anywhere else.

Yup.

> There's a 3rd option:
>
>  3) Put a 'completion-predicate' property on the relevant woman.el
>     commands, so that command-completion-default-include-p does TRT
>     with them even though the modes don't match.

But the problem here is that these aren't woman.el commands -- they are
man.el commands, but used by woman.el?

> Because whatever we decide to do now, and the considerations to go
> into that decision, will be most probably taken as the canonical
> solution for such situations.

Yes, it's good to bump into something like this this early in the
tagging process.

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

Re: master 388a874 2/4: Do interactive mode tagging for man.el

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Stefan Kangas <stefan@marxist.se>,  emacs-devel@gnu.org
> Date: Sat, 20 Feb 2021 13:26:03 +0100
> 
> > There's a 3rd option:
> >
> >  3) Put a 'completion-predicate' property on the relevant woman.el
> >     commands, so that command-completion-default-include-p does TRT
> >     with them even though the modes don't match.
> 
> But the problem here is that these aren't woman.el commands -- they are
> man.el commands, but used by woman.el?

Sorry, I meant the relevant man.el commands.

Re: master 388a874 2/4: Do interactive mode tagging for man.el

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> 2) Introduce a new mode, `man-shared-mode', inheriting from
>    special-mode, that both `Man-mode' and `woman-mode' can inherit
>    from.  Then tag the commands using that.

I think that makes sense, and would have the additional effect of
documenting, right in the Man-* command functions, that these aren't
just used in man.el, which may be nice if somebody later alters the
functions.

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

Re: master 388a874 2/4: Do interactive mode tagging for man.el

[Stefan Kangas]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Kangas <stefan@marxist.se> writes:
>
>> 2) Introduce a new mode, `man-shared-mode', inheriting from
>>    special-mode, that both `Man-mode' and `woman-mode' can inherit
>>    from.  Then tag the commands using that.
>
> I think that makes sense, and would have the additional effect of
> documenting, right in the Man-* command functions, that these aren't
> just used in man.el, which may be nice if somebody later alters the
> functions.

OK, here's a patch.

[-- Attachment #2: 0001-Fix-interactive-mode-tagging-for-man-and-woman.patch --]
[-- Type: text/x-diff, Size: 6943 bytes --]

From 583f66871145c60797f0b148f502d6e3516aff5a Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Sun, 21 Feb 2021 11:19:57 +0100
Subject: [PATCH] Fix interactive mode tagging for man and woman

* lisp/man.el (man-shared-mode): New mode inheriting special-mode.
(Man-mode):
* lisp/woman.el (woman-mode): Inherit from man-shared-mode.

* lisp/man.el (man-follow, Man-update-manpage)
(Man-fontify-manpage, Man-cleanup-manpage, Man-next-section)
(Man-previous-section, Man-goto-section)
(Man-goto-see-also-section, Man-follow-manual-reference)
(Man-kill, Man-goto-page, Man-next-manpage)
(Man-previous-manpage): Change interactive mode tag to
man-shared-mode.

This was discussed in:
https://lists.gnu.org/r/emacs-devel/2021-02/msg01619.html
---
 lisp/man.el   | 35 +++++++++++++++++++++--------------
 lisp/woman.el |  4 +++-
 2 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/lisp/man.el b/lisp/man.el
index 70b8aa8eb2..2d8bf340b1 100644
--- a/lisp/man.el
+++ b/lisp/man.el
@@ -1024,7 +1024,7 @@ man
 ;;;###autoload
 (defun man-follow (man-args)
   "Get a Un*x manual page of the item under point and put it in a buffer."
-  (interactive (list (Man-default-man-entry)) Man-mode)
+  (interactive (list (Man-default-man-entry)) man-shared-mode)
   (if (or (not man-args)
 	  (string= man-args ""))
       (error "No item under point")
@@ -1143,7 +1143,7 @@ Man-getpage-in-background
 
 (defun Man-update-manpage ()
   "Reformat current manpage by calling the man command again synchronously."
-  (interactive nil Man-mode)
+  (interactive nil man-shared-mode)
   (when (eq Man-arguments nil)
     ;;this shouldn't happen unless it is not in a Man buffer."
     (error "Man-arguments not initialized"))
@@ -1239,7 +1239,7 @@ Man-softhyphen-to-minus
 (defun Man-fontify-manpage ()
   "Convert overstriking and underlining to the correct fonts.
 Same for the ANSI bold and normal escape sequences."
-  (interactive nil Man-mode)
+  (interactive nil man-shared-mode)
   (goto-char (point-min))
   ;; Fontify ANSI escapes.
   (let ((ansi-color-apply-face-function #'ansi-color-apply-text-property-face)
@@ -1355,7 +1355,7 @@ Man-cleanup-manpage
 Normally skip any jobs that should have been done by the sed script,
 but when called interactively, do those jobs even if the sed
 script would have done them."
-  (interactive "p" Man-mode)
+  (interactive "p" man-shared-mode)
   (if (or interactive (not Man-sed-script))
       (progn
 	(goto-char (point-min))
@@ -1527,7 +1527,14 @@ Man-page-from-arguments
 
 (defvar bookmark-make-record-function)
 
-(define-derived-mode Man-mode special-mode "Man"
+(define-derived-mode man-shared-mode special-mode "Man Shared"
+  "Parent mode for `Man-mode' like modes.
+This mode is here to be inherited by modes that need to use
+commands from `Man-mode'.  Used by `woman'.
+(In itself, this mode currently does nothing.)"
+  :interactive nil)
+
+(define-derived-mode Man-mode man-shared-mode "Man"
   "A mode for browsing Un*x manual pages.
 
 The following man commands are available in the buffer.  Try
@@ -1723,7 +1730,7 @@ Man-unindent
 
 (defun Man-next-section (n)
   "Move point to Nth next section (default 1)."
-  (interactive "p" Man-mode)
+  (interactive "p" man-shared-mode)
   (let ((case-fold-search nil)
         (start (point)))
     (if (looking-at Man-heading-regexp)
@@ -1739,7 +1746,7 @@ Man-next-section
 
 (defun Man-previous-section (n)
   "Move point to Nth previous section (default 1)."
-  (interactive "p" Man-mode)
+  (interactive "p" man-shared-mode)
   (let ((case-fold-search nil))
     (if (looking-at Man-heading-regexp)
 	(forward-line -1))
@@ -1771,7 +1778,7 @@ Man-goto-section
           (chosen (completing-read prompt Man--sections
                                    nil nil nil nil default)))
      (list chosen))
-   Man-mode)
+   man-shared-mode)
   (setq Man--last-section section)
   (unless (Man-find-section section)
     (error "Section %s not found" section)))
@@ -1780,7 +1787,7 @@ Man-goto-section
 (defun Man-goto-see-also-section ()
   "Move point to the \"SEE ALSO\" section.
 Actually the section moved to is described by `Man-see-also-regexp'."
-  (interactive nil Man-mode)
+  (interactive nil man-shared-mode)
   (if (not (Man-find-section Man-see-also-regexp))
       (error "%s" (concat "No " Man-see-also-regexp
 		     " section found in the current manpage"))))
@@ -1835,7 +1842,7 @@ Man-follow-manual-reference
 	     (chosen (completing-read prompt Man--refpages
 				      nil nil nil nil defaults)))
         chosen)))
-   Man-mode)
+   man-shared-mode)
   (if (not Man--refpages)
       (error "Can't find any references in the current manpage")
     (setq Man--last-refpage reference)
@@ -1844,7 +1851,7 @@ Man-follow-manual-reference
 
 (defun Man-kill ()
   "Kill the buffer containing the manpage."
-  (interactive nil Man-mode)
+  (interactive nil man-shared-mode)
   (quit-window t))
 
 (defun Man-goto-page (page &optional noerror)
@@ -1856,7 +1863,7 @@ Man-goto-page
 	 (error "You're looking at the only manpage in the buffer")
        (list (read-minibuffer (format "Go to manpage [1-%d]: "
                                       (length Man-page-list))))))
-    Man-mode)
+    man-shared-mode)
   (if (and (not Man-page-list) (not noerror))
       (error "Not a man page buffer"))
   (when Man-page-list
@@ -1878,7 +1885,7 @@ Man-goto-page
 
 (defun Man-next-manpage ()
   "Find the next manpage entry in the buffer."
-  (interactive nil Man-mode)
+  (interactive nil man-shared-mode)
   (if (= (length Man-page-list) 1)
       (error "This is the only manpage in the buffer"))
   (if (< Man-current-page (length Man-page-list))
@@ -1889,7 +1896,7 @@ Man-next-manpage
 
 (defun Man-previous-manpage ()
   "Find the previous manpage entry in the buffer."
-  (interactive nil Man-mode)
+  (interactive nil man-shared-mode)
   (if (= (length Man-page-list) 1)
       (error "This is the only manpage in the buffer"))
   (if (> Man-current-page 1)
diff --git a/lisp/woman.el b/lisp/woman.el
index 98f1a47d24..0cd72cb261 100644
--- a/lisp/woman.el
+++ b/lisp/woman.el
@@ -1856,13 +1856,15 @@ woman-reset-emulation
 
 (defvar bookmark-make-record-function)
 
-(define-derived-mode woman-mode special-mode "WoMan"
+(define-derived-mode woman-mode man-shared-mode "WoMan"
   "Turn on (most of) Man mode to browse a buffer formatted by WoMan.
 WoMan is an ELisp emulation of much of the functionality of the Emacs
 `man' command running the standard UN*X man and ?roff programs.
 WoMan author: F.J.Wright@Maths.QMW.ac.uk
 See `Man-mode' for additional details.
 \\{woman-mode-map}"
+  ;; FIXME: Should all this just be re-arranged so that this can just
+  ;; inherit `man-shared-mode' and be done with it?
   (let ((Man-build-page-list (symbol-function 'Man-build-page-list))
 	(Man-strip-page-headers (symbol-function 'Man-strip-page-headers))
 	(Man-unindent (symbol-function 'Man-unindent))
-- 
2.30.0

Re: master 388a874 2/4: Do interactive mode tagging for man.el

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

>> I think that makes sense, and would have the additional effect of
>> documenting, right in the Man-* command functions, that these aren't
>> just used in man.el, which may be nice if somebody later alters the
>> functions.
>
> OK, here's a patch.

Looks good to me.

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

Re: master 388a874 2/4: Do interactive mode tagging for man.el

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Looks good to me.

Thanks, pushed to master (with Eli's suggested name change).

See commit ff759b1d0a.

Re: master 388a874 2/4: Do interactive mode tagging for man.el

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Sun, 21 Feb 2021 04:37:45 -0600
> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> 
> * lisp/man.el (man-shared-mode): New mode inheriting special-mode.

I suggest to call it "man-common" or "man-basic".

Thanks.

Re: master 388a874 2/4: Do interactive mode tagging for man.el

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Kangas <stefan@marxist.se>
>> Date: Sun, 21 Feb 2021 04:37:45 -0600
>> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
>>
>> * lisp/man.el (man-shared-mode): New mode inheriting special-mode.
>
> I suggest to call it "man-common" or "man-basic".

Yes, that's better.  I'll change it.

Re: master 388a874 2/4: Do interactive mode tagging for man.el

[Lars Ingebrigtsen]

I've further tweaked my interactive tagging helper command to do
grepping to give a bit of information about whether the command in
question is used in other places in Emacs (like in the wo/man case) and
tested it in a couple of small files.  Seems to work OK, but can
probably do with some tweaking to reduce false positives.

(global-set-key [(hyper l)] 'my-fix-command)
(defvar my-prev-mode nil)
(defun my-fix-command ()
  (interactive)
  (let ((mode nil)
	(regexp "(define-derived-mode \\([^ \t\n]+\\)\\|(defun \\([^ \t\n]+-mode\\) ")
	change bindings)
    (if (not (re-search-forward "^ *\\((interactive\\)" nil t))
	(message "No more interactive in this file")
      (recenter nil t)
      (save-match-data
	(save-excursion
	  (beginning-of-defun)
	  (let ((form (read (current-buffer))))
	    (when (and (listp form)
		       (eq (car form) 'defun))
	      (setq bindings
		    (shell-command-to-string
		     (format
		      "cd %s../lisp; grep -r --include '*.el' \"define-key.*'%s\""
		      data-directory (cadr form)))))))
	(save-excursion
	  (when (or (re-search-backward regexp nil t)
		    (re-search-forward regexp nil t))
	    (setq mode (or (match-string 1) (match-string 2)))))
	(setq change (read-string (format "%sChange to: "
					  (or bindings ""))
				  (or mode my-prev-mode))))
      (when (plusp (length change))
	(setq mode change)
	(goto-char (match-beginning 1))
	(let ((form (read (current-buffer))))
	  (goto-char (match-beginning 1))
	  (forward-char 1)
	  (if (> (length form) 1)
	      (progn
		(forward-sexp 2)
		(insert " " mode))
	    (forward-sexp 1)
	    (insert " nil " mode))
	  (forward-sexp -1))
	(setq my-prev-mode mode)))))

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