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

Allow to mark, and act on, multiple candidates in collect buffer #166

Closed
bdarcus opened this issue Feb 23, 2021 · 31 comments
Closed

Allow to mark, and act on, multiple candidates in collect buffer #166

bdarcus opened this issue Feb 23, 2021 · 31 comments

Comments

@bdarcus
Copy link

bdarcus commented Feb 23, 2021

The conversation here has me wondering:

Would it be possible, and wise, to allow one to mark, and act on, multiple candidates in a collect buffer, as in helm?

Use case:

I want to open multiple files, so I:

  1. narrow the candidates
  2. run embark-collect-snapshot
  3. mark the three I want, using some keybinding
  4. hit return for the default action (open the files)

PS - I'm new to embark, so recognize I may be missing some nuance.

@oantolin
Copy link
Owner

oantolin commented Feb 23, 2021

To be clear: we are only talking about applying the same single-argument action to each one of several candidates, right? We aren't talking about a new kind of actions that receives multiple candidates at once?

I have considered a feature to mark candidates and apply the same action to each one, and am of two minds about it.

If the only way to mark candidates is one at a time, like, you move to a candidate and press a key to mark it, then move to another, etc., then it's probably not worth it, since it hardly saves any keystrokes: you could just activate embark-collect-direct-action-minor-mode-map, which binds the actions directly without needing to call embark-act first, and go down the list of candidates running the actions.

If I added more ways to mark candidates, say, mark all, marking all that match a regexp, toggle marked/unmarked, etc. I'd be reimplementing politza's great tablist package. So I'd be inclined to instead make a small embark-tablist package, that depends on both embark and tablist, and provides an embark-tablist-act-on-marked command.

By the way, even without the embark-tablist package existing yet, I think it's already worth using tablist with embark collect buffers.

@oantolin
Copy link
Owner

Note that for example Ivy doesn't (or rather, didn't back when I used it) have a way to mark a bunch of items first and act on them later. Instead, it just has a way to set an action that will take a single keystroke to execute, and then you march down the list and run it on the items you want.

@minad
Copy link
Contributor

minad commented Feb 23, 2021

You mentioned this tablist package multiple times already. How is it useful for embark snapshots? Do you enable some tablist-mode by default in your collect buffers? I need a simple example to understand what you use it for right now, since this embark-act-on-marked command does not yet exist :)

But it seems that such an embark-act-on-marked would satisfy the use case by @bdarcus perfectly.

@oantolin
Copy link
Owner

Without the embark-act-on-marked commands, it's mostly useful to focus Embark collect buffers to a subset of lines. Of course we now also have consult-focus-lines. But even with consult-focus-lines, the tablist package might still offer some advantage: it has commands to mark by regexp, to mark a single line, to mark all, to unmark all, and to toggle marked/unmarked (just like dired). You can easily remove the lines that are marked and restore them later.

@oantolin
Copy link
Owner

oantolin commented Feb 23, 2021

