Some nits about the new popup

[benma]

I was trying to look for previous issues, apologies if I missed some.

magit-version: Magit 20190215.1903, Git 2.17.1, Emacs 25.2.2, gnu/linux

I upgraded today and saw that the popup buffer behaves differently (I guess with the switch to transient?). My small nit is that it takes ownership too much. Concretely it would be nice if it did not hijack all my usual emacs bindings, especially the C-x bindings, and the windmove shift-{left,up,right,down} keys to move between windows.

The other small nit: it was great being able to treat the popup as a normal buffer, where I could enter and do C-s to search quickly for a command instead of reading through all of them to find the one I need.

Finally, can you give me a quick pointer how to restore q as an alias for C-g to quit magit popups? At the moment, q still works for e.g. log, stash, revision buffers, but not the popups.

As always, thanks for all the great work!

[tarsius]

My small nit is that it takes ownership too much. Concretely it would be nice if it did not hijack all my usual emacs bindings, especially the C-x bindings, and the windmove shift-{left,up,right,down} keys to move between windows.

I see transient as a kind of prefix key and/or prefix argument on steroids. From that perspective it makes sense to not allow users to use other bindings while the transient is active:

• If you press C-x intending to press C-f to open a file next, but realize that you don't know the filename, then you also cannot invoke whatever command would give you the answer, and then finish with C-f. Instead you have to cancel the incomplete C-x ... key sequence first.

• The same goes for prefix arguments. If you prepare the prefix arguments intending to call a certain command and then realize that you have to call another command first, then you cannot remember the prefix for a later command; it always applies to the very next command.

Transients can be "suspended" using C-z and then resumed using transient-resume (no default binding). So unlike for prefix arguments you can actually abort entering infix arguments, do something else, and then resume with the arguments from before. It's not the same, but maybe you will come to like this feature.

---

While a transient is active the current-buffer is actually the buffer from which the transient was invoked. Transient does not restore that as the current buffer before invoking a suffix or infix command. This makes it possible to act on the thing that was and still is at point and allows one suffix command (which does not exit the transient) to intentionally change the current buffer and/or point, so that the next suffix acts on that location instead.

Allowing arbitrary commands to change those things too would mean that a suffix command wouldn't be able to do that for the benefit of the next suffix because then we would be forced to restore the location before invoking a suffix.

That's not very relevant right now because all existing transients are used to pass arguments to some suffix command (or as plain command dispatchers), but eventually I want to use transients for the sort of thing that hydras are used for, i.e. "sticky prefixes". By the way, Hydra has these limitations too.

---

it was great being able to treat the popup as a normal buffer, where I could enter and do C-s to search quickly for a command instead of reading through all of them to find the one I need.

Someone else mentioned this on Reddit. This is a use-case that had not occurred to me. Unfortunately using isearch in transient is not possible. It probably would be possible to implement a isearch-like feature, but it would not be as good, hard to implement, and probably a bit brittle. I'm afraid you will have to live without this. At least I don't intend to implement this.

---

I am sorry to say that you probably won't get these things back. On the bright side, that covers all lost functionality I believe, so at least you are not in for any other surprises.

---

Finally, can you give me a quick pointer how to restore q as an alias for C-g to quit magit popups? At the moment, q still works for e.g. log, stash, revision buffers, but not the popups.

That's possible:

