Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generalize embark-mark and embark-unmark #253

Closed
minad opened this issue Jul 7, 2021 · 120 comments
Closed

Generalize embark-mark and embark-unmark #253

minad opened this issue Jul 7, 2021 · 120 comments
Assignees

Comments

@minad
Copy link
Contributor

minad commented Jul 7, 2021

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

This gives us convenient actions on multiple candidates.

@minad minad changed the title Add embark-select/collect-one which allows to select candidates one by one Multi actions, add embark-select/collect-one which allows to select candidates one by one Jul 16, 2021
@minad
Copy link
Contributor Author

minad commented Jul 16, 2021

@jdtsmith
Copy link

jdtsmith commented Jul 22, 2021

A related even simpler but still powerful idea: allow mini-buffer embark to act on all the displayed candidates (instead of the first or single selected). I find my self often selecting buffers using consult+orderless (10x faster than any other way), then exporting to an ibuffer, then selecting all (t), then killing (D) or running multi-occur (O) on them. An embark "act on all" would save the side trip to ibuffer (where I will be selecting all 99% of the time anyway).

@minad
Copy link
Contributor Author

minad commented Jul 22, 2021

Yes, indeed. My idea is to have two features:

  1. A collection function which allows to collect candidates one by one and results in a collect buffer
  2. An act function to act on everything in the current context (collect buffer, minibuffer)

Then one can only use (2) to act on a candidate subset obtained by plain filtering using orderless. But one can also do one by one manual collection (1) if that is desired.

@oantolin
Copy link
Owner

Since this functionality isn't implemented I can't be sure, but I feel like the ability to run the same action on all current candidates would probably be enough for me. I'm not sure I'd miss the ability to collect candidates haphazardly.

@minad
Copy link
Contributor Author

minad commented Jul 22, 2021

I'm not sure I'd miss the ability to collect candidates haphazardly.

I have another idea - it would be slightly UI specific: I could add some command to Vertico/Selectrum which allows you to select candidates. And then I write a separate Embark collector which collects just the selected candidates and which will be registered right before the all-candidates-collector. Then we need commands embark-act-all and embark-dwim-all, which should act on all candidates returned from the candidates collector. Problem solved?

@bdarcus
Copy link

bdarcus commented Jul 28, 2021

Not sure where best to put this, but maybe here.

FYI, I had a long back-and-forth in the past couple of weeks trying to get the author of this project to use completing-read everywhere.

https://github.com/jkitchin/org-ref-cite

I mostly succeeded, save for one piece. He explains why he stayed with ivy as:

Ivy is required for this, as it allows single, multiple, and sequential actions on the selected candidates.

@minad
Copy link
Contributor Author

minad commented Jul 28, 2021

It seems this package is mostly decoupled? Is there a problem to write multiple frontends, one for ivy, one for helm one only completing-read+embark? Also it seems to be that there is overlap with your bibtex-actions package.

@bdarcus
Copy link

bdarcus commented Jul 28, 2021

It seems this package is mostly decoupled?

Definitely.

Is there a problem to write multiple frontends, one for ivy, one for helm one only completing-read+embark?

Not at all. I just thought it could be valuable to hear what an ivy-focused developer values there.

Also it seems to be that there is overlap with your bibtex-actions package.

Yeah, they're different ways to use the same bibtex-completion backend.

But that's cool. It is all modular, so we can easily mix-and-match components. I, for example, can use his hydra with my CR front-end, and he can in theory use my embark-act at-point integration with his ivy front-end.

@bdarcus
Copy link

bdarcus commented Aug 21, 2021

I'm not sure I'd miss the ability to collect candidates haphazardly.

I have another idea - it would be slightly UI specific: I could add some command to Vertico/Selectrum which allows you to select candidates. And then I write a separate Embark collector which collects just the selected candidates and which will be registered right before the all-candidates-collector. Then we need commands embark-act-all and embark-dwim-all, which should act on all candidates returned from the candidates collector. Problem solved?

