Adding transient to Emacs core

Adding transient to Emacs core

[Jonas Bernoulli]

Hello,

A while go [1] we discussed adding Transient to Emacs core, not just GNU
Elpa.  The consensus appeared to be that it should be added to Emacs, so
that other core packages could use it, which I agree with.  I would like
to do that now.

I would prefer to continue maintaining Transient in its current
repository [2] and only import releases into the Emacs repository.
At first I would like to only import "transient.el", later also
"transient.texi".  (At this point friendlier documentation is the
area that needs the most work.)

So what do you think; should be proceed with this?

     Cheers,
     Jonas

[1]: id:e329f68a-21d2-6c1a-5528-44c4e8513091@posteo.net
     https://lists.gnu.org/archive/html/emacs-devel/2021-02/msg01384.html
[2]: [git clone] https://github.com/magit/transient.git

Re: Adding transient to Emacs core

[Eli Zaretskii]

> From: Jonas Bernoulli <jonas@bernoul.li>
> Date: Mon, 19 Apr 2021 17:51:43 +0200
> 
> A while go [1] we discussed adding Transient to Emacs core, not just GNU
> Elpa.  The consensus appeared to be that it should be added to Emacs, so
> that other core packages could use it, which I agree with.  I would like
> to do that now.
> 
> I would prefer to continue maintaining Transient in its current
> repository [2] and only import releases into the Emacs repository.
> At first I would like to only import "transient.el", later also
> "transient.texi".  (At this point friendlier documentation is the
> area that needs the most work.)
> 
> So what do you think; should be proceed with this?

Yes, please.  Externally maintained packages are nothing new in Emacs;
see Org, for example.

Thanks.

Re: Adding transient to Emacs core

[Jonas Bernoulli]

Eli Zaretskii <eliz@gnu.org> writes:

>> So what do you think; should [we] proceed with this?
>
> Yes, please.  Externally maintained packages are nothing new in Emacs;
> see Org, for example.

Should I just push this to master myself or open a ticket in debbugs?

Should I add it as "lisp/transient.el" or "lisp/emacs-lisp/transient.el"?

I'm also going to add this NEWS entry:

> ** transient.el
>
> This library implements support for powerful keyboard-driven menus.
> Such menus can be used as simple visual command dispatchers.  More
> complex menus take advantage of infix arguments, which are somewhat
> similar to prefix arguments, but are more flexible and discoverable.

Re: Adding transient to Emacs core

[Eli Zaretskii]

> From: Jonas Bernoulli <jonas@bernoul.li>
> Cc: emacs-devel@gnu.org
> Date: Tue, 20 Apr 2021 14:39:14 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> So what do you think; should [we] proceed with this?
> >
> > Yes, please.  Externally maintained packages are nothing new in Emacs;
> > see Org, for example.
> 
> Should I just push this to master myself or open a ticket in debbugs?

Depends on how you feel about its current form.  If you want some kind
of review and comments before pushing, debbugs is better.  But if you
are certain the code is in good shape and any comments are bound to be
minor, please go ahead and push.

> Should I add it as "lisp/transient.el" or "lisp/emacs-lisp/transient.el"?

IMO, lisp.  Too many defcustom's and defface's to consider this
low-level infrastructure.

> I'm also going to add this NEWS entry:
> 
> > ** transient.el
> >
> > This library implements support for powerful keyboard-driven menus.
> > Such menus can be used as simple visual command dispatchers.  More
> > complex menus take advantage of infix arguments, which are somewhat
> > similar to prefix arguments, but are more flexible and discoverable.

It's a start, thanks.

Re: Adding transient to Emacs core

[Jonas Bernoulli]

Eli Zaretskii <eliz@gnu.org> writes:

>> Should I just push this to master myself or open a ticket in debbugs?
>
> Depends on how you feel about its current form.  If you want some kind
> of review and comments before pushing, debbugs is better.  But if you
> are certain the code is in good shape and any comments are bound to be
> minor, please go ahead and push.

I've pushed.  Stefan Monnier already reviewed it last year and I've
addressed his feedback.

>> > ** transient.el
>> >
>> > This library implements support for powerful keyboard-driven menus.
>> > Such menus can be used as simple visual command dispatchers.  More
>> > complex menus take advantage of infix arguments, which are somewhat
>> > similar to prefix arguments, but are more flexible and discoverable.
>
> It's a start, thanks.

Yeah... ;) I believe I mentioned that documentation still has to be
improved. Sadly that begins with the most basic package description.

Re: Adding transient to Emacs core

[Kévin Le Gouguec]

Jonas Bernoulli <jonas@bernoul.li> writes:

> Yeah... ;) I believe I mentioned that documentation still has to be
> improved. Sadly that begins with the most basic package description.

I understand that you maintain an Org manual for Transient in your
GitHub repository, which is automatically converted to a Texinfo manual?
IIUC, some manuals in Emacs core are also maintained as Org documents
and converted to Texinfo as part of the build process; see
e.g. doc/misc/org.org and doc/misc/modus-themes.org.