(define-key transient-map        "q" 'transient-quit-one)
(define-key transient-edit-map   "q" 'transient-quit-one)
(define-key transient-sticky-map "q" 'transient-quit-seq)

But that will shadow every transient-specific binding for q. Currently this would affect magit-blame and magit-file-dispatch. You would lose the ability to remove blame information without invoking magit-blame-quit directly instead of from a transient.

I have chosen C-g because it is an Emacsy way of aborting an incomplete key sequence, at least more so than q, and because I wanted to make all characters available as keys that can be used for transient bindings.

[kaiwk]

Hi, there. Is there any way to keep using magit-popup in the future? Because magit popup splits window inside, so that it won't change frame layout.

[tarsius]

Maybe doing something like isearch isn't as hard as I imagine initially. I'll probably take a stab at it eventually but it could be a while. Using isearch itself could be problematic because it also uses transient keymaps.

Hi, there. Is there any way to keep using magit-popup in the future?

No.

Because magit popup splits window inside, so that it won't change frame layout.

I don't quite understand what that means. What worked with magit-popup but doesn't work anymore with transient? I think lv messes less with the frame layout because it just uses the echo area -- no window splitting necessary at all.

[michael-heerdegen]

Is there somewhere some documentation about the transition? I used to call in my init file magit-define-popup-* functions, that is now all broken, there were no obsoletion warnings, things just broke with the last upgrade. I guess not all Magit users follow the development process...

I now added a hard require to magit-popup, which I reinstalled, to my init file so I can use Emacs again. Are there replacements for the define-popup-* functions?

[tarsius]

I have added some dummy functions with instructions in 732de659a57914cf0624edfb261b470ad046a3e1.

[kyleam]

And in case a concrete example is useful: Here's the diff of my configuration's conversion from magit-popup to transient. I'm still new to the transient stuff, so it might not be the best way, but it works.

[michael-heerdegen]

@tarsius Thanks, looks good, haven't tried it though. @kyleam Also thanks, I'll have a look.

[michael-heerdegen]

Oh, the wiki page has not yet been done. Let's see if kyleams posted diff is sufficient.

[michael-heerdegen]

Hmm, seems it's not trivial. I'll wait until the wiki page has been filled. Thanks so far.

[michael-heerdegen]

@tarsius Ok, thanks for the wiki. Starting to migrate... One thing that happened so far: seems that it's not easily possible to use transient-append-suffix to add multiple entries after the same place. When I e.g.

(transient-append-suffix 'magit-stash "i" '("j" "index keep worktree" my-magit-stash-index))

and try to add something after the new "j", I get an error not telling me much. Adding something else after "i" also doesn't work as expected: say I try to insert a "k" also after "i", then the "j" entry seems to be removed.

[michael-heerdegen]

Ok, the rest worked. BTW, you only have to have "vdiff-magit" installed to trigger the warning. I found the culprit by adding (debug) to the definition of 'magit--magit-popup-warning'. BTW2 If the warning gets triggered, I only see the second half of the warning text, point being at eob. I guess that's how display-warning just works. Thanks for your work!

[benma]

@tarsius thanks a lot.

From my side the issue is resolved. Feel free to close it when you see fit.

[michael-heerdegen]

FWIW, display-warning logs all warnings in one buffer, one warning after the other, so point has to be at the end of the buffer. If you care, you could use your own popup buffer, but it's really not important.

[michael-heerdegen]

One more thing: The manual that is shipped with the Elpa package still uses obsolete names, e.g. "magit-run-popup" or "magit-dispatch-popup". These commands don't work anymore, so this should probably be fixed soon.

[ejgallego]

I don't quite understand what that means. What worked with magit-popup but doesn't work anymore with transient? I think lv messes less with the frame layout because it just uses the echo area -- no window splitting necessary at all.

For the usual split window configuration that means that we get a full width popup which is quite unnatural on large screens, as compared to a popup confined to the magit window.

[tarsius]

I like the simplicity of lv, but it has become clear that we need more flexibility than it provides. I am going to look into writing a replacement. Contributing to lv itself could also be an option but I think there is value in its simplicity--it just doesn't work well enough for our use case.

[tarsius]

I have just pushed the display-buffer branch. It moves away from lv and instead uses display-buffer and an option that specifies the action argument for that function, like magit-popup did.

That also restores the ability to scroll the popup window again.

Isearch and selecting a suffix using the arrow keys or the mouse is still not possible again. Not sure if I want to bring that back.

Please give it a try.

[tarsius]

See #18 for how to use q instead of C-g to quit transients.

[tarsius]

Except for isearch not being available anymore everything has been taken care of.

I intend to look into what it would entail to bring back isearch eventually. Meanwhile I close this issue because to many other things were discussed above for it to be a good place to talk about isearch.

[ahungry]

I liked the old magit-popup behavior better.

If I had a split frame with one window on the left, and a magit buffer on the right, pressing P for the magit-push functionality would open up a magit-popup just in the right window, leaving the layout of left window alone.

With transient overtaking the echo area (and a huge amount of vertical space) it pushes everything else scrunched up (both the window on the left side, which used to be unaffected, as well as the main magit buffer).

Overall, a very jarring experience. Is there anyway to customize transient to be more horizontal and less vertical? It's only using about 1/10th of the horizontal space, and eating up 1/3rd of my vertical real estate.

[MatthewFluet]

@ahungry Alternate displays of the popup were added at 7e45a57. See the first FAQ entry and customize the transient-display-buffer-action to '(display-buffer-below-selected) to get the magit-popup behavior.

[ahungry]

@MatthewFluet thank you so much!

[tonycpsu]

Having a similar issue here. I'd like the "old" magit popup behavior, and I've customized transient-display-buffer-action to (display-buffer-below-selected). What happens is definitely not what I was expecting.

Previous behavior was: I'd run magit-status, which would open in the right side of my window split. I'd then type a key (e.g. "F") and the popup would show up below, in the same frame. What happens now is this:

The worst part is when I hit ctrl-g to abort, it un-splits my window.

magit 20190404, transient 20190319

[tarsius]

I cannot reproduce this. My guess is that you have installed some other package that controls how buffers are being displayed. Try to reproduce the issue using magit-emacs-Q-command.

[tonycpsu]

OK, it works as intended with emacs -Q so it's got to be another package. Thanks.

[davep-github]

Since transient mode can be suspended, would it be possible to keep the bindings in the transient buffer and allow the default to be set to suspended vs activated? I understand your "like a key sequence" rationale. IMO, the biggest issue is simply that it's not the way I (and others?) are used to Emacs behaving. ibuffer, dired, custom and even the completion buffer allow complete "Emacs'ing." The only case I can think of is your key sequence model and some other minibuffer commands. Disabling transient to do some things and enabling it for others is kind of clumsy (for me at least), and currently my habits result in a `ding' as I try to do thinks. Getting help via man, and the isearch issue point out a potential maintenance issue, which is the possibility of having to add features in order to provide functionality that came for free with popups. I like to visit a notes file for things like commit messages, or for remembering which changes are split over multiple files, without searching all the diffs. At this time, all of this is relatively trivial to me and would only be worth it if it only took a minimal amount of effort on your part. These are just some thoughts. But many thanks. It's very helpful and educational to a newbie like me.

[tarsius]

Getting help via man, and the isearch issue point out a potential maintenance issue, which is the possibility of having to add features in order to provide functionality that came for free with popups.

As always there are trade-offs. The isearch issue is a problem ot transient, indeed. But its the only one that has come to my attention so far that wasn't quickly addressed.

Showing documentation is essentially the same in magit-popup and transient; an explicit implementation is needed in both cases. However the transient implementation is the one that gets some things "for free" in this case, not the magit-popup implementation. The difference is not terribly important, but since you used this example to make the opposite point it is worth pointing out.

The modal nature of the transients is a design decision, which I knew would cause questions but which I firmly believe in.

So I think the opposite is true in general. transient is more complex than magit-popup but the additional features are of great value. That might not be obvious to users, but when you implement transients/popups, then it should quickly become clear that much more though went into the former. transient is the result of my having had to deal with the limitations of magit-popup for years.

It's also worth noting that magit-popup also was a rewrite of an earlier implementation. The two re-implementation efforts were vastly different though. For magit-popup the major goals were (1) let users customize existing popups a feature that was in very high demand but extremely limited and difficult with the previous implementation (2) avoid other users even noticing that a new implementation is in use. For transient the major goals were to (1) make it possible to implement a large number of features that just weren't feasible with the previous implementation (2) use the appropriate emacs features instead of re-implement good-enough, easy to understand, but limited versions thereof (*) and (3) make the necessary changes even if that means that the mental images that users have formed and/or some workflows are broken. There simply was no way forward with magit-popup. Sticking to that implementation would have meant limiting ourselves to the existing feature set. All of the very few additional features added over time were very difficult to implement and resulted in hard-coded special-cases and/or lots of code in caller that needed functionality not provided by the abstraction, which should instead have been generalized.

(*) E.g. use the command-loop, instead of the previous primitive read-char-based implementation. The command-loop is very powerful and as a result complex. Because the command-loop is so powerful and flexible, this does mean that some things have to be handled explicitly. Never-the-less it once more it is transient that get things for free by using the proper abstraction and magit-popup that has to implement features that it could have gotten for free.

I strongly believe in "accomplish more by not implementing unnecessary features". Unfortunately that is not always possible with packages as popular as Magit; there are to many users having an opinion on what feature is necessary. But that's okay, Magit's success is also in part due to my willingness to implement features I do not firmly believe in. (In some cases I stick to that opinion, in others I come around once I got a change to play with that feature because I took the time to implement it.) What I am trying to say is that transient on the other hand does definitely not fall into the feature creep category. Its features exist for a reason, if something is complex, then that is because I have to deal with a complex interface and/or because the feature itself is non-trivial when taking all the use-cases into account.

Except for the isearch thing the "features" that came for free in magit-popup but are "missing" from transient are actually bugs waiting to happen. Unconditionally allowing users to change external state while a transient/popup is active gives them the freedom to put Emacs in a state the active transient/popup cannot deal with.

For example the "change the log arguments in the current log or status buffer" popup/transient cannot be used in non-log/status buffers, but if the user is free to change the buffer after the popup/transient is activated, then that by-passes the "is this a valid buffer" check that the transient prefix commands runs before actually activating the transient state. When users are allowed to run arbitrary commands, then they can change what buffer is current and if they then invokes a suffix command that expects that the current buffer is a valid buffer, then all kinds of things can happen.

---

I firmly believe that transients as implemented by transient are and should be an inherently modal interface. In fact I believe that the are best thought of as a visual and much more powerful synthesis of Emacs' key sequence and prefix argument features.

I can understand that people who have only used magit-popup as a user without also dealing with the implementations of a large number of popups have formed a different mental model. Especially when considering that magit-popup wasn't actually fully modal.

But magit-popup not being fully modal actually was a feature that fell out of the implementation. One might say it was a bug even. magit-popup had the arbitrary limitation that suffix bindings could only be one event long. Pressing a key sequence that is longer than one event and is bound in a foreign keymap was not actively prevented and by-passed the code responsible for checking if there is a suffix binding, resulting in the foreign command being invoked.

Users noticed and made use of that "feature". I didn't remove it because I knew that would break the workflow of some users. But when it came to implement magit-popup's successor I decided to do what I felt was right, and not preserve this bug that had become a feature to some.

---

Since transient mode can be suspended, would it be possible to keep the bindings in the transient buffer and allow the default to be set to suspended vs activated?

I cannot parse that sentence, but I think you are asking "Would it be possible to add an option that would allow me to opt-in to being able to invoke any command while a transient is active?".

To which the answer is: I believe so, it would involve setting the transient-non-suffix slot to transient--do-call for every transient. But I would prefer to not hand you that footgun.

The purpose of the transient-non-suffix is to allow the user to invoke external commands for specific transients. Here is an example taken from https://magit.vc/manual/transient/Comparison-With-Other-Packages.html:

(define-transient-command outline-navigate ()
  :transient-suffix     'transient--do-stay
  :transient-non-suffix 'transient--do-warn
  [("p" "next visible heading" outline-previous-visible-heading)
   ("n" "next visible heading" outline-next-visible-heading)])

For such a transient it makes sense to allow other commands also.

[tarsius]

Arrow key navigation, mouse support, and isearch support have been restored. You need to set an option to enable these features. See #42.

[alezost]

In case someone still prefers the old behavior of magit-popup, I came up with some pretty heavy customizations for transient to make it a normal buffer (i.e., non-transient keys are not blocked, you can switch to other buffers, etc.).

Note: setting transient--buffer-name is important as the default name starts with a space, and such buffers are treated as "special" by Emacs. This variable was introduced by commit 4ef8229feb8a12c9198de2c61593903099c9611a.

I have been using transient only for several days, so there may be some problems that I have not noticed yet, but the following config works for me so far:

(with-eval-after-load 'transient
  (setq
   transient--buffer-name "*transient*"
   ;; transient-detect-key-conflicts t
   ;; transient-highlight-mismatched-keys t
   ;; transient--debug t
   transient-enable-popup-navigation t
   transient-mode-line-format mode-line-format
   transient-display-buffer-action '(display-buffer-below-selected))

  (let ((map transient-base-map))
    (define-key map (kbd "C-g") 'transient-quit-all)
    (define-key map (kbd "C-q") 'transient-quit-one)
    (define-key map (kbd "DEL") 'transient-quit-one))

  (define-key transient-map (kbd "C-h") nil)

  (let ((map transient-popup-navigation-map))
    (define-key map (kbd "<tab>") 'transient-forward-button)
    (define-key map (kbd "<backtab>") 'transient-backward-button ))

  (transient-suffix-put 'transient-common-commands
                        "C-g" :command 'transient-quit-all)
  (transient-suffix-put 'transient-common-commands
                        "C-q" :command 'transient-quit-one)

  (defun al/transient-fix-window ()
    "Return `transient--window' to a 'normal' state."
    (set-window-dedicated-p transient--window nil)
    (set-window-parameter transient--window 'no-other-window nil)
    (with-selected-window transient--window
      (setq
       window-size-fixed nil
       cursor-in-non-selected-windows t
       cursor-type (default-value 'cursor-type)
       mode-line-buffer-identification
       (list ""
             (symbol-name (oref transient--prefix command))
             " " (default-value 'mode-line-buffer-identification)))))

  (define-derived-mode al/transient-mode special-mode "al/transient"
    (setq buffer-read-only nil)
    (al/transient-fix-window))

  (defun al/transient-push-keymap (map)
    (with-demoted-errors "al/transient-push-keymap: %S"
      (internal-push-keymap (symbol-value map) 'al/transient-mode-map)))

  (defun al/transient-pop-keymap (map)
    (with-demoted-errors "al/transient-pop-keymap: %S"
      (internal-pop-keymap (symbol-value map) 'al/transient-mode-map)))

  (defun al/transient-fix-show (&rest _)
    (transient--debug 'al/transient-fix-show)
    (al/transient-fix-window)
    (select-window transient--window))

  (defun al/transient-fix-init (&rest _)
    (transient--debug 'al/transient-fix-init)
    (with-current-buffer transient--buffer-name
      (al/transient-mode)))

  (defun al/transient-fix-pre/post-command (fun &rest args)
    (transient--debug 'al/transient-fix-pre/post-command)
    ;; Do anything only for transient commands.
    (when (or (get this-command 'transient--prefix)
              (string-match-p "\\`transient"
                              (symbol-name this-command))
              (and transient--transient-map
                   (string= (buffer-name) transient--buffer-name)
                   (lookup-key transient--transient-map
                               (this-single-command-raw-keys))))
      (apply fun args)))

  (defun al/transient-fix-delete-window (fun &rest args)
    (unless (eq transient--exitp 'suspend)
      (apply fun args)))

  (advice-add 'transient--minibuffer-setup :override #'ignore)
  (advice-add 'transient--minibuffer-exit :override #'ignore)
  (advice-add 'transient--push-keymap :override #'al/transient-push-keymap)
  (advice-add 'transient--pop-keymap :override #'al/transient-pop-keymap)
  (advice-add 'transient--pre-command :around #'al/transient-fix-pre/post-command)
  (advice-add 'transient--post-command :around #'al/transient-fix-pre/post-command)
  (advice-add 'transient--show :after #'al/transient-fix-show)
  (advice-add 'transient--init-transient :after #'al/transient-fix-init)
  (advice-add 'transient--delete-window :around #'al/transient-fix-delete-window))