Re: [PATCH] Add pinyin-isearch to NonGNU ELPA

diff --git a/.dir-locals.el b/.dir-locals.el
index 87c6e77343c..4a1d973f4fd 100644
--- a/.dir-locals.el
+++ b/.dir-locals.el
@@ -1,11 +1,16 @@
 ((emacs-lisp-mode
   . (
+     ;; FIXME: Just use the standard ;;;/;;;;/;;;;; markers instead,
+     ;; so you don't need these settings at all?
      (outline-regexp . "^;;; \\|^;; -=-=\\|^;;;; ")
      (outline-it-heading-alist .
                                '((";;; " . 1)
                                  (";; -=-=" . 2)
                                  (";;;; " . 3)))
      ;; (eval . (outline-minor-mode 1))
+     ;; FIXME: These look like personal preferences rather than settings
+     ;; needed by anyone looking at or editing the code (IOW they belong
+     ;; in your init file rather than here).
      (eval . (progn (keymap-local-set "C-c k" #'outline-previous-heading)
                     (keymap-local-set "C-c n" #'outline-next-heading)
                     (when (require 'outline-it nil 'noerror)
diff --git a/.gitignore b/.gitignore
index adef969d3d2..dda9f70f0ec 100644
--- a/.gitignore
+++ b/.gitignore
@@ -15,3 +15,7 @@ dist/
 
 # Undo-tree save-files
 *.~undo-tree
+
+# ELPA-generated files.
+/pinyin-isearch-autoloads.el
+/pinyin-isearch-pkg.el
diff --git a/pinyin-isearch-chars.el b/pinyin-isearch-chars.el
index 6426d84a2ef..408c5456a0c 100644
--- a/pinyin-isearch-chars.el
+++ b/pinyin-isearch-chars.el
@@ -63,6 +63,8 @@
 
 (require 'subr-x) ; for 28.1 and `string-join'
 
+;; FIXME: There should be only one `defgroup' for a given group name,
+;; but this group is already defined in `pinyin-isearch.el'.
 (defgroup pinyin-isearch nil
   "Pinyin-isearch customization."
   :group 'pinyin-isearch)
@@ -72,12 +74,16 @@
 (defvar pinyin-isearch-full-fallback) : ; (require 'pinyin-isearch)
 
 (defcustom pinyin-isearch-chars-fallback t
+  ;; FIXME: I do not understand the docstring:
+  ;; - "add pinyin" to what?
+  ;; - If what "was not matched"?
+  ;; - The second line is not a sentence, I don't know what it intends to say.
   "Non-nil means add pinyin that was not matched.
 full query string as a regex variant.
 But only if pinyin cant be interpreted as any characters (before even
  search).
 If there is undecoded letters at the end after dissasembling."
-  :local t
+  :local t ;; FIXME: Why?
   :type 'boolean
   :group 'pinyin-isearch)
 
diff --git a/pinyin-isearch-loaders.el b/pinyin-isearch-loaders.el
index fca3bba8464..a3942357253 100644
--- a/pinyin-isearch-loaders.el
+++ b/pinyin-isearch-loaders.el
@@ -43,35 +43,38 @@
 (defun pinyin-isearch-loaders--get-location-of-input-method (leim-name)
   "Get elisp file location.
 Argument LEIM-NAME input-method name."
-  (car (nthcdr 5 (assoc leim-name input-method-alist))))
+  (let ((im (assoc leim-name input-method-alist)))
+    (cl-assert (eq #'quail-use-package (nth 2 im)))
+    (nth 5 im)))
 
 (defun pinyin-isearch-loaders--quail-define-rules-advice (&rest rules)
   "Replace `quail-define-rules' to catch passed arguments.
 Optional argument ARGS catched RULES argument."
   `(setq pinyin-isearch-loaders--rules ',rules))
 
-(defun pinyin-isearch-loaders--quail-define-package-advice (&rest _args)
-  "Replace `quail-define-package' to disable it.
-Argument ARGS not used."
-  nil)
-
-(defun pinyin-isearch-loaders--quail-defrule-advice (&rest _args)
-  "Replace `quail-defrule' to disable it.
-Argument ARGS not used."
-  nil)
-
 (defun pinyin-isearch-loaders--quail-extractor (quail-file)
+  ;; FIXME: Name and description of argument QUAIL-FILE don't match its
+  ;; use when passed to `pinyin-isearch-loaders--get-location-of-input-method'.
   "Used to set variable `pinyin-isearch-loaders--punct-rules'.
 Argument QUAIL-FILE \"quail/PY.el\" for example."
-  (advice-add 'quail-define-rules :override #'pinyin-isearch-loaders--quail-define-rules-advice)
-  (advice-add 'quail-define-package :override #'pinyin-isearch-loaders--quail-define-package-advice)
-  (advice-add 'quail-defrule :override #'pinyin-isearch-loaders--quail-defrule-advice)
-  (load (concat (pinyin-isearch-loaders--get-location-of-input-method quail-file) ".el"))
-  (advice-remove 'quail-define-rules #'pinyin-isearch-loaders--quail-define-rules-advice)
-  (advice-remove 'quail-define-package #'pinyin-isearch-loaders--quail-define-package-advice)
-  (advice-remove 'quail-defrule #'pinyin-isearch-loaders--quail-defrule-advice)
-  ;; return
-  pinyin-isearch-loaders--rules)
+  ;; FIXME: IIRC some other package faced a similar problem, I suggest you `M-x
+  ;; report-emacs-bug' and ask for an easier way to access this data (maybe
+  ;; it can be reconstructed from the input-method table, but either way
+  ;; Emacs should provide that).  This is a clever hack to get the data,
+  ;; but it's also hideous and dangerous, so we should try to get rid of it.
+  (unwind-protect ;; Remove the advices even in case of an error!
+      (let (pinyin-isearch-loaders--rules)
+        (advice-add 'quail-define-rules :override
+                    #'pinyin-isearch-loaders--quail-define-rules-advice)
+        (advice-add 'quail-define-package :override #'ignore)
+        ;; (advice-add 'quail-defrule :override #'ignore)
+        (load (concat (pinyin-isearch-loaders--get-location-of-input-method quail-file) ".el"))
+        ;; return
+        pinyin-isearch-loaders--rules)
+    (advice-remove 'quail-define-rules #'pinyin-isearch-loaders--quail-define-rules-advice)
+    (advice-remove 'quail-define-package #'ignore)
+    ;; (advice-remove 'quail-defrule #'ignore)
+    ))
 
 (defun pinyin-isearch-loaders--punct-quail-filter (rules)
   "Load RULES for single letters of punctuations."
@@ -100,19 +103,14 @@ Because ǚ and other u tones is very same and with same letter."
 
 ;; ---------- load pinyin from "quail/sisheng"  --------
 
-;; `sisheng-regexp', `sisheng-vowel-table', `sisheng-syllable-table'.
-(defun pinyin-isearch-loaders--quail-make-sisheng-rules-advice (_syllable)
-  "Suppress function `quail-make-sisheng-rules'.
-From quail/sisheng.el, for speed.
-Argument SYLLABLE not used."
-  nil)
-
 (defun pinyin-isearch-loaders-load-chinese-sisheng ()
   "We don't use result, we need only loaded variables `sisheng-*'."
   (when (not (boundp 'sisheng-vowel-table))
-    (advice-add 'quail-make-sisheng-rules :override #'pinyin-isearch-loaders--quail-make-sisheng-rules-advice)
-    (pinyin-isearch-loaders--quail-extractor "chinese-sisheng")
-    (advice-remove 'quail-make-sisheng-rules #'pinyin-isearch-loaders--quail-make-sisheng-rules-advice)))
+    (unwind-protect
+        (progn
+          (advice-add 'quail-make-sisheng-rules :override #'ignore)
+          (pinyin-isearch-loaders--quail-extractor "chinese-sisheng"))
+      (advice-remove 'quail-make-sisheng-rules #'ignore))))
 
 (provide 'pinyin-isearch-loaders)
 ;;; pinyin-isearch-loaders.el ends here
diff --git a/pinyin-isearch.el b/pinyin-isearch.el
index 3b62f46c680..29f72916b7d 100644
--- a/pinyin-isearch.el
+++ b/pinyin-isearch.el
@@ -131,8 +131,7 @@ For pinyin search we enable search only if first syllable is real and
 
 By default we are looking for all characters and pinyin syllables that
  start with typed by user first part of syllable."
-  :local t
-  :group 'pinyin-isearch
+  :local t ;; FIXME: Why?
   :type 'boolean)
 
 (defcustom pinyin-isearch-full-fallback t
@@ -143,24 +142,20 @@ For pinyin search this means we use latin symbols in regex for vowels to
  search for letters without diacritical mark tones.  If is nil and
  syllable was not found pinyin search dont look for latin string.
 Used in `pinyin-isearch-chars--concat-variants'."
-  :local t
-  :group 'pinyin-isearch
+  :local t ;; FIXME: Why?
   :type 'boolean)
 
 (defcustom pinyin-isearch-default-mode 'both
   "Select target text in `pinyin-isearch-mode'.
-Whether to search for for pinyin or for Chinese characters or for
+Whether to search for pinyin or for Chinese characters or for
 both of them.  Used for mode `pinyin-isearch-mode', and functions
 `pinyin-isearch-forward', `pinyin-isearch-backward'."
-  :local t
-  :group 'pinyin-isearch
+  :local t ;; FIXME: Why?
   :type '(choice (const :tag "Search in both: pinyin and Chinese characters" both)
-                 (const :tag "Search in Chinese characters only, same as t" characters)
-                 (const :tag "Search in Chinese characters only, same as characters" t)
+                 (const :tag "Search in Chinese characters only" characters)
                  (const :tag "Search in both: pinyin and Chinese characters strictly" strict-both)
-                 (const :tag "Search in Chinese characters strictly, same as t" strict-characters)
-                 (const :tag "Search in pinyin only, same as nil" pinyin)
-                 (const :tag "Search in pinyin only, same as pinyin" nil)))
+                 (const :tag "Search in Chinese characters strictly" strict-characters)
+                 (const :tag "Search in pinyin only" pinyin)))
 
 (defcustom pinyin-isearch-fix-jumping-flag t
   "Non-nil means fix isearch behavior.
@@ -168,8 +163,7 @@ When typing new character the new search begins from last
 success found occurance, not from when you begin whole search.
 This fix force isearch to begin from the starting point.
 Disable for native isearch behavior."
-  :local t
-  :group 'pinyin-isearch
+  :local t ;; FIXME: Why?
   :type 'boolean)
 
 (defvar-local pinyin-isearch--original-search-default-mode search-default-mode
@@ -194,7 +188,7 @@ Optional argument LAX for isearch special cases."
      ((equal psr regexp-unmatchable) hsr)
      ((equal psr hsr) psr)
      (t
-      (concat "\\(" psr "\\|" hsr "\\)")))))
+      (concat "\\(?:" psr "\\|" hsr "\\)")))))
 
 (defun pinyin-isearch-both-strict-regexp-function (string &optional lax)
   "Strict variant of `pinyin-isearch-both-regexp-function'.
@@ -205,22 +199,26 @@ Argument STRING is a query string, LAX is not used."
     (pinyin-isearch-both-regexp-function string lax)))
 
 (defun pinyin-isearch--set-isearch ()
-  "For values of `pinyin-isearch-default-mode` return function.
+  "For values of `pinyin-isearch-default-mode' return function.
 Return function used to generate regex for isearch.
-According to `pinyin-isearch-default-mode`.
-Used in `pinyin-isearch-forward` and `pinyin-isearch-backward`."
+According to `pinyin-isearch-default-mode'.
+Used in `pinyin-isearch-forward' and `pinyin-isearch-backward'."
   (pcase pinyin-isearch-default-mode
     ('both              #'pinyin-isearch-both-regexp-function)
     ('strict-both       #'pinyin-isearch-both-strict-regexp-function)
     ('strict-characters #'pinyin-isearch-chars-strict-regexp-function)
-    ('characters        #'pinyin-isearch-chars-regexp-function)
-    ('t                 #'pinyin-isearch-chars-regexp-function)
-    ('pinyin            #'pinyin-isearch-pinyin-regexp-function)
-    ('nil               #'pinyin-isearch-pinyin-regexp-function)))
+    ((or 'characters 't) #'pinyin-isearch-chars-regexp-function)
+    (_                  #'pinyin-isearch-pinyin-regexp-function)))
 
 (defun pinyin-isearch--pinyin-fix-jumping-advice ()
   "Advice to fix isearch behavior.  Force search from a starting point."
+  ;; FIXME: I don't understand what this is really doing, or more specifically
+  ;; why it's pinyin-specific?  Doesn't the same behavior occur with normal
+  ;; Isearch?  If yes, then this should apply to all values of
+  ;; `isearch-regexp-function' and if not, please add a comment explaining the
+  ;; problem (which might be solvable better elsewhere).
   (if (and isearch-mode pinyin-isearch-fix-jumping-flag
+           ;; FIXME: Use `memq'.
            (or
             (eq isearch-regexp-function #'pinyin-isearch-pinyin-regexp-function)
             (eq isearch-regexp-function #'pinyin-isearch-chars-regexp-function)
@@ -275,6 +273,13 @@ Optional argument NO-RECURSIVE-EDIT see original function `isearch-backward'."
 
 ;; Generate the toggles globally (Emacs automatically binds them to M-s p, M-s h, M-s s)
 ;; Turns off normal mode.
+;; FIXME: These don't look like independent toggles.
+;; How 'bout having, say, 3 toggles (pinyin/chinese/strict) instead,
+;; where the `strict' toggle has effect only when chinese is enabled?
+;; Or even only 2, where the chinese "toggle" cycles between 3 settings?
+;; FIXME: Pressing `n' twice will set `isearch-regexp-function' to nil
+;; rather than to the previous setting, so it won't be pinyin search any more!
+;; FIXME: This defines `isearch-toggle-both' whose name is not namespace-clean.
 (isearch-define-mode-toggle "both" "n" pinyin-isearch-both-regexp-function
   "Turning on default search for pinyin and characters.")
 (isearch-define-mode-toggle "pinyin" "p" pinyin-isearch-pinyin-regexp-function
@@ -304,31 +309,33 @@ Optional argument NO-RECURSIVE-EDIT see original function `isearch-backward'."
 
     ;; Inherit from the standard isearch-mode M-s prefix map
     ;; This ensures standard options (like M-s _ for symbol search) still fall back correctly
+    ;; FIXME: Should not be necessary.
     (set-keymap-parent map (lookup-key isearch-mode-map (kbd "M-s")))
     map)
   "Keymap for \\[search-map] bindings inside `pinyin-isearch-mode'.")
 
-
 ;;;###autoload
 (define-minor-mode pinyin-isearch-mode
   "Replace key bindings for functions `isearch-forward' and `isearch-backward'.
-Allow with query {pinyin} to find {pīnyīn}.  \\C-\\u \\C-\\s used for
-normal search.
-- M -s p `isearch-toggle-pinyin' to activate pinyin isearch submode.
-- M -s h `isearch-toggle-characters' to activate Chinese characters
+Allow with query {pinyin} to find {pīnyīn}.
+Use \\[universal-argument] to get the normal search.
+- M-s p `isearch-toggle-pinyin' to activate pinyin isearch submode.
+- M-s h `isearch-toggle-characters' to activate Chinese characters
  isearch submode.
-- M -s s `isearch-toggle-strict-both' to activate Chinese and pinying
+- M-s s `isearch-toggle-strict-both' to activate Chinese and pinying
  characters isearch submode.
-- M -s u `isearch-toggle-strict-characters' to activate strict Chinese
+- M-s u `isearch-toggle-strict-characters' to activate strict Chinese
  characters isearch submode.
-- M -s n to activate default pinying-isearch mode.
-- M -s r to activate standard search."
+- M-s n to activate default pinying-isearch mode.
+- M-s r to activate standard search."
   :lighter " p-isearch"
-  :global nil
   :group 'pinyin-isearch
+  ;; FIXME: Move this to a (defvar pinyin-isearch-mode-map ...).
   :keymap (let ((map (make-sparse-keymap)))
+            ;; FIXME: Use [remap isearch-for/backward]?
             (define-key map (kbd "C-s") #'pinyin-isearch-forward)
             (define-key map (kbd "C-r") #'pinyin-isearch-backward)
+            ;; FIXME: This binding should be activated only during Isearch.
             (define-key map (kbd "M-s") pinyin-isearch-m-s-map)
             map)
   ;; Manage the hooks cleanly
@@ -336,6 +343,9 @@ normal search.
       (progn
         (pinyin-isearch-load)
         ;; used in all modes
+        ;; FIXME: AFAICT this hook function is needed only during Isearch,
+        ;; so move the add/remove-hook to the
+        ;; `pinyin-isearch-for/backward' commands?
         (add-hook 'pre-command-hook #'pinyin-isearch--pinyin-fix-jumping-advice nil t))
     ;; else
     (remove-hook 'pre-command-hook #'pinyin-isearch--pinyin-fix-jumping-advice t)))