oantolin / embark

Emacs Mini-Buffer Actions Rooted in Keymaps
GNU General Public License v3.0
925 stars 56 forks source link

Fix duplicate menus after update #301

Closed rudolf-adamkovic closed 3 years ago

rudolf-adamkovic commented 3 years ago

I updated Embark and now see duplicate menus on embark-act:

Screen Shot 2021-07-28 at 17 15 23

My configuration:

(package-install 'embark)
(global-set-key (kbd "C-.") 'embark-act)
(setq-default embark-prompter 'embark-completing-read-prompter)

As for embark-prompter, I followed README.md:

If you find you prefer entering actions that way, you can configure embark to always prompt you for actions by setting the variable embark-prompter to embark-completing-read-prompter.

oantolin commented 3 years ago

The duplication is because you have all three of these things in your configuration:

You can fix the repetition changing any one of these three things. Since you are unlikely to want to stop using Vertico, and since you chose the completing-read prompter consciously, you'll probably want to change the first item: (setq embark-indicator 'embark-minimal-indicator).

bdarcus commented 3 years ago

Since you are unlikely to want to stop using Vertico, and since you chose the completing-read prompter consciously, you'll probably want to change the first item:

Or he could remove that line to try the new "verbose" option?

oantolin commented 3 years ago

True, as @bdarcus says, you might prefer instead of narrowing the list by typing and maybe scrolling around the action list, you might prefer to try using the key bindings directly, which can be faster. For that, as @bdarcus says, just keep the default value for embark-indicator and for embark-prompter.

rudolf-adamkovic commented 3 years ago

@oantolin

Thank you so much for the detailed explanation; it gave me actual understanding of the problem. I did set embark-indicator to 'embark-minimal-indicator, and everything now works as well.

@bdarcus @oantolin

All right, I have tried the new UI. And... it is wonky. I use a relatively small (13") screen and thus cannot always see all the options. So, the first thing I did was to move the scrollbar. That crashed with:

Error running timer: (error "scroll-bar-toolkit-scroll must be bound to an event with parameters")

Then I tried C-v and C-n to scroll, but that did not work either. As a mere end-user, the UI reminds me of Magit, which looks great but breaks a lot of Emacs standards (scrolling, describe key, ...)

oantolin commented 3 years ago

Yeah, the indicator is (currently) purely decorative: if you are using the embark-keymap-prompter, you can read the embark-verbose-indicator, it but not interact with it in any way, not even scroll it. There have already been requests to allow scrolling, and that certainly seems like a good idea.

rudolf-adamkovic commented 3 years ago

@oantolin Thank you so much for your help!

oantolin commented 3 years ago

The scroll-bar still doesn't work to scroll the verbose indicator, but you can now scroll with scroll-other-window and scroll-other-window-down.

oantolin commented 3 years ago

And now you can scroll with the scroll-bar (whose existence you reminded me of when you complained about it not working 😛) or with the mouse wheel.

rudolf-adamkovic commented 3 years ago

@oantolin

Testing, testing.

Great! That said, the new user interface still lags in usability behind the old-school Emacs ways or Vertico. Here are three problems I ran into during my re-testing:

Error running timer: (error "mouse-drag-region must be bound to an event with parameters")
mouse-minibuffer-check: Wrong type argument: window-valid-p, #<window 23>
Shadowed targets at point:  (nil to cycle)
oantolin commented 3 years ago

Testing, testing.

Thanks for testing!

  • We can now scroll down with C-M-v, but there is no ergonomic way to scroll the other way, as the default shortcut isM-<prior>.

The command scroll-other-window-down, besides being bound to M-<prior> is also bound to C-M-S-v, though I understand if you don't consider three holding down three modifiers ergononic. :) Maybe ESC <prior> is better?

Of course, one could remap the key, but still.

If you don't find M-<prior> or C-M-S-v ergonomic in Embark you probably don't find them ergonomic in Emacs in general, so maybe remapping would be useful to you beyond just the verbose indicator, for help windows in general.

Ideally, C-n, C-p, C-v, and M-v would work.

I would find it counterintuitive if any of those worked! In vanilla Emacs all those commands affect the current window, which is the window selected when you ran embark-act. The verbose indicator is the so-called "other window", which is why you scroll it with scroll-other-window and scroll-other-window-down.

Think of this as poppping up a help window with describe-function, say. The window you were in keeps focus, and C-n, C-p, C-v, and M-v scroll it. To scroll the help window you'd use C-M-v or C-M-S-v.

Pressing any of those makes everything disappears abruptly, both Vertico and Embark.

Yes, because your C-n, C-p, C-v, or M-v is taken to be the action you selected. The verbose indicator is closed and the action you specified is carried out in whatever window you called embark-act from.

It is an Embark feature I hold dear to my heart that all the action keymaps are merely for convenience: Embark can run any command at all as an action, and you can use all your normal keybindings (except those shadowed by the relevant embark action keymap) or even M-x to choose an action. There are an extremely limited number of exceptions to this rule: for example prefix arguments (C-u, C-5, etc.) are taken to apply to the action instead of taken to be the action, and now scroll-other-window and scroll-other-window-down are exceptions too.

  • When attempting to select some text, everything disappears abruptly, both Vertico and Embark, and the following message appears:

Yes, I did not attempt to make selecting text from the verbose indicator possible. Is this a feature you'd like? It seems pointless since the next thing you do would be taken to be the action and then would be carried out in the window you were when you ran embark-act, so you wouldn't really be able to do anything with the selected text.

embark-act is really just a glorified prefix key. You wouldn't really expect to be able to type a prefix key, then select some text with the mouse and do something with it, and then go back to finishing the key binding whose prefix you already typed.

  • The top of the window suggests typing nil:
Shadowed targets at point:  (nil to cycle)

@minad kindly fixed this already.

minad commented 3 years ago

I don't think we should add the possibility to select text in the popup. It is only a transient indicator window, which is not meant to be interacted with. If one wants to interact with the keybindings use the completing-read prompter and then export to a collect buffer.

oantolin commented 3 years ago

My feelings exactly, @minad.

minad commented 3 years ago

In case one really wants to lock down the buffer one can also ignore all mouse clicks:

https://github.com/minad/corfu/blob/fbcd636f76de102068865e303e8c795d218af3a5/corfu.el#L272-L278

rudolf-adamkovic commented 3 years ago

@oantolin

Thank you so much for taking time to explain everything to me so well. I now understand the concept of "other window" and why C-n would not make sense in this context. I will try the existing Emacs keybindings and if they are too hard to type, I will remap them.

@oantolin @minad

Personally, I dislike the modern user interfaces that do not allow search, select, copy, and the like. For instance, I needed to copy "Shadowed targets at point: (nil to cycle)" for this issue, and I ended up typing it manually, which felt very unlike Emacs, where I rarely waste time re-typing a message that is in front of me.

P.S. Recently some people asked on emacs-devel why more of Emacs does not use transient.el and one person replied:

On the other hand something has always felt off about transient, in the sense that it is breaking some expected behaviour or couldn't pin-point yet, but just unconsciously stumble over. [1]

This is exactly how I feel about the "new and modern" interfaces in Emacs. Magit has a similar feel to it, and I can never be sure if the program will allow me to select text in the various parts of its user interface. Such uncertainty sucks, and it makes the program worse for power users, in my opinion. I would expect to be forced to re-type a message that is in front of me in Apple or Microsoft applications, because they "know better." But in Emacs?

(Or perhaps this problem is a lack of knowledge on my part?)

[1] https://yhetil.org/emacs-devel/d01fe148-1843-de5f-ebda-ee49486a4fae@gmail.com/T/#mc43d5ece9138015c2930d73354bd7ce54fabd2dc

oantolin commented 3 years ago

For instance, I needed to copy "Shadowed targets at point: (nil to cycle)" for this issue, and I ended up typing it manually, which felt very unlike Emacs, where I rarely waste time re-typing a message that is in front of me.

@salutis That is very unfortunate, and I'd be annoyed too! I was thinking the other day about not killing the *Embark Actions* buffer, just burying it. Since its name starts with space, it would stay out of people's way but be available to switch to after running an action, just in case. (What do you think about this @minad?)

minad commented 3 years ago

Personally, I dislike the modern user interfaces that do not allow search, select, copy, and the like. For instance, I needed to copy "Shadowed targets at point: (nil to cycle)" for this issue, and I ended up typing it manually, which felt very unlike Emacs, where I rarely waste time re-typing a message that is in front of me.

I do understand this sentiment. I feel very much the same. But for people like us there is the minimal indicator and the completing-read prompter. The verbose indicator is meant for beginners and giving an easier introduction etc, since the status of Embark is exposed.

minad commented 3 years ago

P.S. Recently some people asked on emacs-devel why more of Emacs does not use transient.el and one person replied: On the other hand something has always felt off about transient, in the sense that it is breaking some expected behaviour or couldn't pin-point yet, but just unconsciously stumble over. [1]

I have recently been thinking about transient too and I was wondering why it has been accepted into Emacs core so quickly. I like that Embark is only a very thin layer on top of keymaps, while transients are a heavier mechanism which focuses mainly on the UI aspect, doing away with the conceptual simplicity of keymaps. There is already the menu-item mechanism in keymaps and now we even got transient, to build key driven UIs. On the other hand the Magit UI is polished and many users like this, so it is understandable. We will see how things develop regarding transient adoption in Emacs itself.

rudolf-adamkovic commented 3 years ago

@oantolin

I have just noticed that the transient Export dialog in the Org Mode (that I use all the time on a small screen) supports scrolling with "normal keys," but Magit transient dialogs behave like Embark does. But then, I also noticed that the aforementioned Export dialog becomes the "active window," based on the mode line color. What you said makes more and more sense. Also, what a mess for the end-user!

minad commented 3 years ago

@oantolin

Since its name starts with space, it would stay out of people's way but be available to switch to after running an action, just in case. (What do you think about this @minad?)

Usually I prefer if no weird buffers stay around, even if I understand what @salutis says here. Power users can also look into the code ;) It seems better to make it clear that there exist alternatives. And the verbose indicator can stay what it is, a transient indicator, which leaves no remnants around.

On the other I think Helm also doesn't kill its buffers, maybe it doesn't even hide them with a space? At least I've read complaints that Helm buffers would get into the way. So if we do this and don't add a space we keep the buffer around for inspection, but annoy people. If we kill it we annoy users who want to inspect it. But keeping the buffer around with the space-prefixed name, it seems we get the worst from both worlds.

If you want to cater for all needs we could make the buffer name customizable and the buffer kill behavior customizable (bury/kill).

minad commented 3 years ago

@salutis I must say this - please don't call this a mess. There are lot of people with different opinions and Emacs somehow tries to help them all. This also explains why there are so many alternatives around for similar problems. In any case, you can create a better alternative.

oantolin commented 3 years ago

@minad Is it really the worst of both worlds keeping the buffer around with an initial space? I think buffers whose name starts with space are very unobtrusive. For example, I never minded that Selectrum creates one, and didn't even know about it until Clemens told me (to shut me up since I was going on and on about the advantages of having completions displayed in a regular buffer 😛).

I just checked and right now I have 8 buffers with a leading space:

 *Minibuf-1*             *Minibuf-0*             *Echo Area 0*
 *Echo Area 1*           *code-conversion-work*  *code-converting-work*
 *VC-Git* tmp status                             

If I hadn't specifically used consult-buffer to check for hidden buffers I would have had no idea at all that I had so many!

oantolin commented 3 years ago

But also, I like knowing the buffer is killed, even if burying it would be almost as good. So I'll keep things as they are.

minad commented 3 years ago

@oantolin

it really the worst of both worlds keeping the buffer around with an initial space? I think buffers whose name starts with space are very unobtrusive.

And useless in this case. Selectrum doesn't create a buffer anymore btw. I removed this. Also Vertico doesn't create a buffer.

(EDIT: Correction, Selectrum still creates a buffer if a display action is used. vertico-buffer does the same of course.)

But also, I like knowing the buffer is killed, even if burying it would be almost as good. So I'll keep things as they are.

Exactly. It should get killed.

oantolin commented 3 years ago

Selectrum doesn't create a buffer anymore btw.

Great! I can go on and on about the advantages of putting completions into a real buffer again. 😉 Clemens might revert your commit just to shut me up.

rudolf-adamkovic commented 3 years ago

@minad

I apologize. To clarify, I did not mean any particular piece of the puzzle but the situation as a whole, as in "the evolution of user interfaces is [sometimes necessarily] messy."

rudolf-adamkovic commented 3 years ago

You folks know approximately million times more about Emacs than me. Is there no function similar to what embark-collect-snapshot does but for the entire frame? That would solve it for all "non-traditional" user interfaces.

minad commented 3 years ago

@salutis Thanks. It is understandable. But I think Emacs is just not this one paradigm, it has so many different ones. And this weird nature with many wheels is also kind of beautiful ;)

Is there no function similar to what embark-collect-snapshot does but for the entire frame? That would solve it for all "non-traditional" user interfaces.

This seems to be out of scope for Embark ~Emacs~. But in principle you could write a function iterating over all buffers and cloning them somehow to make them accessible. Problem is only that modern UIs like transient will prevent you from invoking this command :grimacing:

(You could still hack around of this of course.)

iyefrat commented 3 years ago

re: the whole transient thing, I only really use them in Magit, but I think they provide something invaluable there that can't really be done with keymaps, namely, allowing you to customize commands in an ad hoc fashion (all the - options). In my opinion this is a significant part of why Magit is so great. Can't really speak with regards to whether or not it was added too soon to Emacs core though.

oantolin commented 3 years ago

It's not so much that it can't be done with keymaps, @iyefrat, but rather that doing it with keymaps (unless you also pop up a help buffer, at which point you are kind of reimplementing transient) is nowhere near as discoverable. For example, isearch has tons of options you can tweak while actively using it, but you pretty much need to read the (long!) isearch section of the manual to know about them.

rudolf-adamkovic commented 3 years ago

@minad

Thanks. It is understandable. But I think Emacs is just not this one paradigm, it has so many different ones. And this weird nature with many wheels is also kind of beautiful ;)

