bug#60740: [PATCH 0/2] emoji changes

bug#60740: [PATCH 0/2] emoji changes

[Jonas Bernoulli]

* The first commit changes `isearch-emoji-by-name' to not use
  transient.  If there are derivations then `completing-read'
  is instead used to let the user chose one.

  I think we should stick to that.  What I mention next, seems
  way to risky at this time and I also do not think it would be
  worth it.  Selecting a derivation using `completing-read' isn't
  bad at all.

  (The problem with using a transient command inside
  `with-isearch-suspended' is that expects to be able to call a
  function, which uses `recursive-edit' in some way.  Once that
  returns, the macro resumes isearch.  But transient does not do that
  and it returns immediately after the transient prefix command
  returns.  That happens before the user selects a derivation by
  invoking one of the suffix commands.  So isearch resumes before
  the user has made any choice.

  One way around that might be to call `recursive-edit' in
  `isearch-emoji-by-name' before calling the code that might then use
  transient.  But that would also have to automatically invoke a
  command inside that recursive edit.  I couldn't find a way to do
  that.  I timer might work, but that seems hacky.

  Another approach could be to break up `with-isearch-suspended' up
  into two functions, one that suspends and another that resumes.
  The suspend function could maybe also responsible for returning
  the resume function as a closure.  The macro could use these two
  functions and existing callers could continue to use that.  But
  `isearch-emoji-by-name' would only call the suspend function and
  would have to somehow pass the resume function to transient, so
  that it could call it when the time has come.)

* The second commit changes emoji.el to use transient.el the way I
  intended it to be used.  (I *think* transient already had support
  for "dynamic" transient commands, when emoji.el was created, but
  some of the convenience features certainly were missing still.) 

  The new implementation changes the remaining entry points to be
  defined using `define-transient-prefix' but still without adding any
  suffix commands to them up front.  Previously no *prefix* command
  was defined up front.  Instead code very similar to what can be
  found in `define-transient-prefix' was used to turn certain nodes
  inside the tree of emoji into (sub-)prefix commands.  These prefix
  commands were then invoked using `funcall', which is not how that
  is supposed to work; I am surprised that worked at all, but I guess
  that just means that emoji doesn't use any of transient's features
  that depend on `this-command' "being correct".

  The new implementation adds a single, named suffix command,
  `emoji-insert-glyph'.  Now every suffix of a transient prefix is
  either that command or a "recursive" call to the prefix itself, with
  its scope narrowed to only part of the original tree of emoji.  That
  is a big improvement over the old implementation, which had to
  define a new command for each and every emoji, and a new command for
  every grouping of emoji.

Lars, previously a hash-table was used to track the emoji, that should
not be derived any further.  I changed that to a list, because I am
yet to encounter a situation where more than one element has to be
tracked.  Are there even any derivations of derivations?  If so,
please point me to an example.

If there are any, then the first commit would have to be adjusted to
account for that.  Otherwise, I think that is ready.  I would still
like to test the second commit a bit more, before applying it.

     Cheers,
     Jonas

bug#60740: [PATCH 1/2] No longer use transient in isearch-emoji-by-name

[Jonas Bernoulli]

* lisp/isearch.el (isearch-emoji-by-name): Use 'emoji--read-emoji'
and if that returns derivations, 'completing-read' to select one
of them.  This fixes bug#60740.
* lisp/international/emoji.el (emoji--init): Autoload.
(emoji--read-emoji): New function, which doesn't use transient
and returns a list of the glyph and all derivations, if any.
(emoji--choose-emoji): Use 'emoji--read-emoji'.
---
 lisp/international/emoji.el | 36 ++++++++++++++++++++----------------
 lisp/isearch.el             | 20 ++++++++------------
 2 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/lisp/international/emoji.el b/lisp/international/emoji.el
index 2d17cf639b0..f75bd877991 100644
--- a/lisp/international/emoji.el
+++ b/lisp/international/emoji.el
@@ -245,6 +245,7 @@ emoji-list-help
           (error "Emoji name is unknown")
         (message "%s" name)))))
 