One very minor advantage of tablist over something like consult-focus-lines is that with tablist you can easily search just the candidates or just the annotations. (I don't consider this an important advantage.)

Another potential use of tablist, is that you can define your own predicates and then use them to mark lines. By default there are predicates to (1) check if a column matches a regexp or (2) is literally equal to some value. But you could add a predicate to check modification times for when the candidates are files, etc. A bunch of these predicates could be added to the hypothetical embark-tablist package.

@oantolin
Copy link
Owner

I think personally I don't need tablist with collect (thought I do love using tablist with pdf-tools annotations). I usually manage to narrow to exactly the candidates I want before I even run embark-collect-snapshot. And once I have a collect buffer I hardly every focus on subset of lines, I'm more likely to just use isearch (or the avy-embark-collect functions) to move to a specific candidate.

@minad
Copy link
Contributor

minad commented Feb 23, 2021

Sounds goods. Thank you for the explanation. I think such an embark-tablist package would be a great addition, but I would still consider it as an advanced feature which is probably rarely needed.

@bdarcus
Copy link
Author

bdarcus commented Feb 23, 2021

To be clear: we are only talking about applying the same single-argument action to each one of several candidates, right? We aren't talking about a new kind of actions that receives multiple candidates at once?

That was my thought.

I have considered a feature to mark candidates and apply the same action to each one, and am of two minds about it.

If the only way to mark candidates is one at a time, like, you move to a candidate and press a key to mark it, then move to another, etc., then it's probably not worth it, since it hardly saves any keystrokes: you could just activate embark-collect-direct-action-minor-mode-map, which binds the actions directly without needing to call embark-act first, and go down the list of candidates running the actions.

Ah, that's one of those "nuances" I had missed. Agreed.

If I added more ways to mark candidates, say, mark all, marking all that match a regexp, toggle marked/unmarked, etc. I'd be reimplementing politza's great tablist package. So I'd be inclined to instead make a small embark-tablist package, that depends on both embark and tablist, and provides an embark-tablist-act-on-marked command.

By the way, even without the embark-tablist package existing yet, I think it's already worth using tablist with embark collect buffers.

I'll take a look; thanks. Perhaps you can include an example config with this on the wiki at some point?

@oantolin
Copy link
Owner

One small problem with trying to use tablist for this is that it would only work in list view. That's my fault: to be honest, tabulated-list-mode is really meant to used in a row-oriented fashion, and embark collect grid view is an abuse; a row in embark's grid view is meaningless, whereas in tabulated-list-mode or tablist, some things sort of assume that each row is a meaninful entity. In particular, in tablist you don't mark cells, you mark entire rows.

@oantolin
Copy link
Owner

oantolin commented Feb 23, 2021

@bdarcus

Ah, that's one of those "nuances" I had missed. Agreed.

So for now, let's just run actions individually, possibly using embark-collect-direct-action-mode to save a keystroke for embark-act. But we can keep thinking whether some sort of marking and batch execution makes sense. I think that actually embark-collect-direct-action-mode is a little more flexible than "mark & act", because you can use single keypresses to run different actions as you go along. It's probably good enough for the "run single action on each of several candidates" problem.

But it's still worth thinking about "mark & act", specially if there turns out to interest in "multi-argument actions": a single action that receives a list of candidates. I think Helm does have those.

I'll take a look; thanks. Perhaps you can include an example config with this on the wiki at some point?

Just install tablist, run M-x tablist-mode in an embark collect buffer, if you want to play around with it. If you like it and want it automatically on, add tablist-mode to embark-collect-mode-hook.

@minad
Copy link
Contributor

minad commented Feb 23, 2021

I think mark and act is also useful if you want to mentally separate the steps. Like mark all, check again and delete.

@oantolin
Copy link
Owner

I think mark and act is also useful if you want to mentally separate the steps. Like mark all, check again and delete.

True.

@bdarcus
Copy link
Author

bdarcus commented Feb 24, 2021

possibly using embark-collect-direct-action-mode

Am I missing something, or is this not documented ATM?

I guess if using this option, one saves a keystroke, but gives up a which-key menu?

@bdarcus
Copy link
Author

bdarcus commented Feb 26, 2021

I was digging a bit into how helm-bibtex deals with this, here's a description:

https://github.com/tmalsburg/helm-bibtex#apply-actions-to-multiple-entries

... and a screenshot (light green rows are marked, darker green is the selected item):

Screenshot from 2021-02-25 21-39-22

BTW, do either of you know whether selectrum allows highlighting the matched text like this?

@bdarcus
Copy link
Author

bdarcus commented Feb 26, 2021

I just realized that I already had tablist-mode installed. I guess doom includes it by default.

The marking functionality is indeed quite nice!

But how do I actually run the actions after marking? Is that not currently possible?

@oantolin
Copy link
Owner

oantolin commented Feb 26, 2021

@bdarcus

possibly using embark-collect-direct-action-mode

Am I missing something, or is this not documented ATM?

You're right: it's not mentioned in the readme! I thought it was.

I guess if using this option, one saves a keystroke, but gives up a which-key menu?

Yes. Though if you want a reminder you can still run embark-act even with embark-collect-direct-action-mode on.

BTW, do either of you know whether selectrum allows highlighting the matched text like this?

The matched text? You mean "India" in that example? Selectrum highlights it by default. So I think maybe I'm misunderstanding the question.

But how do I actually run the actions after marking? Is that not currently possible?

It's not implemented yet.

@bdarcus
Copy link
Author

bdarcus commented Feb 26, 2021

Gotcha; thanks.

Just noting UX question while it occurs to me:

Would it be feasible, if there were such a command that allowed acting on the marked candidates, that it would work the same as if a single candidate were selected?

As in, if I hit return, default action, otherwise "a" or the embark-act kb to access the other commands?

Or would one have to use a separate kb?

@oantolin
Copy link
Owner

That's a good idea and definitely feasible.

@bdarcus
Copy link
Author

bdarcus commented Feb 28, 2021

you could just activate embark-collect-direct-action-minor-mode-map

How do you actually "activate" it?

@oantolin
Copy link
Owner

oantolin commented Feb 28, 2021

Just run embark-collect-direct-action-minor-mode, it is bound to capital A in embark collect buffers.

EDIT: Oh, you use Evil, right? Then you use use the Emacs state in embark collect buffers or bind embark-collect-direct-action-minor-mode in the normal state or use M-x embark-collect-direct-action-minor-mode.

EDIT2: Sorry, I had a bunch of spurious -maps above.

@minad
Copy link
Contributor

minad commented Jul 7, 2021

Copying my comment minad/vertico#74 (comment) here:


I have another idea regarding how CRM could be handled in general - going a route via Embark. I would like a solution which somehow fits into the whole picture. The idea is to add some ability to select candidates one by one, adding them to an *Embark Select/Collect* buffer. Maybe this idea is generally useful for Embark (e.g. collecting URLs across buffers).

  1. Add an embark-select action which can be executed anywhere and can be used to collect items. The items go to an *Embark Select/Collect* buffer
  2. Add embark-completing-read-multiple
  3. Embark collect buffers should support candidates of different types
  4. Embark collect buffers need a function to execute an action on all candidates
(defun embark-select ()
  (interactive)
  ;; Here the candidates should actually go to an embark-collect-mode buffer
  ;; Unfortunately embark-collect-mode buffers only support types of a single candidate (generalize this?)
  (let ((cand (cadr (embark--target)))
        (window-min-height 1)
        (window-resize-pixelwise t))
    (with-current-buffer (get-buffer-create "*Embark Select*")
      (goto-char (point-max))
      (unless (= (point) (point-min))
        (insert "\n"))
      (insert cand))
    (fit-window-to-buffer
     (display-buffer "*Embark Select*"
                     `(display-buffer-in-side-window
                       (window-parameters (mode-line-format . none))
                       (window-height . 1)
                       (side . bottom)
                       (slot . -1))))))

(defun embark-select-setup ()
  (lambda ()
    (with-current-buffer (get-buffer-create "*Embark Select*")
      (erase-buffer))))

(defun embark-completing-read-multiple (&rest args)
  (let ((item (apply #'completing-read args)))
    (or
     ;; If items have been selected with Embark, return them
     (mapcar #'substring-no-properties
             (split-string
              (with-current-buffer (get-buffer-create "*Embark Select*")
                (buffer-string))
              "\n" 'omit-nulls))
     ;; Otherwise return the single item
     (list item))))

(add-hook #'minibuffer-setup-hook #'embark-select-setup)
(define-key vertico-map [backtab] #'embark-select)

@minad
Copy link
Contributor

minad commented Mar 3, 2022

This appeared on reddit again: https://www.reddit.com/r/emacs/comments/t5nc1a/can_consult_mark_multiple_items_to_act_on_all_of/. I also discussed recently about candidate marking with @karthink. I have the following in my configuration, which is nice since it brings the collect buffer to a similar level as Ibuffer or Dired with minimal effort. However @karthink mentioned worries, that the API and user surface of Embark are increasing. So should we add something like this or is it better to avoid it? I didn't make use of tablist here, since the grid view is not compatible with it.

(defun +embark-mark (&optional unmark)
  (interactive)
  (unless (derived-mode-p #'embark-collect-mode)
    (error "Not in an Embark collect buffer"))
  (when-let (target (embark-target-collect-candidate))
    (pcase-let* ((`(,_type ,_cand ,beg . ,end) target)
                 (ov (seq-find (lambda (ov) (overlay-get ov '+embark-mark)) (overlays-at beg))))
      (unless (eq (not ov) unmark)
        (if ov
            (delete-overlay ov)
          (unless (facep 'dired-marked)
            (require 'dired))
          (setq ov (make-overlay beg end))
          (overlay-put ov '+embark-mark t)
          (overlay-put ov 'face 'dired-marked)))))
  (call-interactively #'next-line))

(defun +embark-unmark ()
  (interactive)
  (+embark-mark t))

(defun +embark-marked-candidates ()
  (when-let (ovs (and (derived-mode-p #'embark-collect-mode)
                      (nreverse (seq-filter (lambda (ov) (overlay-get ov '+embark-mark))
                                            (overlays-in (point-min) (point-max))))))
    (cons embark--type
          (save-excursion
            (mapcar (lambda (ov)
                      (goto-char (overlay-start ov))
                      (cadr (embark-target-collect-candidate)))
                    ovs)))))

(add-to-list 'embark-candidate-collectors #'+embark-marked-candidates)
(define-key embark-collect-mode-map "m" #'+embark-mark)
(define-key embark-collect-mode-map "u" #'+embark-unmark)

@oantolin
Copy link
Owner

oantolin commented Mar 3, 2022

Ooh, I like it. It just needs replacing (call-interactively #'next-line) with (forward-button): there is grid view which can show multiple candidates on one line, and also some candidates can span multiple lines (great for browsing the kill-ring, for instance). (Aside: why the call-interactively there?)

I think that's a nice compact addition that adds something people might occasionally use. I'd be in favor of adding that to Embark.

@bdarcus
Copy link
Author

bdarcus commented Mar 3, 2022

should we add something like this or is it better to avoid it?

I'm not able to test it ATM, but WDYT @minad? I see @oantolin is in favor.

@minad
Copy link
Contributor

minad commented Mar 3, 2022

Okay, I am also in favor of adding this here, since it brings the collect buffer more in line with Dired and Ibuffer. I will prepare a PR.

There is always the worry that we are overdoing it here, adding too many features, too much complexity. In this case the addition is orthogonal to the rest of the code and it is quite simpel. It doesn't add complexity to the existing code, it only increases the set of available commands (compare with #466 where steps are taken to untangle existing complex code).

EDIT: However, actually there exist complications - e.g. when switching the display mode and when reverting the display. Maybe we should wait a bit with merging this after #466 has been merged. Furthermore I wonder if it makes sense to strip down the Embark display code even more. Do we need the different display modes, do we need the toggling between the modes, do we win anything from using tabulated-list-mode, ...? I think Embark could get by with a simpler display implementation without losing anything from its core values. I prepared a draft PR in #467. It is missing the code to update the overlays when switching the display mode.

@karthink
Copy link
Contributor

karthink commented Mar 3, 2022

Between adding an embark-select and this approach, I agree with @minad that this approach is better, and not just because the code is simpler. By providing dired/ibuffer style marking, it's also closer in spirit to Emacs' default usage paradigm. I have a couple of questions about how this should work.

  • In dired/ibuffer, the same command works on a single candidate or on a set of marked candidates. Here it looks like embark-act still works only on the target at point while embark-act-all works on all marked candidates. Does it make more sense for embark-act to act on all marked candidates instead?

  • Should we add a command to invert the marked candidate selection? This is typically available under t as dired-toggle-marks or ibuffer-toggle-marks. (Also a command to umark all)

@minad
Copy link
Contributor

minad commented Mar 3, 2022

Does it make more sense for embark-act to act on all marked candidates instead?

No, this does not fit the Embark paradigm. embark-act uses the current target, while embark-act-all uses the candidate collector. We need this distinction, since both target finders and collectors may return non-nil in the current context. I don't think the argument that Ibuffer or Dired do things in a certain way matters that much. We should keep the current Embark design. The proposal of adding embark-collect-mark is just a cheap addition on top of the existing infrastructure. Note that when acting with Embark in Dired buffers we also act on the current file at point. When files are marked then embark-act-all will act only on the marked candidates. So the behavior will be consistent across Embark at least.

Should we add a command to invert the marked candidate selection?

I am not opposed to adding that, but I also think that this is not an important addition. First things first. The current implementation needs a bit of cleanup anyway (#466, #468), then we can add the marking code and then we can add even more commands to improve the marking but I am advocating for taking it slowly instead of stuffing too many features into Embark.

@karthink
Copy link
Contributor

karthink commented Mar 4, 2022

No, this does not fit the Embark paradigm. embark-act uses the current target, while embark-act-all uses the candidate collector...Note that when acting with Embark in Dired buffers we also act on the current file at point. When files are marked then embark-act-all will act only on the marked candidates. So the behavior will be consistent across Embark at least.

Okay, that distinction makes sense.

I am not opposed to adding that, but I also think that this is not an important addition. First things first.

Yes, the marking UI is a secondary concern. Just wanted to throw the idea in there.

@minad
Copy link
Contributor

minad commented Mar 28, 2022

@oantolin This issue can be closed?

@oantolin
Copy link
Owner

Yes! Thanks for pointing this out, and for finding those other duplicates.

@minad
Copy link
Contributor

minad commented Mar 28, 2022

I think there are a few more stale issues which can be closed, such that it becomes a bit more clear which issues may be worked on in the future. I will also ping you there.

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

4 participants