Coming back to this "another idea", WDYT?

How it would interact with ideas previously discussed in #166 WRT to embark-collect? You use the "collect" verb here, but I can't tell if you mean that generically, or if this a way to populate the collect buffer?

@bdarcus
Copy link

bdarcus commented Sep 21, 2021

Since this functionality isn't implemented I can't be sure, but I feel like the ability to run the same action on all current candidates would probably be enough for me. I'm not sure I'd miss the ability to collect candidates haphazardly.

Perhaps this would be the correct first step, and if we find this hypothesis is off, can always add the more fine-grained control later?

I was thinking about this feature again when thinking about using consult--multi to select and open multiple files of different categories.

@oantolin
Copy link
Owner

I added an embark-act-all command (91e6db4) to act on all current candidates (its mostly useful in the minibuffer, but I guess I could see using it for something in dired buffers, for example: you could mark some files and then use embark-act-all to run consult-file-externally on all of them). Please test!

@minad
Copy link
Contributor Author

minad commented Dec 10, 2021

This is great! Thank you. I will give it a test.

@minad minad closed this as completed Dec 10, 2021
@oantolin
Copy link
Owner

oantolin commented Dec 12, 2021

Apparently I only ever used ibuffer when I want to kill a bunch of similarly named buffers: from consult-buffer I'd export the buffers I wanted to kill to ibuffer, toggle the selection, kill them all. Now with embark-act-all it feels like I may never use ibuffer again. 😛

Similarly, I do most of my file management tasks with file actions from find-file, and it seems that about half of the remaining uses of dired were to delete a bunch of similarly named files. I guess I may eventually get to the point where I only use dired for wdired-mode, which actually seems perfectly reasonable.

@Elilif
Copy link

Elilif commented Apr 10, 2023

I have another idea - it would be slightly UI specific: I could add some command to Vertico/Selectrum which allows you to select candidates. And then I write a separate Embark collector which collects just the selected candidates and which will be registered right before the all-candidates-collector. Then we need commands embark-act-all and embark-dwim-all, which should act on all candidates returned from the candidates collector. Problem solved?

@minad @bdarcus @oantolin
I recently did something similar, but I didn’t use embark-collect because implementing similar functionality with it was a bit cumbersome.

Here is my code:

(defvar eli/vertico-marked-list nil
  "List of marked candidates in minibuffer.")

(defun eli/vertico-mark ()
  "Mark candidates in minibuffer"
  (interactive)
  (let*
	  ((selected (embark--vertico-selected))
	   (target (cdr selected)))
	(if (member target eli/vertico-marked-list)
		(setq eli/vertico-marked-list (delete target eli/vertico-marked-list))
	  (push target eli/vertico-marked-list))))

(defun eli/vertico-marked-p (candidate)
  "Return t if CANDIDATE is in `eli/vertico-marked-list'."
  (member (concat vertico--base candidate) eli/vertico-marked-list))