Hope that helps (and that I'm actually correct about how the manuals in
Emacs core are maintained), sorry for the noise if not.

Re: Adding transient to Emacs core

[Stefan Kangas]

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> Jonas Bernoulli <jonas@bernoul.li> writes:
>
>> Yeah... ;) I believe I mentioned that documentation still has to be
>> improved. Sadly that begins with the most basic package description.
>
> I understand that you maintain an Org manual for Transient in your
> GitHub repository, which is automatically converted to a Texinfo manual?
> IIUC, some manuals in Emacs core are also maintained as Org documents
> and converted to Texinfo as part of the build process; see
> e.g. doc/misc/org.org and doc/misc/modus-themes.org.
>
> Hope that helps (and that I'm actually correct about how the manuals in
> Emacs core are maintained), sorry for the noise if not.

You are correct, see:

    doc/misc/modus-themes.org
    doc/misc/org-setup.org
    doc/misc/org.org

Re: Adding transient to Emacs core

[Dmitry Gutov]

On 19.04.2021 18:51, Jonas Bernoulli wrote:
> The consensus appeared to be that it should be added to Emacs, so
> that other core packages could use it, which I agree with.

Could we add some usages of it as well, to justify the addition?

Or at least a list of such places.

Re: Adding transient to Emacs core

[Jonas Bernoulli]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 19.04.2021 18:51, Jonas Bernoulli wrote:
>> The consensus appeared to be that it should be added to Emacs, so
>> that other core packages could use it, which I agree with.
>
> Could we add some usages of it as well, to justify the addition?

Well several people mentioned that they would start using it once it
is in core.  So that can happen now.

> Or at least a list of such places.

I also have implemented a few commands for built-in packages, that I
intend to submit soon.  Here's a few in list form:

- epa-dispatch
- epa-mail-dispatch
- epa-key-list-dispatch
- mml[-dispatch]

Re: Adding transient to Emacs core

[Dmitry Gutov]

On 20.04.2021 19:59, Jonas Bernoulli wrote:
>> Could we add some usages of it as well, to justify the addition?
> Well several people mentioned that they would start using it once it
> is in core.  So that can happen now.
> 
>> Or at least a list of such places.
> I also have implemented a few commands for built-in packages, that I
> intend to submit soon.  Here's a few in list form:
> 
> - epa-dispatch
> - epa-mail-dispatch
> - epa-key-list-dispatch
> - mml[-dispatch]

Thanks!

Re: Adding transient to Emacs core

[Madhu]

* Jonas Bernoulli <87czuqi86o.fsf@bernoul.li> :
Wrote on Mon, 19 Apr 2021 17:51:43 +0200:
> A while go [1] we discussed adding Transient to Emacs core, not just GNU
> Elpa.  The consensus appeared to be that it should be added to Emacs, so
> that other core packages could use it, which I agree with.  I would like
> to do that now.
>
> I would prefer to continue maintaining Transient in its current
> repository [2] and only import releases into the Emacs repository.
> At first I would like to only import "transient.el", later also
> "transient.texi".  (At this point friendlier documentation is the
> area that needs the most work.)
>
> So what do you think; should be proceed with this?


I think transient.el has serious design problems with respect to emacs.

I had sent the following to jonas back in 2019, and got a response
asking me to open a github account - which I did not have at that time.
Until these issues are addressed I would prefer that new code in the
core not use transient.el

- <20190717.093415.1664326708841095012.enometh@meer.net>

Hello - some comments on transient.el

* c7ad1f01f4ff9e5125bcec99dfb9c3dedadfc369
| Author:     Jonas Bernoulli <jonas@bernoul.li>
| AuthorDate: Thu May 9 12:25:48 2019 +0200
|
|Transient expects the popup buffer to be displayed in a new window.
|If that is not the case, then it won't work properly and this commit
|does not do anything to make it work properly anyway (because that
|is not possible).

This design leads to spectacular failure modes with emacs and does not
really close issue #44.  The assumption in transient's code is that
display-buffer-in-side-window will always succeed but that is not how
display-buffer is specified to work:

If a window cannot be popped up in the current frame display-buffer
will create a new frame.

Transient mode must accomodate this or it must disclaim to emacs
support and clearly specify that it is fundamentally incompatible with
emacs (since you are using display-buffer to display this pop-up
window you are effectively supporting user customization of
display-buffer. In which case you should not use display-buffer at all
since you cannot support legitimate use of display-buffer)

Effectively the transient abstraction as it stands is incompatible
with emacs design and cannot be made to work with it. Besides the new
window is not scrollable or searchable, it doesnt support any standard
emacs operations.  Effectively transient-mdoe destroys the value of
any packages written on top of it.

|Regardless, the user should still be able to exit the transient state
|even when things are broken like that, but doing so failed if the
|transient popup was displayed in the only window because we ended up
|trying to delete window, which is not allowed.  Continue to try to
|kill the transient window, but if that fails, then continue with the
|rest of the exit procedure.

