Generalize embark-mark and embark-unmark

[minad]

Very much related to #166, feel free to merge these feature requests.

• Allow candidates to be collected one by one using a special embark-select action, add them to a collect buffer
• Add the possibility to act on all candidates at once in a collect buffer or a subset of the candidates (#166)

This gives us convenient actions on multiple candidates.

[oantolin]

Got it: I'll put whitespace-cleanup-region on F (for formatting, but capital because it is less important than fill-region which is on f). I can move mark to C-SPC and put this new select action on SPC. How does that sound?

[minad]

Any thoughts on a key binding for deselecting all? Maybe the commands could be merged? With a prefix argument it deselects all? That would save us the trouble of picking a key binding.

Got it: I'll put whitespace-cleanup-region on F (for formatting, but capital because it is less important than fill-region which is on f). I can move mark to C-SPC and put this new select action on SPC. How does that sound?

Yes, both ideas sound good.

[oantolin]

Mmh. If I select some consult-line candidates and collect them RET in the collect buffer doesn't quite work on them, but embark-act-dwim does work. I'm getting something transformer-related slightly wrong somewhere.

[minad]

Mmh. If I select some consult-line candidates and collect them RET in the collect buffer doesn't quite work on them, but embark-act-dwim does work. I'm getting something transformer-related slightly wrong somewhere.

RET works when I do a direct embark-collect from consult-line. Maybe check if the two different ways of creating a collect buffer (via selection and via direct collect) result in different buffer content? I hope there is no larger design issue somewhere. So far the change is pretty smooth I think.

[oantolin]

They do: the difference is that direct collection results in consult-location targets, and doing it through selection results in multi-category targets. :) I'm thinking about how best to handle this difference.

[oantolin]

That's a small bug right there: if all multi-category candidates refine to the same type, that's what you are supposed to get when you collect.

[minad]

They do: the difference is that direct collection results in consult-location targets, and doing it through selection results in multi-category targets. :) I'm thinking about how best to handle this difference.

Oh, I wonder about this one. I had thought that refinement handles multi category candidates of the same category? This is what also what I meant in https://github.com/oantolin/embark/commit/c5a916732053db0c70a5151e0b5dd03f4b5c4978#r109136014 btw. Maybe my excitement was a bit premature. ;)

[oantolin]

It is supposed to work that way. And indeed export, which relies on this seems to be working. I'm not sure what's going on yet.

[oantolin]

Oh, that's silly: I think export tries for a common refinement of the type and collect just doesn't. If I'm right, this is a very old oversight then!

[oantolin]

Oh no, it's more subtle than that: collect does look for a common refinement, but it keeps the original type and original candidates exactly in case the default action has an override. This selection business is a new case: here the default action doesn't want the original types and targets, but the refined ones! That had never happened before.

[minad]

Okay, then you probably have to rewrite the embark-selected-candidates collector. For completion UIs do not use multi-category but instead the original minibuffer category. Only use multi-category for normal buffer candidates.

[oantolin]

OK. I think everything is done now with respect to the issues noticed so far. I wrote a bit of documentation for the manual too. I'll test it for a little while before merging.

On the side of internals, I think it might be nice to adopt @minad's suggestion of changing the calling convention for non-interactive actions (but, @minad also suggested, still supporting the current convention with a warning).

EDIT: spoke too soon: I haven't added visual feedback for icomplete. :)

[minad]

Great! It would be good to see if we can cleanup the internals a little more before merging. But these are minor issues. I just think it is always a good opportunity to do some cleanup when merging a new or generalized feature, instead of merging additional problematic code.

1. The calling convention for non-interactive actions.
2. The multi-category refinement commented in https://github.com/oantolin/embark/commit/0ad1b4069e965c6a25359bf1ff9111ce3e2389f3.

[oantolin]

I think I've now correctly fixed the multi-category refinement issue. I just store original targets and types instead of transformed ones, and if all original types in a selection match, then I manually refine multi-category to that original type, which then allows the usual refinement to take place. The realization that covering all cases correctly requires two rounds of refinement, convinced me to do one round manually in embark-selected-candidates. I also don't blindly assume all candidates selected from, say, a collect buffer have the same type, since you may want to select other targets in a collect buffer that aren't part of that buffer's collection. Like maybe you have a collection of elisp libraries, but you want to select the underlying files instead.

[oantolin]

About the calling convention for non-interactive actions: we do reuse some other Emacs functions in Embark's default configuration that I had forgotten about: bookmark-show-annotation, bookmark-edit-annotation, elp-restore-function, untrace-function. In my personal configuration I also reuse pocket-lib-add-urls.

