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

From: Eli Zaretskii <[email-masked]>
Date: Sat, 04 Feb 2023 10:30:36 +0200

> Cc: [email-masked]
> From: Jonas Bernoulli <[email-masked]>
> 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?