with-demoted-errors does not always work in killing off the new frame,
and emacs gets into a generally irrecoverable state (unless you have
taken precautions after having studied the transient.el's failure
modes before)

Re: Adding transient to Emacs core

[Eli Zaretskii]

> From: Madhu <enometh@meer.net>
> Date: Mon, 26 Apr 2021 08:00:37 +0530
> 
> I think transient.el has serious design problems with respect to emacs.

Then please describe those problems in detail, preferably in a bug
report.  (You allude here to some discussions you had with Jonas, but
we weren't part of them, and so those references tell us nothing
useful.)

> Until these issues are addressed I would prefer that new code in the
> core not use transient.el

We cannot agree with this conclusion unless we understand the details.

Thanks.

Re: Adding transient to Emacs core

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Madhu <enometh@meer.net>
>> Date: Mon, 26 Apr 2021 08:00:37 +0530
>> 
>> I think transient.el has serious design problems with respect to emacs.
>
> Then please describe those problems in detail, preferably in a bug
> report.  (You allude here to some discussions you had with Jonas, but
> we weren't part of them, and so those references tell us nothing
> useful.)

It seems Madhu is referencing this commit:

https://github.com/magit/transient/commit/c7ad1f01f4ff9e5125bcec99dfb9c3dedadfc369

and these discussions

https://github.com/magit/transient/issues/34
https://github.com/magit/transient/issues/44

that claim transient breaks with an unusual display-buffer-alist
configuration.

>> Until these issues are addressed I would prefer that new code in the
>> core not use transient.el
>
> We cannot agree with this conclusion unless we understand the details.
>
> Thanks.

-- 
	Philip K.

Re: Adding transient to Emacs core

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: Madhu <enometh@meer.net>,  emacs-devel@gnu.org
> Date: Mon, 26 Apr 2021 12:54:55 +0000
> 
> > Then please describe those problems in detail, preferably in a bug
> > report.  (You allude here to some discussions you had with Jonas, but
> > we weren't part of them, and so those references tell us nothing
> > useful.)
> 
> It seems Madhu is referencing this commit:
> 
> https://github.com/magit/transient/commit/c7ad1f01f4ff9e5125bcec99dfb9c3dedadfc369
> 
> and these discussions
> 
> https://github.com/magit/transient/issues/34
> https://github.com/magit/transient/issues/44
> 
> that claim transient breaks with an unusual display-buffer-alist
> configuration.

Thanks, but I'd still prefer to have the details spelled out in our
issue tracker.  E.g., some of us might wish to avoid accessing GitHub
repositories.

Re: Adding transient to Emacs core

[Madhu]

*  Eli Zaretskii <eliz@gnu.org> <83r1ixjmaw.fsf@gnu.org>
Wrote on Mon, 26 Apr 2021 14:51:51 +0300
> Then please describe those problems in detail, preferably in a bug
> report.

I believe I did that in sufficient detail - my message detailed the
exact assumptions made by jonas' code and the exact scenarios in which
those assumptions fail.

Did you read it for the details?

>  (You allude here to some discussions you had with Jonas, but
> we weren't part of them, and so those references tell us nothing
> useful.)

>> Until these issues are addressed I would prefer that new code in the
>> core not use transient.el
>
> We cannot agree with this conclusion unless we understand the details.

I believe Jonas understands the problems but isn't concerned about the
failure scenarios.

The main issue is there can be (very reasonable) situations in which
you will not be able to give emacs input when prompted by transient.

Re: Adding transient to Emacs core

[Eli Zaretskii]

> Date: Mon, 26 Apr 2021 23:26:19 +0530 (IST)
> Cc: emacs-devel@gnu.org
> From: Madhu <enometh@meer.net>
> 
> *  Eli Zaretskii <eliz@gnu.org> <83r1ixjmaw.fsf@gnu.org>
> Wrote on Mon, 26 Apr 2021 14:51:51 +0300
> > Then please describe those problems in detail, preferably in a bug
> > report.
> 
> I believe I did that in sufficient detail - my message detailed the
> exact assumptions made by jonas' code and the exact scenarios in which
> those assumptions fail.
> 
> Did you read it for the details?

Of course.  It still left me wondering about the specifics.

> >> Until these issues are addressed I would prefer that new code in the
> >> core not use transient.el
> >
> > We cannot agree with this conclusion unless we understand the details.
> 
> I believe Jonas understands the problems but isn't concerned about the
> failure scenarios.

I would like to understand the problem myself in sufficient detail.

> The main issue is there can be (very reasonable) situations in which
> you will not be able to give emacs input when prompted by transient.

I asked to describe them in enough detail for someone like me to
understand and be able to reason about them.

Re: Adding transient to Emacs core

[Eli Zaretskii]