+;;;###autoload
 (defun emoji--init (&optional force inhibit-adjust)
   (when (or (not emoji--labels)
             force)
@@ -638,7 +639,7 @@ emoji--split-long-lists
                          collect (cons (concat (string prefix) "-group")
                                        (seq-take bit 77))))))))
 
-(defun emoji--choose-emoji ()
+(defun emoji--read-emoji ()
   ;; Use the list of names.
   (let* ((table
           (if (not emoji-alternate-names)
@@ -678,21 +679,24 @@ emoji--choose-emoji
 	       (complete-with-action action table string pred)))
            nil t)))
     (when (cl-plusp (length name))
-      (let* ((glyph (if emoji-alternate-names
-                        (cadr (split-string name "\t"))
-                      (gethash name emoji--all-bases)))
-             (derived (gethash glyph emoji--derived)))
-        (if (not derived)
-            ;; Simple glyph with no derivations.
-            (progn
-              (emoji--add-recent glyph)
-              (insert glyph))
-          ;; Choose a derived version.
-          (let ((emoji--done-derived (make-hash-table :test #'equal)))
-            (setf (gethash glyph emoji--done-derived) t)
-            (funcall
-             (emoji--define-transient
-              (cons "Choose Emoji" (cons glyph derived))))))))))
+      (let ((glyph (if emoji-alternate-names
+                       (cadr (split-string name "\t"))
+                     (gethash name emoji--all-bases))))
+        (cons glyph (gethash glyph emoji--derived))))))
+
+(defun emoji--choose-emoji ()
+  (pcase-let ((`(,glyph ,derived)) (emoji--read-emoji))
+    (if (not derived)
+        ;; Simple glyph with no derivations.
+        (progn
+          (emoji--add-recent glyph)
+          (insert glyph))
+      ;; Choose a derived version.
+      (let ((emoji--done-derived (make-hash-table :test #'equal)))
+        (setf (gethash glyph emoji--done-derived) t)
+        (funcall
+         (emoji--define-transient
+          (cons "Choose Emoji" (cons glyph derived))))))))
 
 (defvar-keymap emoji-zoom-map
   "+" #'emoji-zoom-increase
diff --git a/lisp/isearch.el b/lisp/isearch.el
index 22e27764127..bfa71756146 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -2774,25 +2774,21 @@ isearch-char-by-name
 					   (mapconcat 'isearch-text-char-description
 						      string ""))))))))
 
-(defvar emoji--derived)
 (defun isearch-emoji-by-name (&optional count)
   "Read an Emoji name and add it to the search string COUNT times.
 COUNT (interactively, the prefix argument) defaults to 1.
 The command accepts Unicode names like \"smiling face\" or
 \"heart with arrow\", and completion is available."
   (interactive "p")
+  (emoji--init)
   (with-isearch-suspended
-   (let ((emoji (with-temp-buffer
-                  ;; Derived emoji not supported yet (bug#60740).
-                  ;; So first load `emoji--labels', then `emoji--init'
-                  ;; will not fill `emoji--derived' that is set
-                  ;; to an empty hash table below.
-                  (ignore-errors (require 'emoji-labels))
-                  (let ((emoji--derived (make-hash-table :test #'equal)))
-                    (emoji-search))
-                  (if (and (integerp count) (> count 1))
-                      (apply 'concat (make-list count (buffer-string)))
-                    (buffer-string)))))
+   (pcase-let* ((`(,glyph . ,derived) (emoji--read-emoji))
+                (emoji (if derived
+                           (completing-read "Select derivation: "
+                                            (cons glyph derived) nil t)
+                         glyph)))
+     (when (and (integerp count) (> count 1))
+       (setq emoji (apply 'concat (make-list count emoji))))
      (when emoji
        (setq isearch-new-string (concat isearch-string emoji)
              isearch-new-message (concat isearch-message
-- 
2.39.1

bug#60740: [PATCH 2/2] Rewrite emoji's usage of transient

[Jonas Bernoulli]

* lisp/international/emoji.el:
(emoji--done-derived): Remove variable.
(emoji-insert, emoji-recent, emoji-search, emoji-list-select):
Define using 'transient-define-prefix'.  Use a base suffix group
whose value is calculated dynamically.
(emoji--setup-prefix, emoji--setup-suffixes, emoji-group-description):
New functions used to dynamically calculate suffixes.
(emoji--narrow): New suffix class, used to pass state to recursive,
narrowed invocations of the prefix command the user initially invoked.
(emoji-insert-glyph): New suffix command that is used for all glyphs
that have no derivations.  Previously a separate command was define
for each glyph.
(emoji--fontify-glyph): Replace 'inhibit-derived' argument with
'done-derived' argument.
(emoji--define-transient): Remove function.
(emoji--layout): New function, replacing 'emoji--define-transient'.
Return the suffixes in 'define-transient-prefix' format.  Unlike
the replaced function, do not define any new commands, instead use
either the current prefix command or 'emoji-insert-glyph'.
(emoji--recent-transient): Remove function.
(emoji--char-sequence): New function.
(emoji--add-recent): Remove all text properties from glyph.
(emoji--choose-emoji): Remove function.
---
 lisp/international/emoji.el | 266 +++++++++++++++++-------------------
 1 file changed, 125 insertions(+), 141 deletions(-)

diff --git a/lisp/international/emoji.el b/lisp/international/emoji.el
index f75bd877991..b920582fee0 100644
--- a/lisp/international/emoji.el
+++ b/lisp/international/emoji.el
@@ -68,38 +68,86 @@ emoji--labels
 (defvar emoji--all-bases nil)
 (defvar emoji--derived nil)
 (defvar emoji--names (make-hash-table :test #'equal))
-(defvar emoji--done-derived nil)
 (define-multisession-variable emoji--recent (list "😀" "😖"))
 (defvar emoji--insert-buffer)
 
-;;;###autoload
-(defun emoji-insert ()
+;;;###autoload (autoload 'emoji-insert "emoji" nil t)
+(transient-define-prefix emoji-insert ()
   "Choose and insert an emoji glyph."
+  :variable-pitch t
+  [:class transient-columns
+   :setup-children emoji--setup-suffixes
+   :description emoji--group-description]
   (interactive "*")
   (emoji--init)
-  (unless (fboundp 'emoji--command-Emoji)
-    (emoji--define-transient))
-  (funcall (intern "emoji--command-Emoji")))
+  (emoji--setup-prefix 'emoji-insert "Emoji" nil
+                       `(("Recent" ,@(multisession-value emoji--recent))
+                         ,@emoji--labels)))
 
-;;;###autoload
-(defun emoji-recent ()
+;;;###autoload (autoload 'emoji-recent "emoji" nil t)
+(transient-define-prefix emoji-recent ()
   "Choose and insert one of the recently-used emoji glyphs."
+  :variable-pitch t
+  [:class transient-columns
+   :setup-children emoji--setup-suffixes
+   :description emoji--group-description]
   (interactive "*")
   (emoji--init)
-  (unless (fboundp 'emoji--command-Emoji)
-    (emoji--define-transient))
-  (funcall (emoji--define-transient
-            (cons "Recent" (multisession-value emoji--recent)) t)))
+  (emoji--setup-prefix 'emoji-recent "Recent" t
+                       (multisession-value emoji--recent)))
 
-;;;###autoload
-(defun emoji-search ()
+;;;###autoload (autoload 'emoji-search "emoji" nil t)
+(transient-define-prefix emoji-search ()
   "Choose and insert an emoji glyph by typing its Unicode name.
 This command prompts for an emoji name, with completion, and
 inserts it.  It recognizes the Unicode Standard names of emoji,
 and also consults the `emoji-alternate-names' alist."
+  :variable-pitch t
+  [:class transient-columns
+   :setup-children emoji--setup-suffixes
+   :description emoji--group-description]
   (interactive "*")
   (emoji--init)
-  (emoji--choose-emoji))
+  (pcase-let ((`(,glyph . ,derived) (emoji--read-emoji)))
+    (if derived
+        (emoji--setup-prefix 'emoji-search "Choose Emoji"
+                             (list glyph)
+                             (cons glyph derived))
+      (emoji--add-recent glyph)
+      (insert glyph))))
+
+(defun emoji--setup-prefix (command title done-derived spec)
+  (transient-setup
+   command nil nil
+   :scope (if (eq transient-current-command command)
+              (cons (oref (transient-suffix-object) title)
+                    (oref (transient-suffix-object) done-derived))
+            (cons title done-derived))
+   :value (if (eq transient-current-command command)
+              (oref (transient-suffix-object) children)
+            spec)))
+
+(defun emoji--setup-suffixes (_)
+  (transient-parse-suffixes
+   (oref transient--prefix command)
+   (pcase-let ((`(,title . ,done-derived) (oref transient--prefix scope)))
+     (emoji--layout (oref transient--prefix command) title
+                    (oref transient--prefix value) done-derived))))
+
+(defun emoji--group-description ()
+  (car (oref transient--prefix scope)))
+
+(defclass emoji--narrow (transient-suffix)
+  ((title :initarg :title)
+   (done-derived :initarg :done-derived)
+   (children :initarg :children)))
+
+(transient-define-suffix emoji-insert-glyph ()
+  "Insert the emoji you selected."
+  (interactive nil not-a-mode)
+  (let ((glyph (oref (transient-suffix-object) description)))
+    (emoji--add-recent glyph)
+    (insert glyph)))
 
 ;;;###autoload
 (defun emoji-list ()
@@ -179,11 +227,10 @@ emoji--list-generate
                  'help-echo (emoji--name glyph))))
       (insert "\n\n"))))
 
-(defun emoji--fontify-glyph (glyph &optional inhibit-derived)
+(defun emoji--fontify-glyph (glyph &optional done-derived)
   (propertize glyph 'face
-              (if (and (not inhibit-derived)
-                       (or (null emoji--done-derived)
-                           (not (gethash glyph emoji--done-derived)))
+              (if (and (not (or (eq done-derived t)
+                                (member glyph done-derived)))
                        (gethash glyph emoji--derived))
                   ;; If this emoji has derivations, use a special face
                   ;; to tell the user.
@@ -206,33 +253,30 @@ emoji-list-mode
   :interactive nil
   (setq-local truncate-lines t))
 
-(defun emoji-list-select (event)
+;;;###autoload (autoload 'emoji-list-select "emoji" nil t)
+(transient-define-prefix emoji-list-select (event)
   "Select the emoji under point."
+  :variable-pitch t
+  [:class transient-columns
+   :setup-children emoji--setup-suffixes
+   :description emoji--group-description]
   (interactive (list last-nonmenu-event) emoji-list-mode)
   (mouse-set-point event)
   (let ((glyph (get-text-property (point) 'emoji-glyph)))
     (unless glyph
       (error "No emoji under point"))
-    (let ((derived (gethash glyph emoji--derived))
-          (end-func
-           (lambda ()
-             (let ((buf emoji--insert-buffer))
-               (quit-window)
-               (if (buffer-live-p buf)
-                   (switch-to-buffer buf)
-                 (error "Buffer disappeared"))))))
-      (if (not derived)
-          ;; Glyph without derivations.
-          (progn
-            (emoji--add-recent glyph)
-            (funcall end-func)
-            (insert glyph))
-        ;; Pop up a transient to choose between derivations.
-        (let ((emoji--done-derived (make-hash-table :test #'equal)))
-          (setf (gethash glyph emoji--done-derived) t)
-          (funcall
-           (emoji--define-transient (cons "Choose Emoji" (cons glyph derived))
-                                    nil end-func)))))))
+    (let ((buf emoji--insert-buffer))
+      (quit-window)
+      (if (buffer-live-p buf)
+          (switch-to-buffer buf)
+        (error "Buffer disappeared")))
+    (let ((derived (gethash glyph emoji--derived)))
+      (if derived
+          (emoji--setup-prefix 'emoji-list-select "Choose Emoji"
+                               (list glyph)
+                               (cons glyph derived))
+        (emoji--add-recent glyph)
+        (insert glyph)))))
 
 (defun emoji-list-help ()
   "Display the name of the emoji at point."
@@ -476,97 +520,51 @@ emoji--add-glyph
         (setq parent elem))
       (nconc elem (list glyph)))))
 
-(defun emoji--define-transient (&optional alist inhibit-derived
-                                          end-function)
-  (unless alist
-    (setq alist (cons "Emoji" emoji--labels)))
-  (let* ((mname (pop alist))
-         (name (intern (format "emoji--command-%s" mname)))
-         (emoji--done-derived (or emoji--done-derived
-                                  (make-hash-table :test #'equal)))
-         (has-subs (consp (cadr alist)))
-         (layout
-          (if has-subs
-              ;; Define sub-maps.
-              (cl-loop for entry in
-                       (emoji--compute-prefix
-                        (if (equal mname "Emoji")
-                            (cons (list "Recent") alist)
-                          alist))
-                       collect (list
-                                (car entry)
-                                (emoji--compute-name (cdr entry))
-                                (if (equal (cadr entry) "Recent")
-                                    (emoji--recent-transient end-function)
-                                  (emoji--define-transient
-                                   (cons (concat mname " > " (cadr entry))
-                                         (cddr entry))))))
-            ;; Insert an emoji.
-            (cl-loop for glyph in alist
-                     for i in (append (number-sequence ?a ?z)
-                                      (number-sequence ?A ?Z)
-                                      (number-sequence ?0 ?9)
-                                      (number-sequence ?! ?/))
-                     collect (let ((this-glyph glyph))
-                               (list
-                                (string i)
-                                (emoji--fontify-glyph
-                                 glyph inhibit-derived)
-                                (let ((derived
-                                       (and (not inhibit-derived)
-                                            (not (gethash glyph
-                                                          emoji--done-derived))
-                                            (gethash glyph emoji--derived))))
-                                  (if derived
-                                      ;; We have a derived glyph, so add
-                                      ;; another level.
-                                      (progn
-                                        (setf (gethash glyph
-                                                       emoji--done-derived)
-                                              t)
-                                        (emoji--define-transient
-                                         (cons (concat mname " " glyph)
-                                               (cons glyph derived))
-                                         t end-function))
-                                    ;; Insert the emoji.
-                                    (lambda ()
-                                      (interactive nil not-a-mode)
-                                      ;; Allow switching to the correct
-                                      ;; buffer.
-                                      (when end-function
-                                        (funcall end-function))
-                                      (emoji--add-recent this-glyph)
-                                      (insert this-glyph)))))))))
-         (args (apply #'vector mname
-                      (emoji--columnize layout
-                                        (if has-subs 2 8)))))
-    ;; There's probably a better way to do this...
-    (setf (symbol-function name)
-          (lambda ()
-            (interactive nil not-a-mode)
-            (transient-setup name)))
-    (pcase-let ((`(,class ,slots ,suffixes ,docstr ,_body)
-                 (transient--expand-define-args (list args))))
-       (put name 'interactive-only t)
-       (put name 'function-documentation docstr)
-       (put name 'transient--prefix
-            (apply (or class 'transient-prefix) :command name
-                   (cons :variable-pitch (cons t slots))))
-       (put name 'transient--layout
-            (transient-parse-suffixes name suffixes)))
-    name))
-
-(defun emoji--recent-transient (end-function)
-  "Create a function to display a dynamically generated menu."
-  (lambda ()
-    (interactive)
-    (funcall (emoji--define-transient
-              (cons "Recent" (multisession-value emoji--recent))
-              t end-function))))
+(defun emoji--layout (command title spec done-derived)
+  (let ((has-subs (consp (cadr spec))))
+    (emoji--columnize
+     (if has-subs
+         (cl-loop for (key desc . glyphs) in (emoji--compute-prefix spec)
+                  collect
+                  (list key
+                        (emoji--compute-name (cons desc glyphs))
+                        command
+                        :class 'emoji--narrow
+                        :title (concat title " > " desc)
+                        :done-derived (or (string-suffix-p "Recent" desc)
+                                          done-derived)
+                        :children glyphs))
+       (cl-loop for glyph in spec
+                for char in (emoji--char-sequence)
+                for key = (string char)
+                for derived = (and (not (or (eq done-derived t)
+                                            (member glyph done-derived)))
+                                   (gethash glyph emoji--derived))
+                collect
+                (if derived
+                    (list key
+                          (emoji--fontify-glyph glyph done-derived)
+                          command
+                          :class 'emoji--narrow
+                          :title (concat title " " glyph)
+                          :done-derived (or (eq done-derived t)
+                                            (cons glyph done-derived))
+                          :children (cons glyph derived))
+                  (list key
+                        (emoji--fontify-glyph glyph done-derived)
+                        'emoji-insert-glyph))))
+     (if has-subs 2 8))))
+
+(defun emoji--char-sequence ()
+  (append (number-sequence ?a ?z)
+          (number-sequence ?A ?Z)
+          (number-sequence ?0 ?9)
+          (number-sequence ?! ?/)))
 
 (defun emoji--add-recent (glyph)
   "Add GLYPH to the set of recently used emojis."
   (let ((recent (multisession-value emoji--recent)))
+    (set-text-properties 0 (length glyph) nil glyph)
     (setq recent (delete glyph recent))
     (push glyph recent)
     ;; Shorten the list.
@@ -684,20 +682,6 @@ emoji--read-emoji
                      (gethash name emoji--all-bases))))
         (cons glyph (gethash glyph emoji--derived))))))
 
-(defun emoji--choose-emoji ()
-  (pcase-let ((`(,glyph ,derived)) (emoji--read-emoji))
-    (if (not derived)
-        ;; Simple glyph with no derivations.
-        (progn
-          (emoji--add-recent glyph)
-          (insert glyph))
-      ;; Choose a derived version.
-      (let ((emoji--done-derived (make-hash-table :test #'equal)))
-        (setf (gethash glyph emoji--done-derived) t)
-        (funcall
-         (emoji--define-transient
-          (cons "Choose Emoji" (cons glyph derived))))))))
-
 (defvar-keymap emoji-zoom-map
   "+" #'emoji-zoom-increase
   "-" #'emoji-zoom-decrease)
-- 
2.39.1

bug#60740: [PATCH 0/2] emoji changes

[Eli Zaretskii]

> Cc: larsi@gnus.org
> From: Jonas Bernoulli <jonas@bernoul.li>
> Date: Thu,  2 Feb 2023 00:09:16 +0100
> 
> * The first commit changes `isearch-emoji-by-name' to not use
>   transient.  If there are derivations then `completing-read'
>   is instead used to let the user chose one.
> 
>   I think we should stick to that.  What I mention next, seems
>   way to risky at this time and I also do not think it would be
>   worth it.  Selecting a derivation using `completing-read' isn't
>   bad at all.
> 
>   (The problem with using a transient command inside
>   `with-isearch-suspended' is that expects to be able to call a
>   function, which uses `recursive-edit' in some way.  Once that
>   returns, the macro resumes isearch.  But transient does not do that
>   and it returns immediately after the transient prefix command
>   returns.  That happens before the user selects a derivation by
>   invoking one of the suffix commands.  So isearch resumes before
>   the user has made any choice.
> 
>   One way around that might be to call `recursive-edit' in
>   `isearch-emoji-by-name' before calling the code that might then use
>   transient.  But that would also have to automatically invoke a
>   command inside that recursive edit.  I couldn't find a way to do
>   that.  I timer might work, but that seems hacky.
> 
>   Another approach could be to break up `with-isearch-suspended' up
>   into two functions, one that suspends and another that resumes.
>   The suspend function could maybe also responsible for returning
>   the resume function as a closure.  The macro could use these two
>   functions and existing callers could continue to use that.  But
>   `isearch-emoji-by-name' would only call the suspend function and
>   would have to somehow pass the resume function to transient, so
>   that it could call it when the time has come.)
> 
> * The second commit changes emoji.el to use transient.el the way I
>   intended it to be used.  (I *think* transient already had support
>   for "dynamic" transient commands, when emoji.el was created, but
>   some of the convenience features certainly were missing still.) 
> 
>   The new implementation changes the remaining entry points to be
>   defined using `define-transient-prefix' but still without adding any
>   suffix commands to them up front.  Previously no *prefix* command
>   was defined up front.  Instead code very similar to what can be
>   found in `define-transient-prefix' was used to turn certain nodes
>   inside the tree of emoji into (sub-)prefix commands.  These prefix
>   commands were then invoked using `funcall', which is not how that
>   is supposed to work; I am surprised that worked at all, but I guess
>   that just means that emoji doesn't use any of transient's features
>   that depend on `this-command' "being correct".
> 
>   The new implementation adds a single, named suffix command,
>   `emoji-insert-glyph'.  Now every suffix of a transient prefix is
>   either that command or a "recursive" call to the prefix itself, with
>   its scope narrowed to only part of the original tree of emoji.  That
>   is a big improvement over the old implementation, which had to
>   define a new command for each and every emoji, and a new command for
>   every grouping of emoji.

Thanks.

Would it make sense to install the first patch on the emacs-29 branch
and the second on master?  Or did I misunderstand your explanations
above?  AFAIU, the first patch solves the immediate issue in this bug,
right?

bug#60740: [PATCH 0/2] emoji changes

[Jonas Bernoulli]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks.
>
> Would it make sense to install the first patch on the emacs-29 branch
> and the second on master?  Or did I misunderstand your explanations
> above?  AFAIU, the first patch solves the immediate issue in this bug,
> right?

That's correct and it would make sense to do it like this.  At the same
time I think that it is risky both to install and not install the second
patch on emacs-29.  On the one hand past changes to transient did not
break emoji, even though I did not specifically try to prevent that.  On
the other the risk of that happening going forward is not zero.  If Lars
finds the time to look at the refactoring before the pre-release, I
think it would be best to install it on emacs-29, but if not, I am find
with it going on master instead.

bug#60740: [PATCH 0/2] emoji changes

[Eli Zaretskii]

> From: Jonas Bernoulli <jonas@bernoul.li>
> Cc: 60740@debbugs.gnu.org, larsi@gnus.org
> Date: Sat, 04 Feb 2023 13:05:28 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Thanks.
> >
> > Would it make sense to install the first patch on the emacs-29 branch
> > and the second on master?  Or did I misunderstand your explanations
> > above?  AFAIU, the first patch solves the immediate issue in this bug,
> > right?
> 
> That's correct and it would make sense to do it like this.  At the same
> time I think that it is risky both to install and not install the second
> patch on emacs-29.  On the one hand past changes to transient did not
> break emoji, even though I did not specifically try to prevent that.  On
> the other the risk of that happening going forward is not zero.  If Lars
> finds the time to look at the refactoring before the pre-release, I
> think it would be best to install it on emacs-29, but if not, I am find
> with it going on master instead.

My suggestion is for now to install the first patch on emacs-29 and
the second one on master.  We can always backport from master to
emacs-29 if we decide so later.

From my POV at this time, it is important to resolve the few remaining
issues that prevent us from starting the Emacs 29 pretest, and this
issue is one of them.  Which is why I suggest to do the above now, and
consider backporting the second patch to emacs-29 later if we have
time.

Thanks.

bug#60740: [PATCH 0/2] emoji changes

[Jonas Bernoulli]

Eli Zaretskii <eliz@gnu.org> writes:

> My suggestion is for now to install the first patch on emacs-29 and
> the second one on master.

I've installed the first and will install the second once someone else
has merged emacs-29 into master.

> We can always backport from master to emacs-29 if we decide so later.
>
> From my POV at this time, it is important to resolve the few remaining
> issues that prevent us from starting the Emacs 29 pretest, and this
> issue is one of them.  Which is why I suggest to do the above now, and
> consider backporting the second patch to emacs-29 later if we have
> time.

I didn't realize that was on option, and assumed that after the
pre-release is done, it would get *much* more difficult to get a
non-bugfix installed than before.  I now assume it is only "quite
a bit harder, but better that than delaying the pre-release". ;P

bug#60740: [PATCH 0/2] emoji changes

[Eli Zaretskii]

> From: Jonas Bernoulli <jonas@bernoul.li>
> Cc: 60740@debbugs.gnu.org, larsi@gnus.org
> Date: Sun, 05 Feb 2023 15:40:48 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > My suggestion is for now to install the first patch on emacs-29 and
> > the second one on master.
> 
> I've installed the first and will install the second once someone else
> has merged emacs-29 into master.

OK, thanks.

bug#60740: [PATCH 0/2] emoji changes

[Eli Zaretskii]

> From: Jonas Bernoulli <jonas@bernoul.li>
> Cc: 60740@debbugs.gnu.org, larsi@gnus.org
> Date: Sun, 05 Feb 2023 15:40:48 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > My suggestion is for now to install the first patch on emacs-29 and
> > the second one on master.
> 
> I've installed the first

This apparently leads to

    ELC      ../lisp/isearch.elc

  In end of data:
  isearch.el:2785:40: Warning: the function `emoji--read-emoji' is not known to be defined.

    ELC      international/emoji.elc

  In emoji--choose-emoji:
  international/emoji.el:688:37: Warning: Unused lexical variable `emoji--read-emoji'

bug#60740: [PATCH 0/2] emoji changes

[Jonas Bernoulli]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jonas Bernoulli <jonas@bernoul.li>
>> Cc: 60740@debbugs.gnu.org, larsi@gnus.org
>> Date: Sun, 05 Feb 2023 15:40:48 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > My suggestion is for now to install the first patch on emacs-29 and
>> > the second one on master.
>> 
>> I've installed the first
>
> This apparently leads to

I looked at the log to prevent that, but I think I just redirected
stdout and forgot about stderr.  Byte-compiler warnings go to stderr,
right?

>     ELC      ../lisp/isearch.elc
>
>   In end of data:
>   isearch.el:2785:40: Warning: the function `emoji--read-emoji' is not
>   known to be defined.

What's the usual way of dealing with this in Emacs itself?  I wasn't
sure using an autoload would work here, and was going to use another
approach if this resulted in a warning.  Unfortunately I missed the
warning.  Should I use 'declare-function' instead and require 'emoji'
inside 'isearch-emoji-by-name'?

>     ELC      international/emoji.elc
>
>   In emoji--choose-emoji:
>   international/emoji.el:688:37: Warning: Unused lexical variable
>   `emoji--read-emoji'

Yikes.

bug#60740: [PATCH 0/2] emoji changes

[Eli Zaretskii]

> From: Jonas Bernoulli <jonas@bernoul.li>
> Cc: 60740@debbugs.gnu.org, larsi@gnus.org
> Date: Sun, 05 Feb 2023 17:29:12 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > This apparently leads to
> 
> I looked at the log to prevent that, but I think I just redirected
> stdout and forgot about stderr.  Byte-compiler warnings go to stderr,
> right?

In batch mode, yes.

> >     ELC      ../lisp/isearch.elc
> >
> >   In end of data:
> >   isearch.el:2785:40: Warning: the function `emoji--read-emoji' is not
> >   known to be defined.
> 
> What's the usual way of dealing with this in Emacs itself?  I wasn't
> sure using an autoload would work here, and was going to use another
> approach if this resulted in a warning.  Unfortunately I missed the
> warning.  Should I use 'declare-function' instead and require 'emoji'
> inside 'isearch-emoji-by-name'?

The below is better, IMO.  I installed it.

diff --git a/lisp/isearch.el b/lisp/isearch.el
index bfa7175..62ac6f1 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -2773,7 +2773,7 @@ isearch-char-by-name
 	       isearch-new-message (concat isearch-message
 					   (mapconcat 'isearch-text-char-description
 						      string ""))))))))

+(autoload 'emoji--read-emoji "emoji")
 (defun isearch-emoji-by-name (&optional count)
   "Read an Emoji name and add it to the search string COUNT times.
 COUNT (interactively, the prefix argument) defaults to 1.