[minad]

About the calling convention for non-interactive actions: we do reuse some other Emacs functions in Embark's default configuration...

We could still consider supporting both calling convention, but at this point it may not be simpler anymore.

Some more comments:

1. I see an embark-collect-toggle-selection function, which can be removed.
2. There is also embark-deselect-all mentioned in embark--act.
3. embark--selection lacks a docstring.
4. There is a leftover comment marks t m u U in the embark-collect-map.
5. Should embark--toggle-select push down to unread-command-events such that we move down during marking as we did before? Or will this be too brittle or unexpected?
6. Why is embark--toggle-select an around-action hook? Couldn't it be a simpler pre-action hook?
7. I still wonder about the "leaking" of text properties for the selected candidates. As far as I understood we preserve text properties for non-interactive function actions, right? Do we leak the text properties of selected (and then acted upon) candidates this way?

[oantolin]

Thanks for pointing out the little things to clean up!

6. Why is embark--toggle-select an around-action hook? Couldn't it be a simpler pre-action hook?

Yes, it could. It doesn't actually matter whether it is pre-, post- or around- and the code of the function doesn't change (you'll notice it doesn't do anything with the :run parameter). I think I picked around- just because picking either pre- or post- would be asymmetric. 😄

I'm not too keen on 5, it feels unexpected to me. I definitely wouldn't like that behavior in regular buffers! Maybe in collect buffers it's almost OK, but then it should be something that moves you to the next candidate even if it is multiline. Probably if it is limited to the minibuffer it would be OK; but then people could just use a keyboard macro C-. SPC <down> to mark & move.

Finally 7 is a very good question that I need to think more about.

[hmelman]

whitespace-cleanup-region seems like a pretty rare action. Maybe it should be put on an entirely different binding.

M-SPC seems like a reasonable fit.

[hmelman]

Any thoughts on a key binding for deselecting all? Maybe the commands could be merged? With a prefix argument it deselects all? That would save us the trouble of picking a key binding.

I'll admit to not completely following everything going on here (which is fine), but as I said, I'd expect unmark-all to be on U. That's what helm does, also dired, and also magit (kinda, it's Unstage all). A prefix arg (on what binding are we talking?), isn't necessarily bad, and it's probably the second place I'd think of that functionality (I feel like there are similar things in Emacs but can't think of an example off-hand).

[oantolin]

The currently implemented proposal, @hmelman, is (using C-. to stand for whatever you choose to bind embark-act to) that C-. SPC toggles whether a target is selected or not, and C-. C-u SPC deselects all targets.

[oantolin]

Maybe in the minibuffer, collect buffers and the *Completions* buffer it would also make sense to have a way to select all or, better yet, to toggle all. (This would not be reasonable for regular buffers!)

[Elilif]

@oantolin It seems that the change of embark-selected-candidates in 55db7435ebc4d399034a59b5f591085a546d5f26 does not pass the embark--selection text property, which will cause embark--selected-target-bounds in embark-act-all to return nil. That will cause unexpected behaviors in some actions such as delete-region.

[oantolin]

Oh, wow, yeah, good catch, @Elilif.

[oantolin]

Should be fixed now.

[Elilif]

@oantolin Should we prepend embark-selected to existing ones in minibuffer? As shown in the following screenshot, I have marked two commands, but because the priority of embark-selected is low, it is difficult to distinguish whether they have been marked (only -all was highlighted with embark-selected).

[minad]

@oantolin Overall this looks pretty good. Also the added complexity is low since we could get rid of embark-collect-mark/unmark. :)

Yes, it could. It doesn't actually matter whether it is pre-, post- or around- and the code of the function doesn't change (you'll notice it doesn't do anything with the :run parameter). I think I picked around- just because picking either pre- or post- would be asymmetric. smile

When I saw the around hook I was looking for the :run. Therefore it may be less confusing if you use either pre or post.

Finally 7 is a very good question that I need to think more about.

Regarding the leaking of text properties - it seems to me that the advantages of using text properties are mostly gone due to the changes to the refinement code. It may be better if we make embark--selections just a plain data structure. This way we can guarantee that no such leaking can happen. In particular the overlays stored as text properties are problematic since they result in a memory leak and also make the resulting strings unserializable, e.g., if you store them in a history variable.

[oantolin]

@Elilif Is this maybe a theming issue? This what it looks like for me with both embark-act and embark-act-all selected, and with the Vertico selection being embark-act.

[oantolin]

it seems to me that the advantages of using text properties are mostly gone due to the changes to the refinement code.