> Date: Tue, 27 Apr 2021 07:39:03 +0530 (IST)
> From: Madhu <enometh@meer.net>
> 
> >> I believe I did that in sufficient detail - my message detailed the
> >> exact assumptions made by jonas' code and the exact scenarios in which
> >> those assumptions fail.
> >>
> >> Did you read it for the details?
> >
> > Of course.  It still left me wondering about the specifics.
> 
> The specifics follow when these situations are triggered.  I hope you
> are not being purposely obtuse.

I'm asking for a specific recipe where the problems rear their head.
Can you please provide such a recipe, or recipes if there are multiple
issues?

> > I asked to describe them in enough detail for someone like me to
> > understand and be able to reason about them.
> 
> Please write me an actual test program which you can use which uses
> transient to prompt for input and I will try provide the settings
> which might cause a problem.

I'm sorry, I don't have time to do that: too many things on my plate.
I'm asking you to use one of the programs where you did have those
problems already, I'm guessing it won't be hard for you, since you
already bumped into such problems.

Re: Adding transient to Emacs core

[Jonas Bernoulli]

Madhu <enometh@meer.net> writes:

> I think transient.el has serious design problems with respect to emacs.
>
> I had sent the following to jonas back in 2019, and got a response
> asking me to open a github account - which I did not have at that
> time.

What bothered me more than the fact that you contacted my privately, is
the wording of your communication, the highlight being:

> Effectively transient-mdoe destroys the value of any packages written
> on top of it.

I have replied to your mail by asking you to refrain from remarks like
this and that you communicate publicly.  I requested the latter in the
hope that that would help with the former.

It is disappointing that you ignore that request to refrain from such
hyperbole and instead copy your original message verbatim.  (Except for
the weird preface "[private mail please do not respool on google/public
servers]").

Just re-sending a mail from two years ago also means that it ignores
all technical improvements since than.

> Besides the new window is not scrollable or searchable, it doesnt
> support any standard emacs operations.

No longer true, but you have to explicitly enable it with:

  (setq transient-enable-popup-navigation t)

> with-demoted-errors does not always work in killing off the new frame,
> and emacs gets into a generally irrecoverable state (unless you have
> taken precautions after having studied the transient.el's failure
> modes before)

This no longer happens. I have figured out the last remaining heisenbug
that could put Emacs in an inconsistent state months ago and there have
been zero new reports since then.  The error handling also has improved
substantially, so even if something does go spectacularly wrong, this
should no longer put Emacs in an inconsistent state.

> This design leads to spectacular failure modes with emacs and does not
> really close issue #44.  The assumption in transient's code is that
> display-buffer-in-side-window will always succeed but that is not how
> display-buffer is specified to work:
>
> If a window cannot be popped up in the current frame display-buffer
> will create a new frame.

There is room for improvement, but the current worst case scenario is
the following.  Note that this can only happen if the current frame is
(1) less than ~5 lines high, (2) no other frame already exists, and (3)
the window manager selects the newly created frame.  In that case a new
frame briefly flashes and disappears again.  I.e. it is impossible to
invoke the suffixes of that transient prefix command, but Emacs does not
enter any "spectacular failure modes" (no transient keymaps and/or hooks
get stuck).  You can simply increase the frame size and then invoke the
transient prefix again.