Interesting. I need to better internalize this perspective. Thank you so much for sharing! After your comment, I am starting to see Emacs more like a Linux distribution (with all its packages) than the Linux kernel (with its modules).

This seems to be out of scope for Emacs. [...]

I think you meant Embark, because it surely must be in scope for Emacs. Ha-ha! If this bothers me again, for the third time, I shall bring it up on emacs-devel.

rudolf-adamkovic commented 3 years ago

@iyefrat

allowing you to customize commands in an ad hoc fashion (all the - options)

Interesting. Light-bulb moment! You are right, that is a great interaction model. (That said, it does not preclude selection, copying, and search.)

minad commented 3 years ago

@salutis

Interesting. I need to better internalize this perspective. Thank you so much for sharing! After your comment, I am starting to see Emacs more like a Linux distribution (with all its packages) than the Linux kernel (with its modules).

Yes, a Linux distribution seems like a better comparison (But the Linux kernel is also quite chaotic - many overlapping subsystems, rewrites etc).

However if a package gets accepted into Emacs itself, in particular if it is a library like transient, it can potentially affect many parts of the whole system. You cannot untangle it anymore. Usually Emacs introduces customization options to avoid complaints by angry users who dislike the new modern UI. Looking forward to the option isearch-use-transient... ;)

(I meant Embark.)