I wish, @minad, but the reason to attach them as text properties remains: it's because candidate collectors are expected to a return a cons of a candidate type and a list of strings. Before selections that was sufficient, because candidate collectors were mostly used for embark-export and embark-collect which don't need information on target bounds. But now, if we want to allow editing actions on selections in regular buffers, embark-act-all needs to know the bounds of each target, and this information needs to be included in the return value of embark-selected-candidates somehow. The only two options are (1) text properties, (2) changing the candidate collector protocol to allow richer return values.

I'm not too keen on changing protocols, though maybe that's a good idea here. Another way to address this is that embark-act-all could still use the overlay text property to extract the bounds but then call the action on a copy of the string without the text property.

[hmelman]

The currently implemented proposal, @hmelman, is (using C-. to stand for whatever you choose to bind embark-act to) that C-. SPC toggles whether a target is selected or not, and C-. C-u SPC deselects all targets.

Maybe in the minibuffer, collect buffers and the Completions buffer it would also make sense to have a way to select all or, better yet, to toggle all. (This would not be reasonable for regular buffers!)

Sounds reasonable. Maybe C-. C-u C-u SPC toggles all? Make a single C-u do whatever is more common and C-u C-u the less common function. I think it's in collect buffers I'd expect single letter bindings for these, so that it's more dired-like. My installed version of embark (in embark-collect-mode-map) has these on m (mark), u (unmark), U (unmark-all), is that staying?

[oantolin]

@hmelman I was thinking about C-. C-u C-u SPC to toggle all, we do need some way of doing that. In the current code the bindings for m, u, t and U in collect buffers are just gone. I need to decide whether to just change the interface of collect buffers here or reimplement the old one on top of the new selection functionality.

[hmelman]

I was thinking about C-. C-u C-u SPC to toggle all, we do need some way of doing that.

I think this would be fine.

In the current code the bindings for m, u, t and U in collect buffers are just gone. I need to decide whether to just change the interface of collect buffers here or reimplement the old one on top of the new selection functionality.

Is it just SPC to mark and args for the other functions? The consistency is probably good. It would be nice to be able to enable the old (dired-like) bindings, if not in the package, then at least with some hints in the wiki. I think I'd have to play with it to know if I wanted it or not; I think it would depend on what other emacs things these various interfaces feel like.

[oantolin]

Ooh, putting code on the wiki to enable the old interface is a good idea if we decide to change it, @hmelman. Thanks, that hadn't occurred to me!

There is currently no way to do what t used to do, toggle the selection status of each item in the collect buffer. There is currently a way to select all if the selection is empty: C-. A SPC; when the selection is empty, embark-act-all acts on all candidates in the collect buffer (or in the minibuffer), and then this toggles the selection status of each one, selecting them all.

The ability to deselect all is a little redundant: if you know the selection is non-empty, then C-. A SPC will act on the selection, toggling the selection status of each item and therefore deselecting all of them.

Maybe I could even remove C-. C-u SPC and just tell people to do C-. A SPC instead. It's a neat sort of toggle too: as we've seen, it will select everything if nothing is selected and deselected everything if anything is selected.

[minad]

@oantolin

Maybe I could even remove C-. C-u SPC and just tell people to do C-. A SPC instead. It's a neat sort of toggle too: as we've seen, it will select everything if nothing is selected and deselected everything if anything is selected.

This sounds like a great idea. Let's do this to keep things simpler and to avoid the redundancy. Since we are adding an entirely new interface, it makes a lot of sense to keep it as small as possible.

I'm not too keen on changing protocols, though maybe that's a good idea here. Another way to address this is that embark-act-all could stile use the overlay text property to extract the bounds but then call the action on a copy of the string without the text property.

Do I understand correctly that the only missing information is the bounds of the collected candidates? Then changing the protocol seems justified, also since target finders return bounds. The collectors could optionally return bounds too, which for now would be only relevant for the selection collector. Stripping the information before acting seems like an acceptable solution too, but it is more ad-hoc and is probably more bug prone in the long run.

[oantolin]

I was pretty happy in the minibuffer with just embark-act-all before because I almost always can match exactly the targets I want, but sometimes I did want something like all targets matching some pattern except for one. When that happened, I would use Orderless's negative literals to exclude the one I didn't want, but now I can type the pattern, select all with C-. A SPC, then move the Vertico selection to the one I want to exclude and toggle it. I think this new approach might be better than using Orderless to exclude the one I don't want, because often to make sure I wasn't accidentally excluding more I would type ! and almost the whole name of the exception.

[oantolin]

I removed the prefix argument case of embark-toggle-select. I think I probably will introduce a new case of the calling convention for candidate collectors. Here's what I was thinking:

Target finders can return (cons type target-string) or (cons type (cons target-string bounds)) (where bounds is (cons start end), with either numbers or markers). Currently candidate collectors must return (cons type list-of-target-strings) so maybe we can keep that case and add a case (cons type list-of-targets-with-bounds), where each element of list-of-targets-with-bounds is of the form (cons target-string bounds).

Another option would be switch everything over to plists, which I've been wanting to do anyway for a while, but it seemed like just gratuitous incompatibility to make that change alone.

[minad]

...and add a case (cons type list-of-targets-with-bounds), where each element of list-of-targets-with-bounds is of the form (cons target-string bounds).

This sounds good enough. I don't really see a reason to switch over to a more complex format.

[oantolin]

OK! I think this is done! I'll wait a bit to give time to test a little more thoroughly just in case.

Now candidate collectors are allowed to return bounds information for their candidates. I updated the existing candidate collectors to do so, and this allowed addressing @minad's concern about leaked text properties: the actions now only receive the bounds, not the overlay in a text property.

[minad]

@oantolin I just gave the current selections branch a quick test. It works well on my side! Maybe it would be time to add a changelog given the addition of this nice feature and the removal of the marking function. You can start with an empty one like I did here. ;)

[oantolin]

I added a changelog and merged the selections branch.

[minad]

@oantolin Great, thank you! Maybe a bit late, but I have one small small bikeshed - shall we rename embark-toggle-select to embark-select? Generally Embark uses concise names for its main entry points (act, collect, export, ...) and I would argue embark-select is a pretty fundamental action. embark-toggle-select could may be confused with an operation which does some toggling of the set of selected candidates. Also talking about "toggling the selection state" is kind of less clear than just saying "select".

[bdarcus]

I just finally managed to try it, @oantolin; nice work!

One little thing:

When I select an item using this, it deactivates embark-act, and I have to reactivate.

That's not intentional, is it?

If not, is there somewhere I should look in my config?

[oantolin]

It is intentional. You can change this behavior with (add-to-list 'embark-repeat-actions #'embark-toggle-select).

But why do you want to keep embark-act open? After selecting an item you want to immediately run a different action on the same item?

[bdarcus]

I want to select multiple items, and run the same action on them. Isn't that the idea for the feature?

But I can only select them after having activated embark-act.

[oantolin]

Yes, that is the idea. The way you do it is, assuming you bind embark-act to C-., is to type C-. SPC on each item you want to select, and then use embark-act-all which is C-. A to run some action on all of them.

If that's too many C-., you can bind a keyboard macro to C-. SPC. You can can also bind the command embark-act-all to a single key, if you wish.

[bdarcus]

OIC; thanks.

And this should work with embark-multitarget-actions? It doesn't seem to at first glance.

[bdarcus]

Nevermind.

On Thu, Apr 20, 2023, 11:12 AM Omar Antolín Camarena < @.***> wrote:

Yes, that is the idea. The way you do it is, assuming you bind embark-act to C-. is to type C-. SPC on each item you want to select, and then use embark-act-all which is C-. A to run some action on all of them.

If that's too many C-., you can bind a keyboard macro to C-. SPC. You can can also bind the command embark-act-all to a single key, if you wish.

— Reply to this email directly, view it on GitHub https://github.com/oantolin/embark/issues/253#issuecomment-1516508794, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAAI3XDY2UPFMPIXPY4323XCFG4TANCNFSM477HVCNA . You are receiving this because you were mentioned.Message ID: @.***>

[oantolin]

embark-act-all lets you call two types of actions on a selection of items: actions that work on a single item at a time, and actions that receive a list of all the items in one go. To tell embark-act-all it is supposed to pass a list of all items at once to the action, you add the action to embark-multitarget-actions. The vast majority of actions work on a single item at a time.

[oantolin]

Ah, I saw your "nevermind" a little too late.

[bdarcus]

I forgot to do embark-act-all :-)

I do wonder if in time that might go away?

If one selects multiple, then are you not, by definition, wanting to act on all?

[minad]

@bdarcus

I forgot to do embark-act-all :-) I do wonder if in time that might go away? If one selects multiple, then are you not, by definition, wanting to act on all?

Acting on a single candidate is different than acting on multiple candidates. I would prefer if this distinction is kept. If you want an act dwim style command you could write:

(defun embark-act-on-single-or-all ()
  (interactive)
  (if embark--selection (embark-act-all) (embark-act)))

EDIT: However this creates problems with the selection action itself, since as soon as you selected an item you probably don't want to unselect it again right away. You should then at least bind the selection action and embark-act-on-single-or-all on different keys :)