Fixing this remaining issue might be as easy as:

  (define-key transient-predicate-map
    [handle-switch-frame] 'transient--do-stay)

This is not the default yet but I might do this or something similar in
the near future.  This also makes it possible to:

  (setq transient-display-buffer-action
        '(display-buffer-pop-up-frame))

And even without that there were several alternative display actions
available that work just fine, e.g.:

  (setq transient-display-buffer-action
        '(display-buffer-in-child-frame))

  (setq transient-display-buffer-action
        '(display-buffer-below-selected))

> (since you are using display-buffer to display this pop-up window you
> are effectively supporting user customization of display-buffer. In
> which case you should not use display-buffer at all since you cannot
> support legitimate use of display-buffer)

Yes, transient does not support every conceivable display action, but
neither does anything else.  What you are saying here is "you don't
support all possible customizations, therefore you should offer none".


     Jonas

Re: Adding transient to Emacs core

[Madhu]

*  Jonas Bernoulli <jonas@bernoul.li> <87pmyhdvla.fsf@bernoul.li>
Wrote on Mon, 26 Apr 2021 15:27:45 +0200

> I have replied to your mail by asking you to refrain from remarks like
> this and that you communicate publicly.  I requested the latter in the
> hope that that would help with the former.

[I apologize - I wasn't sensitized enough to your sensibilities.]

> Just re-sending a mail from two years ago also means that it ignores
> all technical improvements since than.

This may be true: the version I was using was a commit from June 2020.

>> Besides the new window is not scrollable or searchable, it doesnt
>> support any standard emacs operations.
>
> No longer true, but you have to explicitly enable it with:
>
>   (setq transient-enable-popup-navigation t)
>
>> with-demoted-errors does not always work in killing off the new frame,
>> and emacs gets into a generally irrecoverable state (unless you have
>> taken precautions after having studied the transient.el's failure
>> modes before)
>
> This no longer happens. I have figured out the last remaining heisenbug
> that could put Emacs in an inconsistent state months ago and there have
> been zero new reports since then.  The error handling also has improved
> substantially, so even if something does go spectacularly wrong, this
> should no longer put Emacs in an inconsistent state.

I assume this is on the current `master' branch - and will be using
this shortly.


>> This design leads to spectacular failure modes with emacs and does not
>> really close issue #44.  The assumption in transient's code is that
>> display-buffer-in-side-window will always succeed but that is not how
>> display-buffer is specified to work:
>>
>> If a window cannot be popped up in the current frame display-buffer
>> will create a new frame.
>
> There is room for improvement, but the current worst case scenario is
> the following.  Note that this can only happen if the current frame is
> (1) less than ~5 lines high, (2) no other frame already exists, and (3)
> the window manager selects the newly created frame.  In that case a new
> frame briefly flashes and disappears again.  I.e. it is impossible to
> invoke the suffixes of that transient prefix command, but Emacs does not
> enter any "spectacular failure modes" (no transient keymaps and/or hooks
> get stuck).  You can simply increase the frame size and then invoke the
> transient prefix again.

(setq pop-up-windows nil) is a valid setting.

The spectacular scenarios I've encountered are when the frame did
*not* go away - and you cannot or control emacs and have to switch to
a different terminal and kill it.

I hope you understand the inconvenience that this sort of situation
causes.

I understans that you say this has been addressed.


> Fixing this remaining issue might be as easy as:
>
>   (define-key transient-predicate-map
>     [handle-switch-frame] 'transient--do-stay)
>
> This is not the default yet but I might do this or something similar in
> the near future.  This also makes it possible to:
>
>   (setq transient-display-buffer-action
>         '(display-buffer-pop-up-frame))
>
> And even without that there were several alternative display actions
> available that work just fine, e.g.:
>
>   (setq transient-display-buffer-action
>         '(display-buffer-in-child-frame))
>
>   (setq transient-display-buffer-action
>         '(display-buffer-below-selected))

>> (since you are using display-buffer to display this pop-up window you
>> are effectively supporting user customization of display-buffer. In
>> which case you should not use display-buffer at all since you cannot
>> support legitimate use of display-buffer)
>
> Yes, transient does not support every conceivable display action, but
> neither does anything else.  What you are saying here is "you don't
> support all possible customizations, therefore you should offer none".

If you say that (pop-up-windows nil) is not a valid customization, I
would strongly disagree with that.

If transient cannot handle input for some configuration then there
should be a fallback to emacs mechanisms that *can* handle input.

If the package does not support use display-buffer according to the
design of display-buffer, I maintain it will have a negative impact if
adopted in the core and one is constrained to use it (instead of
keeping it optional)

Re: Adding transient to Emacs core

[Eli Zaretskii]

> Date: Mon, 26 Apr 2021 23:03:18 +0530 (IST)
> From: Madhu <enometh@meer.net>
> Cc: emacs-devel@gnu.org
> 
> If you say that (pop-up-windows nil) is not a valid customization, I
> would strongly disagree with that.
> 
> If transient cannot handle input for some configuration then there
> should be a fallback to emacs mechanisms that *can* handle input.
> 
> If the package does not support use display-buffer according to the
> design of display-buffer, I maintain it will have a negative impact if
> adopted in the core and one is constrained to use it (instead of
> keeping it optional)

Once again: we need specific facts about the current behavior of
transient.el in these circumstances.  Let's not discuss problems that
could have happened theoretically, let's describe the real problems
with the situations where they happen, and let's discuss how to solve
those problems (if they do indeed happen).

A bug report with the details is still the best medium for such
practical discussions.

Re: Adding transient to Emacs core

[Stefan Monnier]

> If the package does not support use display-buffer according to the
> design of display-buffer, I maintain it will have a negative impact if
> adopted in the core and one is constrained to use it (instead of
> keeping it optional)

In my experience, among the packages that have to display something (as
opposed to your usual major-mode which only provides commands to
navigate and edit text), very few correctly deal with all possible
`display-buffer-alist` customizations.  The vast majority assumes
a single-frame setup and break in random way in less
conventional situations.

So if transient.el misbehaves in some of those cases, it's in pretty
good company.

In the transient.el API the only thing that could inherently cause
problem, AFAICT is the fact that it relies on being able to
display a "sizeable" amount of information to the user, but it's hard to
imagine a comparable feature that doesn't make similar assumptions, and
it's hard to imagine a situation where there's really no way to make
it work.

So any problem it might suffer should be a mere "implementation issue"
that can be fixed without impacting the packages which rely on it.


        Stefan

Re: Adding transient to Emacs core

[Madhu]

*  Stefan Monnier <monnier@iro.umontreal.ca> <jwvwnspos54.fsf-monnier+emacs@gnu.org>
Wrote on Mon, 26 Apr 2021 13:52:49 -0400

>> If the package does not support use display-buffer according to the
>> design of display-buffer, I maintain it will have a negative impact if
>> adopted in the core and one is constrained to use it (instead of
>> keeping it optional)

> In my experience, among the packages that have to display something
> (as opposed to your usual major-mode which only provides commands to
> navigate and edit text), very few correctly deal with all possible
> `display-buffer-alist` customizations.  The vast majority assumes a
> single-frame setup and break in random way in less conventional
> situations.
>
> So if transient.el misbehaves in some of those cases, it's in pretty
> good company.

And I have the option of not being forced to use those packages when
dealing with emacs.

> In the transient.el API the only thing that could inherently cause
> problem, AFAICT is the fact that it relies on being able to
> display a "sizeable" amount of information to the user, but it's hard to

No this is pretty fundamental. The designer of the transient prompt
can make the prompt big enough to trigger the situations where it
fails.

> imagine a comparable feature that doesn't make similar assumptions, and
> it's hard to imagine a situation where there's really no way to make
> it work.
>
> So any problem it might suffer should be a mere "implementation issue"
> that can be fixed without impacting the packages which rely on it.

Except when they can't because of funamental incompatible assumptions.

(Note I am responding at the same level of generality as your post)

If a core package which worked fine is now modified to use a package
which you introduce and the new package and cannot be used in certain
circumstances (that the developer of the new package has not or cannot
consider) I maintain that the new package destroys existing value -
and if that is the explict intent I hope it is made explicit.

Re: Adding transient to Emacs core

[Stefan Monnier]

Madhu [2021-04-27 07:33:17] wrote:
> *  Stefan Monnier <monnier@iro.umontreal.ca> <jwvwnspos54.fsf-monnier+emacs@gnu.org>
> Wrote on Mon, 26 Apr 2021 13:52:49 -0400
>
>>> If the package does not support use display-buffer according to the
>>> design of display-buffer, I maintain it will have a negative impact if
>>> adopted in the core and one is constrained to use it (instead of
>>> keeping it optional)
>
>> In my experience, among the packages that have to display something
>> (as opposed to your usual major-mode which only provides commands to
>> navigate and edit text), very few correctly deal with all possible
>> `display-buffer-alist` customizations.  The vast majority assumes a
>> single-frame setup and break in random way in less conventional
>> situations.
>>
>> So if transient.el misbehaves in some of those cases, it's in pretty
>> good company.
>
> And I have the option of not being forced to use those packages when
> dealing with emacs.

I'm tempted to guess that you're "young and naïve" because this affects
many core packages that you can't conveniently avoid
(e.g. `read-from-minibuffer` comes to mind).  You just happened not to
bump into those problems because your config is not sufficiently
unusual, or because you find the resulting misbehavior normal.

Admittedly, the situation is a lot better now than back in Emacs-21.

>> In the transient.el API the only thing that could inherently cause
>> problem, AFAICT is the fact that it relies on being able to
>> display a "sizeable" amount of information to the user, but it's hard to
>
> No this is pretty fundamental. The designer of the transient prompt
> can make the prompt big enough to trigger the situations where
> it fails.

Such claims are useless (if not counter productive) without concrete
evidence to back them up.

> (Note I am responding at the same level of generality as your post)

I don't think I can get much more specific about my statement since my
statement is one of absence of problem, for which it's difficult
by nature to provide evidence.

In contrast you claim failure modes, for which you should be able to
provide evidence quite easily.

> If a core package which worked fine is now modified to use a package
> which you introduce and the new package and cannot be used in certain
> circumstances (that the developer of the new package has not or cannot
> consider) I maintain that the new package destroys existing value -
> and if that is the explict intent I hope it is made explicit.

Then I suggest when that happens you report a bug against the change
which made the pre-existing feature start relying on `transient`.

Until that happens, all `transient` does is provide a new functionality
for use in new features, so all you can do is provide evidence that its
design is poor, ideally by providing alternative designs that don't
suffer from the problems you see (of course, you'll first need to
explain what those problems are in order to get some traction).


        Stefan

Re: Adding transient to Emacs core

[Jonas Bernoulli]

Madhu <enometh@meer.net> writes:

> [I apologize - I wasn't sensitized enough to your sensibilities.]

Thanks!

> I assume this is on the current `master' branch - and will be using
> this shortly.

I pushed it to transient's master a few months ago.  The version in
Emacs includes it as well.

> I understans that you say this has been addressed.

Correct.

> If you say that (pop-up-windows nil) is not a valid customization, I
> would strongly disagree with that.

I have addressed this by let-binding that variable to t around the call
to display-buffer.  There's just no way around that because transient's
buffer just has to be displayed somewhere other than the selected
window. (Of course you can display it int another frame instead of in
another window of the same frame, but then this binding should cause no
offense, because in this scenario has no effect.)

[I haven't pushed that to Emacs yet, but you can find it on transient's
own master branch.]

> If transient cannot handle input for some configuration then there
> should be a fallback to emacs mechanisms that *can* handle input.

I consider the above binding to be such a fallback.

> If the package does not support use display-buffer according to the
> design of display-buffer, I maintain it will have a negative impact if
> adopted in the core and one is constrained to use it (instead of
> keeping it optional)

You can (setq transient-show-popup nil) to get this behavior: "If nil,
then do not show the popup unless the user explicitly requests it, by
pressing an incomplete prefix key sequence."

Hm, you probably don't like that "an incomplete prefix key sequence"
triggers the popup to be shown still.  I might implement a "never"
variant that never ever shows bindings the way transient was designed to
do it, but instead makes `describe-bindings` available.  That would add
a feel to it that is very similar to that of regular prefix commands.

Re: Adding transient to Emacs core

[Philip Kaludercic]

Jonas Bernoulli <jonas@bernoul.li> writes:

> I have addressed this by let-binding that variable to t around the call
> to display-buffer.  There's just no way around that because transient's
> buffer just has to be displayed somewhere other than the selected
> window.

What is the issue with displaying the buffer in the selected window? It
seems like something interesting, if I don't want my frames to be
rearranged.

-- 
	Philip K.

Re: Adding transient to Emacs core

[Gregory Heytings]

>
> I have addressed this by let-binding that variable to t around the call 
> to display-buffer.  There's just no way around that because transient's 
> buffer just has to be displayed somewhere other than the selected 
> window.
>

I agree with Philip here: why is there "just no way around that"?  At 
least when the frame is too small, it could make sense to display the 
transient buffer "above" the selected window.

Re: Adding transient to Emacs core

[Jonas Bernoulli]

Gregory Heytings <gregory@heytings.org> writes:

>>
>> I have addressed this by let-binding that variable to t around the call 
>> to display-buffer.  There's just no way around that because transient's 
>> buffer just has to be displayed somewhere other than the selected 
>> window.
>>
>
> I agree with Philip here: why is there "just no way around that"?  At 
> least when the frame is too small, it could make sense to display the 
> transient buffer "above" the selected window.

Many Emacs commands operate on the "current thing".  Different commands
treat different things as the current thing.  It could be the selected
frame or window, the current buffer, line, character, value of a certain
text property at point, ...

`kill-this-buffer' for example operates on the current buffer.

Any Emacs command can be added as the suffix of a transient prefix
command, for example:

  (transient-define-prefix my-buffer-commands ()
    "Do stuff to buffers."
    [:description (lambda () (format "Act on current buffer (%s)" (current-buffer)))
     ("k" "kill the current buffer" kill-this-buffer)])

  (global-set-key [f1] 'my-buffer-commands)

If the selected window were repurposed to display transient's buffer,
then that would change what buffer is the current buffer and it would
become impossible to act on the buffer that was previously the current
buffer or on "the thing under the cursor" in that buffer.

Re-purposing the selected window would massively reduce the usefulness
of a huge number of commands or even make them completely useless.

Re: Adding transient to Emacs core

[Gregory Heytings]

>
> If the selected window were repurposed to display transient's buffer, 
> then that would change what buffer is the current buffer and it would 
> become impossible to act on the buffer that was previously the current 
> buffer or on "the thing under the cursor" in that buffer.
>

Yes, I understand this, but when the frame is too small to display both 
the current window and the transient buffer window, would it not be better 
to do that anyway, and to restore the previously current buffer before 
executing the command?

Re: Adding transient to Emacs core

[Jonas Bernoulli]

Gregory Heytings <gregory@heytings.org> writes:

>>
>> If the selected window were repurposed to display transient's buffer, 
>> then that would change what buffer is the current buffer and it would 
>> become impossible to act on the buffer that was previously the current 
>> buffer or on "the thing under the cursor" in that buffer.
>>
>
> Yes, I understand this, but when the frame is too small to display both 
> the current window and the transient buffer window, would it not be better 
> to do that anyway, and to restore the previously current buffer before 
> executing the command?

I guess it is a matter of preference.

This is the default value of `transient-display-buffer-action:

    (display-buffer-in-side-window
     (side . bottom)
     (inhibit-same-window . t))

The (inhibit-same-window . t) is there to *prevent* it from falling back
to that behavior.

One way of dealing with the fact that different people want different
fallback behavior in this case would be to add a note about this to the
doc-string.

Another possible fallback would be to not display the popup buffer at
all when the selected frame isn't high enough and to instead display a
message in the echo area about what the issue is, while still allowing
the user to invoke suffix commands.

All of this will require some thought and experimentation.  Please share
yours and give me the time I need to try things out.

Re: Adding transient to Emacs core

[Philip Kaludercic]

Jonas Bernoulli <jonas@bernoul.li> writes:

> Any Emacs command can be added as the suffix of a transient prefix
> command, for example:
>
>   (transient-define-prefix my-buffer-commands ()
>     "Do stuff to buffers."
>     [:description (lambda () (format "Act on current buffer (%s)" (current-buffer)))
>      ("k" "kill the current buffer" kill-this-buffer)])
>
>   (global-set-key [f1] 'my-buffer-commands)

I just tried this out, and it seems that transient it very different
compared to magit-popup, that powers my local version of magit (from
MELPA Stable).

I am uncertain how I feel about this, as it seems to be something
completely different than what I was thinking about all the time.

At the same time it does not seem obvious why, especially with transient
menus that may contain a lot of options (say a ffmpeg interface)
shouldn't be able to use the entire frame. Especially from a user
perspective.

> If the selected window were repurposed to display transient's buffer,
> then that would change what buffer is the current buffer and it would
> become impossible to act on the buffer that was previously the current
> buffer or on "the thing under the cursor" in that buffer.

What do you mean by "what buffer is the current buffer"? I am somewhat
confused by what you are trying to say here.

> Re-purposing the selected window would massively reduce the usefulness
> of a huge number of commands or even make them completely useless.

Again, I do not see how this follows? My verison of Magit has
magit-display-buffer-function, that allows me to display the buffer in
the selected window, with no loss of functionality. What does transient
do or need that prevents this?

-- 
	Philip K.

Re: Adding transient to Emacs core

[Jonas Bernoulli]

Philip Kaludercic <philipk@posteo.net> writes:

> At the same time it does not seem obvious why, especially with transient
> menus that may contain a lot of options (say a ffmpeg interface)
> shouldn't be able to use the entire frame. Especially from a user
> perspective.

It is of course possible that a certain transient menu only features
suffix commands that do not care which buffer/window/frame is current.
And in the future I might add a feature that would allow the author of
such a menu to indicate that that is the case and that the entire frame
should be used to display that particular menu. But that a potential new
feature and not what we are currently discussing.

>> If the selected window were repurposed to display transient's buffer,
>> then that would change what buffer is the current buffer and it would
>> become impossible to act on the buffer that was previously the current
>> buffer or on "the thing under the cursor" in that buffer.
>
> What do you mean by "what buffer is the current buffer"? I am somewhat
> confused by what you are trying to say here.

Would it be less confusing if instead I had said "WHICH buffer is the
current buffer"?  I don't think I can describe this any better; maybe
someone else can.  I'm having problems understanding what you find
confusing about that paragraph.

>> Re-purposing the selected window would massively reduce the usefulness
>> of a huge number of commands or even make them completely useless.
>
> Again, I do not see how this follows? My verison of Magit has
> magit-display-buffer-function, that allows me to display the buffer in
> the selected window, with no loss of functionality.  What does
> transient do or need that prevents this?

As I have tried (and apparently failed) to explain earlier, it is
important to some suffix commands that the same thing is current at the
time they are invoked as was current when the menu was entered using the
prefix command.

This is true regardless of whether Magit-Popup or Transient is used,
but these two implementation differ in how they ensure this:

- Magit-Popup does it by (1) remembering what buffer is current by
  recording the current window configuration when the prefix is invoked
  but before actually displaying the menu in a buffer.  Later when the
  user invokes a suffix, it (2) intercepts that action in order to (3)
  restore the saved window configuration before (4) actually invoking
  the suffix command using `call-interactively'.

- Transient ensures that the buffer that was current before the menu was
  displayed, simply by not messing that up in the first place.

So yes, Magit-Popup can make the menu buffer the current buffer "with no
loss of functionality", but that is only the case because it explicitly
makes sure the old window-configuration is restored before it matters.

Ultimately it does not matter: everything that is possible with one
approach is also possible with the other, but having implemented both,
I do have a favorite.

It would be useful if users would describe the concrete behaviors that
they find undesirable, instead of criticizing implementation details
they might not fully understand.  There has been a lot of concrete,
actionable feedback in the past and I have listened to that and made
improvements in response.

Re: Adding transient to Emacs core

[martin rudalics]

 >> If you say that (pop-up-windows nil) is not a valid customization, I
 >> would strongly disagree with that.
 >
 > I have addressed this by let-binding that variable to t around the call
 > to display-buffer.  There's just no way around that because transient's
 > buffer just has to be displayed somewhere other than the selected
 > window.

This would constitute a major bug in `display-buffer' - `pop-up-windows'
is semi-obsolete ("provided for backward compatibility only").  The
canonical way to do what you want is to provide an

(inhibit-same-window . t)

action alist entry.

martin

Re: Adding transient to Emacs core

[Jonas Bernoulli]

martin rudalics <rudalics@gmx.at> writes:

>  >> If you say that (pop-up-windows nil) is not a valid customization, I
>  >> would strongly disagree with that.
>  >
>  > I have addressed this by let-binding that variable to t around the call
>  > to display-buffer.  There's just no way around that because transient's
>  > buffer just has to be displayed somewhere other than the selected
>  > window.
>
> This would constitute a major bug in `display-buffer' - `pop-up-windows'
> is semi-obsolete ("provided for backward compatibility only").  The
> canonical way to do what you want is to provide an
>
> (inhibit-same-window . t)
>
> action alist entry.

We were already using (inhibit-same-window . t) and indeed if I remove
the let-binding, then things continue to function.  I am removing this
unnecessary kludge again.