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

Marking candidates #467

Closed
wants to merge 2 commits into from
Closed

Marking candidates #467

wants to merge 2 commits into from

Conversation

minad
Copy link
Contributor

@minad minad commented Mar 3, 2022

See the discussion in #166

@minad
Copy link
Contributor Author

minad commented Mar 3, 2022

@oantolin Can we merge #466 before proceeding on this one?

@minad minad force-pushed the marking-candidates branch from 9b6b353 to 4cda2c5 Compare March 3, 2022 15:06
@oantolin
Copy link
Owner

oantolin commented Mar 3, 2022

@oantolin Can we merge #466 before proceeding on this one?

Sure, but it'll take me a couple of days to get to this probably.

@minad
Copy link
Contributor Author

minad commented Mar 3, 2022

Sure, but it'll take me a couple of days to get to this probably.

There is no hurry here! I just think it is better to do a bit of cleanup first before we add new features, see also #468.

@oantolin
Copy link
Owner

I've implemented the simplified embark-collect & embark-live now.

@oantolin
Copy link
Owner

I'm going to take this code and update it for the recent changes.

@oantolin oantolin closed this Mar 21, 2022
oantolin added a commit that referenced this pull request Mar 21, 2022
@oantolin
Copy link
Owner

Done. 6a8e6de

I added toggle-marks and unmark-all commands as well.

@minad
Copy link
Contributor Author

minad commented Mar 21, 2022

Great, thanks! How did you address this comment about moving the overlays when toggling the view modes?

;; TODO Do we need this variable?
;; Also add a list of marked candidates, or a list of overlays?
;; When switching the display modes we have to move the overlays too.
;; We could also turn this variable into an alist, with value=marked.

@oantolin
Copy link
Owner

I did not address it all. 🙄 The marks just vanish. 🤣

@oantolin
Copy link
Owner

I created issue #473 so I don't forget to fix this.

@bdarcus
Copy link

bdarcus commented Mar 21, 2022

Where would you like feedback on this, or possibly dumb questions, @oantolin? Here?

@oantolin
Copy link
Owner

Here would be fine, @bdarcus.

@bdarcus
Copy link

bdarcus commented Mar 21, 2022

OK, so I tested this on my obvious use case: inserting multiple citations.

So I added org-cite-insert to embark-multitarget-actions, and then:

  1. narrowed completions
  2. ran embark-collect
  3. marked two candidates
  4. ran embart-act-all

... and here's where I got stuck.

I think ideally embark would just run the command.

But it doesn't; it prompts if I want to run it. If I do, it still doesn't actually do so, and prompts again.

In the end, I can't get a result, and find it all confusing at this point.

Am I doing something wrong?

And did you add an option to not prompt? I recall we discussed this earlier.

@oantolin
Copy link
Owner

There is an option to avoid confirming embark-act-all, it's called embark-confirm-act-all and defaults to t.

I'm not sure what happened after you confirmed the command, the part that says "If I do, it still doesn't actually do so, and prompts again." I'll test here and see if I can reproduce that.

@bdarcus
Copy link

bdarcus commented Mar 21, 2022

Also, how would you two imagine keybindings for mark/unmark?

I had been wondering about SPC for both, but I guess, notwithstanding the question of whether that's a good idea, that's currently not possible since there's no embark-collect-toggle-mark (there is a "toggle-marks" command, but that applies to all candidates in the collect buffer)?

@oantolin
Copy link
Owner

oantolin commented Mar 21, 2022

Oh, I try to not invent key bindings if I can avoid it, I just copied copied dired and ibuffer: mark is m, unmark is u, toggle everything is t, unmark everything is U. That's what I imagine Emacs user already have muscle memory for.

@bdarcus
Copy link

bdarcus commented Mar 21, 2022

Oh, I try to not invent key bindings if I can avoid it: I just copied copied dired and ibuffer, mark is m, unmark is u, toggle everything is t, unmark everything is U. That's what I imagine Emacs user already have muscle memory for.

Ah right; makes total sense!

@oantolin
Copy link
Owner

I see what you mean with org-cite-insert prompting again. It's an issue even when not using multi-target actions and without marks: if you press RET on a candidate in the Embark Collect buffer, you still get prompted and need to enter an empty input.

@oantolin
Copy link
Owner

So I added org-cite-insert to embark-multitarget-actions

I think this shouldn't work, because that tells Embark to call the functions with a list of candidates, and org-cite-insert does not take a list as an argument.

@bdarcus
Copy link

bdarcus commented Mar 21, 2022

So I added org-cite-insert to embark-multitarget-actions

I think this shouldn't work, because that tells Embark to call the functions with a list of candidates, and org-cite-insert does not take a list as an argument.

OK. I need to think about that some more.

But if I do citar-insert-citation instead, I do get a result closer to expectation, but it's not the key that gets inserted, but the whole candidates.

I don't recall seeing anything like that with embark-act-all earlier.

@oantolin
Copy link
Owner

This seems to work if you have org-cite-insert-processor set to basic:

(defun cite-all (refs)
  (interactive)
  (cl-letf (((symbol-function #'org-cite-basic--complete-key)
             (lambda (&optional _)
               (let ((tbl (org-cite-basic--key-completion-table)))
                 (mapcar (lambda (ref) (gethash ref tbl)) refs)))))
    (org-cite-insert nil)))

(add-to-list 'embark-multitarget-actions 'cite-all)

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

Successfully merging this pull request may close these issues.

3 participants