iyefrat commented 3 years ago

It's not so much that it can't be done with keymaps, @iyefrat, but rather that doing it with keymaps (unless you also pop up a help buffer, at which point you are kind of reimplementing transient) is nowhere near as discoverable.

@oantolin yeah fair point. I wonder why building transient on top of keymaps like that wasn't they way tarsuis went about it. It's probably too late to change that now even if it he'd want to.

rudolf-adamkovic commented 3 years ago

@minad Perhaps as time goes on and Emacs adopts transient.el in a large number of its workflows, the "standard" transient interface will get proper selection, copy, search, etc.

rudolf-adamkovic commented 3 years ago

So, I updated Emacs (from master) and packages (via package.el) and the duplicate menus are back. Is this me?

Here is my configuration:

(setq-default embark-indicator 'embark-minimal-indicator)
(setq-default embark-prompter 'embark-completing-read-prompter)
Screen Shot 2021-08-09 at 00 48 15
oantolin commented 3 years ago

Yeah, sorry, the indicator system changed again. You can have several indicators active at once now, and so the variable is called embark-indicators in plural. You'll probably want to keep it at almost the new default value, just replacing embark-mixed-indicator with embark-minimal-indicator (keeping the other two elements).

oantolin commented 3 years ago

So this:

(setq-default embark-indicators
              '(embark-minimal-indicator
                embark-highlight-indicator
                embark-isearch-highlight-indicator))

The first element you already know, the second handles higlighting the target at point when you act outside the minibuffer. That functionality was previously special-cased but is now just another indicator. The last element only does something for symbol targets and what it does is isearch-style highlighting of the other ocurrences of the symbol in the buffer.

rudolf-adamkovic commented 3 years ago

@oantolin

That works. Thank you so much!

I plan to try and reevaluate Embark's defaults soon. I briefly tried them today (by commenting out my configuration), and I noticed a short delay (perhaps 300-500 milliseconds) before presenting the default (mixed) indicator. The minimal indicator (with Vertico) appears immediately. I wonder, is the delay a bug or a feature?

minad commented 3 years ago

@salutis Take a look at the customizable variables.

rudolf-adamkovic commented 3 years ago

@minad Ah, embark-mixed-indicator-delay. All is clear now! Thank you again.

oantolin commented 3 years ago

Also, if you prefer the buffer listing actions to pop-up immediately it's probably more sensible to use the verbose indicator rather than the mixed indicator with a delay of 0.

rudolf-adamkovic commented 3 years ago

@oantolin

Also, if you prefer the buffer listing actions to pop-up immediately it's probably more sensible to use the verbose indicator rather than the mixed indicator with a delay of 0.

You answered my question before I asked. 😄 I changed Embark to use the verbose indicator and removed the custom delay of zero. Now everything pops up immediately and with no delays whatsoever. Just how I like it. Thank you so much for caring!

We can now scroll down with C-M-v, but there is no ergonomic way to scroll the other way, as the default shortcut isM-<prior>.

Correction: I have just discovered that adding the shift key (C-M-S-v) scrolls the other way. This is exactly how I expected it to work. I withdraw my objection!

rudolf-adamkovic commented 3 years ago

For the future reader (or archeologist), this is what I ended up with:

Screen Shot 2021-08-11 at 11 43 15