(defun eli/vertico--format-candidate-hl-marked (args)
  "Highlight marked vertico items."
  (let* ((cand (car args)))
	(if (eli/vertico-marked-p cand)
		(add-face-text-property 0 (length cand) 'embark-collect-marked nil cand)
	  (vertico--remove-face 0 (length cand) 'embark-collect-marked cand))
	args))

(advice-add #'vertico--format-candidate :filter-args #'eli/vertico--format-candidate-hl-marked)

(defun eli/vertico-marked-list-clean ()
  "Initialize `eli/vertico-marked-list' and `eli/vertico-mark-type'."
  (setq eli/vertico-marked-list nil))

(add-hook 'minibuffer-setup-hook #'eli/vertico-marked-list-clean)

(defun eli/embark-vertico-marked-list ()
  (when eli/vertico-marked-list
    (cons (car (embark--vertico-selected)) (reverse eli/vertico-marked-list))))

(add-hook 'embark-candidate-collectors #'eli/embark-vertico-marked-list -100)
(setq embark-confirm-act-all nil)
(keymap-set vertico-map "C-SPC" #'eli/vertico-mark)
(keymap-set vertico-map "C-," #'embark-act-all)

Now you can use eli/vertico-mark in minibuffer to mark candidates, then call embark-act-all to do something.

For instance:

  1. insert candidates

  2. kill selected buffers

I think it is easier to operate than embark-collect, and more flexible than embark-act-all.

@oantolin
Copy link
Owner

oantolin commented Apr 10, 2023

That looks quite nice, @Elilif! Maybe it's worth making a tiny package that depends on Embark and on Vertico for this? Or at least documenting it here on the Embark wiki.

I haven't tried your code yet, but if I'm reading it correctly, you can continue typing in the minibuffer to narrow further and this does not forget which candidates of the wider list were marked; that's great!

The one thing I don't like about this type of interface is that it is pretty hard to see all marked candidates at once, although I guess Embark can help with that: just insert them somewhere. If I understand the code correctly, they stay marked so you can act on them again.

@bdarcus
Copy link

bdarcus commented Apr 10, 2023

The one thing I don't like about this type of interface is that it is pretty hard to see all marked candidates at once ...

I guess this is related to previous discussion that resulted in the now defunct consult-completing-read-multiple; that ideally there'd be some sort of grouping of the marked candidates?

@minad
Copy link
Contributor Author

minad commented Apr 10, 2023

The snippet made by @Elilif would be quite a simple addition. Unfortunately it breaks down as soon as you want "more", like showing all the marked candidates and also interacting nicely with filtering. But maybe remembering the candidates is just good enough, since you can also show them by collecting them? Anyway, these limitations are the reason why we didn't proceed with this feature in the past.

If we want this, we would also need additional integration code for other UIs. One possibility would be to generalize embark-collect-mark and embark-collect-unmark to arbitrary buffers. The mark/unmark commands would obtain the current candidate via the embark-target-finders hook and add them so some buffer-local (?) list. Marking and unmarking themselves would just become simple actions themselves. The marked list can then be returned by a hook added to embark-candidate-collectors. Furthermore we'd need to hook into UIs like Vertico to show the marked candidates (vertico--format-candidate). I see some elegance in making marking/unmarking plain actions. Maybe @oantolin agrees and we could try this out.

@Elilif
Copy link

Elilif commented Apr 11, 2023

But maybe remembering the candidates is just good enough, since you can also show them by collecting them?

Yeah, my intention is to have a method that could be between embark-collect and embark-act-all, because when I only need to select a smaller subset of candidates (such as killing some buffers or opening some files), the operation of embark-collect is too complex, while embark-act-all cannot meet my needs.

@oantolin
Copy link
Owner

@minad: That does sound worth trying.

@Elilif: I haven't actually missed the ability to mark candidates in the minibuffer, and I'm not completely sure why I haven't. I think the main reason is that if I want to do the same thing to a bunch of candidates there is almost always a clear pattern to their names: I want to kill all vc buffers and their names start with *vc; or I want to kill all buffers whose names start with * except *scratch*; or I want to delete all files whose name is foo with any extension; or to delete all files with extension .log; etc.

So the vast majority of the time embark-act-all is good enough for me. If there are one or two exceptions to a pattern I just exclude them using orderless-without-literal. I don't seem to get myself into situations where I want to run the same action on some weird patternless collection of candidates; I don't actually use embark-collect or even dired and ibuffer that often, because embark-act-all seems to be good enough 95% of the time. Why do you want this ability to mark candidates in the minibuffer? I do think it is a good idea to have it, even if I don't personally need it, but I am curious to know why people want it.

@minad
Copy link
Contributor Author

minad commented Apr 12, 2023

@oantolin

I don't seem to get myself into situations where I want to run the same action on some weird patternless collection of candidates;

From my experience this mostly happens with killing buffers, deleting files etc, while not having to think about patterns. Oh I want to delete this one, mark, this one, mark, oh and that one over there too, down down down mark.

If we can make this work with reasonable effort, I am in favor of trying this. For me the convincing reason is that it is a generalization of the existing Embark collect marking infrastructure. I can try to implement it for default completion, Embark collect and Vertico. But no guarantee that it will work out nicely. I'd like to avoid adding serious complications to the integration code if possible, since this is a recipe for unstability in the long term.

@oantolin
Copy link
Owner

I too am in favor of trying this, I was just trying to see more of the appeal. For me, if there is a clear pattern to the candidate I want to act on, I reach for embark-act-all; if not, if I'm going to have to scroll down to each candidate anyway, I just run the action on each one. If it's really complicated, I collect or export. I do like the idea of marking in the minibuffer (maybe even if it only has visual feedback on Vertico!), but I don't see a need for it, and was trying to get a feeling for why other people want this feature.

Marking, unmarking and making embark-act-all work is easy. The only tricky part is getting some visual feedback on what is currenty marked. That requires UI-specific code. For Vertico I think advising vertico--format-candidate is the best option. Maybe for default completion similarly advising completion--insert-strings is a good option. It would be nice to unify the approach to visual feedback in various UIs, but I don't see how to do that yet.

@minad
Copy link
Contributor Author

minad commented Apr 12, 2023

I do like the idea of marking in the minibuffer (maybe even if it only has visual feedback on Vertico!), but I don't see a need for it, and was trying to get a feeling for why other people want this feature.

I think one additional motiviation is that completion is restarted for some commands when acting. This means the method of scrolling and acting on each candidate individually is inconvenient. For example Consult narrowing is lost, the selected candidate is lost, etc. I've also recently learned that Helm aborts completion altogether after acting (at least by default), which avoids the kind of restarting ugliness in the first place.

For Vertico I think advising vertico--format-candidate is the best option.

For Vertico one can use a cl-defmethod, which is a cleaner approach than an advice.

Maybe for default completion similarly advising completion--insert-strings is a good option.

No, for default completion, Embark collect and all other normal buffers, Embark should simply install an overlay over the target. This is why I think the generalization is elegant. The target finders should return boundaries in these cases, right? We could simply demand that the target finders must return boundaries for this to work. Only Vertico, Ivy, Icomplete should require special casing.

@bdarcus
Copy link

bdarcus commented Apr 12, 2023

I think one additional motiviation is that completion is restarted for some commands when acting.

Yes, I was wondering about this example.

emacs-citar/citar#714

Edit: The original mastodon thread, with some discussion of the limitation of the embark-collect workaround.

https://mastodon.social/@gmoretti/109460454739376441

Though in that case we're using our custom multi-select UI.

@minad
Copy link
Contributor Author

minad commented Apr 12, 2023

@bdarcus embark--restart preserves the input, but not most of the other minibuffer state.

@minad minad mentioned this issue Apr 12, 2023
Closed
@bdarcus
Copy link

bdarcus commented Apr 20, 2023

OIC; thanks.

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

@bdarcus
Copy link

bdarcus commented Apr 20, 2023 via email

@oantolin
Copy link
Owner

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
Copy link
Owner

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

@bdarcus
Copy link

bdarcus commented Apr 20, 2023 via email

@minad
Copy link
Contributor Author

minad commented Apr 20, 2023

@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 :)

@oantolin
Copy link
Owner

oantolin commented Apr 20, 2023

I was going to post exactly the same function that @minad did!

I have lots of Embark things in my muscle memory now, like C-. DEL to delete an entire s-expression. If suddenly C-. started acting on the selection that would certainly screw me up, since I sometimes do a little editing between selecting targets.

@bdarcus
Copy link

bdarcus commented Apr 20, 2023

I want to emphasize it was a question, and I'm not set in my opinion.

But that seems incoherent at first glance. If you select multiple, you want to act on multiple.

I was surprised after selecting multiple, per my earlier question, that it only acted on one.

Obviously you'll have to see what others users says, but just wanted to put the idea out there, based on my initial experience.

@oantolin
Copy link
Owner

oantolin commented Apr 20, 2023

The way I see it, if I select multiple candidates I eventually want to act on them, but not necessarily right now. For example, yesterday I was editing some notes and wanted to add references for some claims I made, but I was also doing other editing, like rewording, reordering some paragraphs, etc., anf I didn't want to stop to look up the references. So instead I selected the sentences for which I needed to add a reference and was free to use embark anyway for editing (I use it a lot to reorder paragraphs and sentences for example, or to delete entire parenthetical remarks from anywhere inside the remark). Then when I finished editing I ran embark-collect to get a buffer with all the sentences needing a reference, and only then started looking them up.

More philosophically, if embark-act forced you to act on the selection it would become a modal interface: once you select anything you are now officially in selection mode and loose the ability to decide what to act on until you leave selection mode. I don't like modal interfaces and while I will happily provide building for others to build their own modal prisons, I wouldn't make that the default in Embark.

I find even the modal minibuffer a little constraining and thus recommend enabling recursive minibuffers.

@oantolin oantolin reopened this Apr 20, 2023
@oantolin oantolin self-assigned this Apr 20, 2023
@oantolin
Copy link
Owner

oantolin commented Apr 20, 2023

Plus that function that @minad wrote is short and sweet and solves your problem. We can add it to the wiki and even to the manual, but I I'd prefer to keep embark-act and embark-act-all separate.

@oantolin
Copy link
Owner

Oh, right @minad, with your embark-act-single-or-all you need a separate binding to select items. That's easily solved, but definitely worth pointing out.

@minad
Copy link
Contributor Author

minad commented Apr 20, 2023

@bdarcus

But that seems incoherent at first glance. If you select multiple, you want to act on multiple.

FWIW I don't want that. To me the Embark design seems coherent as is. But I understand that your proposed workflow is useful and maybe preferrable. I would bind some key to a macro which directly selects and another key to such an embark-act dwim style command. For my taste this is not as nice as the current design, since one needs another key, the selection action is singled out as a separate action. Furthermore I usually want extra protection for embark-act-all.

@oantolin What about the proposed renaming in #253 (comment)? I saw that you already bumped the version.

@minad
Copy link
Contributor Author

minad commented Apr 20, 2023

There were are few more things which came to my mind recently, but I forgot them over the discussion. I wonder if we can be sufficiently sure that embark--selections is valid. For example what happens if we select candidates in Vertico on an action which removes a candidate but isn't marked as embark--restart. Then we probably act on an invalid candidate, right? This is probably expected and solved via embark--restart. But what about candidates within a buffer - what if the user deletes or edits a marked overlay? Do we need some protection here? Did I understand correctly that embark--selections is always preserved after acting? Is this convenient and expected in normal buffers or would we need a post action hook which cleans the selection?

@oantolin
Copy link
Owner

Oh, no! I hadn't seen (or maybe I did and forgot) the proposal to rename embark-toggle-select to embark-select! I really like that idea (even though the name would be a little inaccurate since it doesn't select, it toggles!). Would it be awful if I changed it now?

@oantolin
Copy link
Owner

The question of the selection becoming invalid is tough. You are correct, @minad, that currently embark--selections is always preserved. I have taken advantage of that in fact. To move stuff around in a buffer you can delete-region and embark-insert, and that works even if you delete first. Right now it is the user's responsibility to understand that even if the highlighted stuff changes, the selection is still made up of the original candidates. 😬

It would be easy to implement an alternative model in which:

  • candidates that have no bound are preserved,
  • candidates that do have bounds are "autoupdated" to always match whatever is in the overlay, but their type stays as whatever it was when you selected them,
  • candidates with bounds that are deleted (i.e., for which the start and end eventually coincide) are automatically deselected.

Would that be better?

@minad
Copy link
Contributor Author

minad commented Apr 20, 2023

@oantolin

Oh, no! I hadn't seen (or maybe I did and forgot) the proposal to rename embark-toggle-select to embark-select! I really like that idea (even though the name would be a little inaccurate since it doesn't select, it toggles!). Would it be awful if I changed it now?

You could change it now, update it everywhere including the changelog and then tag version 0.23. This has also happened to me, that I tagged multiple versions in quick succession because a mistake got in first. I usually avoid tagging a new stable version directly after making any kind of significant change. I usually wait a few days or a week. Many users will pull your package anyway from MELPA or ELPA-devel. Then incoming feedback can be addressed before the release.

@minad
Copy link
Contributor Author

minad commented Apr 20, 2023

It would be easy to implement an alternative model in which:

I think we should first gain some experience with the current model and maybe only then make changes afterwards. If the decision is made to use another model I would try something simpler - candidates without bounds are always preserved and candidates with bounds are automatically deselected when they are edited. One can use simple overlay modification hooks for that, such that this would be a trivial change. See https://github.com/minad/jinx/blob/2f7f4ac9ce3f69a0d086e2600406c36f46ff3a07/jinx.el#L266-L268.

@minad
Copy link
Contributor Author

minad commented Apr 20, 2023

My main concern about the current model is that one somehow loses track of the selected candidates. But in order to tell this, I have to gain more experience with the workflow. But anyway, one can always easily inspect embark--selections.

@oantolin
Copy link
Owner

I usually avoid tagging a new stable version directly after making any kind of significant change. I usually wait a few days or a week.

I should definitely start doing that. I have done that most of the time, in fact. Here I jumped the gun and it also happened to me once before.

Do you describe the changes in the changelog even before bumping the version number? That sounds like a good idea for early adopters.

@oantolin
Copy link
Owner

I agree with using the current system for a while before changing the model.

@oantolin
Copy link
Owner

candidates with bounds are always preserved and candidates without bounds are automatically deselected when they are edited

You have those two swapped, right?

@minad
Copy link
Contributor Author

minad commented Apr 20, 2023

You have those two swapped, right?

Right, edited.

@minad
Copy link
Contributor Author

minad commented Apr 20, 2023

I should definitely start doing that. I have done that most of the time, in fact. Here I jumped the gun and it also happened to me once before.

Yes, this has happened before. But it is usual that it takes some time to develop a robust release workflow. I am regularly tagging new releases for my packages if new features got in, but almost never right away. Then I usually inspect the last release in order to ensure that I don't forget anything, basically git show --stat <tag>.

Do you describe the changes in the changelog even before bumping the version number? That sounds like a good idea for early adopters.

Yes, see for example https://github.com/minad/vertico/blob/main/CHANGELOG.org.

@minad
Copy link
Contributor Author

minad commented Apr 20, 2023

There are also many different "philosophies" about how a changelog should look like. I only describe user-facing changes or very relevant technical changes. I also try to keep the changelog concise and usually don't reference the issue tracker. It should be somewhat readable.

There are also more formal changelogs which always reference the issues (https://github.com/radian-software/selectrum/blob/master/CHANGELOG.md), but for me this hurts readability. It may be better in the long run if you want to investigate something. But there is still always the git log for that. For me the worst example are the Emacs "changelogs" which are just extracted from the git log, e.g., https://github.com/emacs-mirror/emacs/blob/master/lisp/ChangeLog.16. Totally useless. The NEWS files on the other hand are very useful, but are often lacking information.

@oantolin
Copy link
Owner

I think I agree with what you put into the changelog. Maybe except in being very brief always. For a major new feature, I'd like to not send people to the manual.

I did the renaming and bumped the version again. 😬

@minad
Copy link
Contributor Author

minad commented Apr 20, 2023

I think I agree with what you put into the changelog. Maybe except in being very brief always. For a major new feature, I'd like to not send people to the manual.

Makes sense. Your changelog entry looks very good. The only danger is that you will end up with a massive log after a few releases, which takes a lot of time to read. I am still finding my best style for changelogs. :)

I did the renaming and bumped the version again.

Great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants