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

Embark support #299

Closed
akirak opened this issue Aug 29, 2022 · 36 comments
Closed

Embark support #299

akirak opened this issue Aug 29, 2022 · 36 comments

Comments

@akirak
Copy link
Contributor

akirak commented Aug 29, 2022

Hello,

I want to get an olp entered from the user in org-ql-completing-read. Specifically, I am thinking of the following use cases:

  • Let the user choose a heading to clock into. If the heading does not exist, fire org-capture with the input as the title.
  • Let the user choose a heading to refile an entry into. If the heading does not exist, create a new heading.

This can be solved by adding a history variable for completing-read in the command. If the function returns nil, the user can get the latest item from the history and split the string with / to get an olp. This is hacky, but it achieves the desired effect with minimal changes to the implementation.

An alternative way is to change

(gethash selected table)

to

(or (gethash selected table)
            (when return-olp
              (split-string selected "/")))

and add :return-olp argument to the function. The extra argument is needed to avoid confusion in normal use cases, but it is more complicated (in API and implementation) than adding the history variable.

What is the best way to solve the problem?

@alphapapa
Copy link
Owner

Hi Akira,

Yes, clocking and refiling are two obvious uses for org-ql-completing-read. (Ideally Org would have a framework that would allow the function used for selecting entries for various purposes to be set by the user, and we could have a mode that would use org-ql-completing-read for that.)

Anyway, you can see the org-ql-refile command I already implemented here:

(defun org-ql-refile (marker)
(That file may not be the permanent home for that command.)

As for getting an outline path, the function returns a marker, so it should be as simple as this:

(org-with-point-at (org-ql-completing-read ...)
  (org-get-outline-path))

So there should be no need to change the API.

@akirak
Copy link
Contributor Author

akirak commented Aug 29, 2022

Thank you for your quick response.

What I want is a unified completion API that can handle both existing entries (returning a marker) and non-existent ones (returning an olp).

I have the following function that can create a heading from an olp (even if its parent does not exist), which can be integrated with org-capture and org-refile:

(defun akirak-org-goto-or-create-olp (olp)
  (let ((level 1))
    (dolist (heading olp)
      (unless (catch 'found
                (while (re-search-forward (format org-complex-heading-regexp-format
                                                  (regexp-quote heading))
                                          nil t)
                  (when (equal level (- (match-end 1) (match-beginning 1)))
                    (throw 'found t))))
        (org-end-of-subtree)
        (insert "\n" (make-string level ?*) " " heading))
      (org-narrow-to-subtree)
      (setq level (org-get-valid-level (1+ level))))))

It would be nice if I could build the olp using org-ql-completing-read.

@alphapapa
Copy link
Owner

What I want is a unified completion API that can handle both existing entries (returning a marker) and non-existent ones (returning an olp).

That is a nice idea, but returning a non-existent heading would seem to violate the concern of org-ql-completing-read, whose purpose is to return matches for a search query (not to return non-matches). If the user selects a heading with org-ql-completing-read, how is the function to know whether the user wants that heading, or a new, non-existent one to be made under it? As well, the input given to org-ql-completing-read is not an outline path but tokens passed (by default) to the rifle predicate; it wouldn't make sense for a new outline path to be generated from that input.

I think you will need to do something like having an alternative key binding, like M-RET, that would use the heading selected with org-ql-completing-read and prompt the user to insert a new child heading under it.

@akirak
Copy link
Contributor Author

akirak commented Aug 29, 2022

That is a nice idea, but returning a non-existent heading would seem to violate the concern of org-ql-completing-read

I understand your point.

you will need to do something like having an alternative key binding

It doesn't look easy to define such a keybinding. It would be nice if org-ql-completing-read used a dedicated keymap!

@alphapapa
Copy link
Owner

Ok, feel free to propose a patch that adds one. :)

@akirak akirak changed the title Getting the olp from org-ql-completing-read Creating a new heading during a org-ql-completing-read session Aug 29, 2022
@akirak
Copy link
Contributor Author

akirak commented Aug 29, 2022

It would be nice if org-ql-completing-read used a dedicated keymap!

I tried to implement this idea, but it turns out that this is not easy, at least for vertico users. Vertico uses its own keymap:

(defun vertico--setup ()
  "Setup completion UI."
  (setq vertico--input t
        vertico--candidates-ov (make-overlay (point-max) (point-max) nil t t)
        vertico--count-ov (make-overlay (point-min) (point-min) nil t t))
  ;; Set priority for compatibility with `minibuffer-depth-indicate-mode'
  (overlay-put vertico--count-ov 'priority 1)
  (setq-local completion-auto-help nil
              completion-show-inline-help nil)
  (use-local-map vertico-map)
  (add-hook 'post-command-hook #'vertico--exhibit nil 'local))

Even if I could somehow create a heading during the minibuffer session, the completion table would never include a marker to the new entry, which would mean org-ql-completing-read returns nil for non-existent entries. The problem may not be easy to solve, so I will change the title of this ticket and leave it open.

@alphapapa
Copy link
Owner

Ok, maybe we can figure something out in the future. Thanks.

@minad
Copy link

minad commented May 25, 2023

Not sure if I understand this issue correctly, but if you want a local keymap for a command you can use minibuffer-with-setup-hook and create a new local map, composed with the existing minibuffer map. See for example jinx--correct-setup from my Jinx package:

This approach should be compatible with all completion UIs (at least as long as you don't override some important bindings which are already in use).

@alphapapa
Copy link
Owner

@minad Thanks.

Since this issue was originally filed, embark has become quite a powerful tool, and it now has support for various Org-related actions: oantolin/embark#562 So I think the best way to support this would be for org-ql to support embark, so we would gain all of its power.

@alphapapa alphapapa changed the title Creating a new heading during a org-ql-completing-read session Embark support Sep 7, 2023
@alphapapa
Copy link
Owner

Repurposing this issue for Embark support.

@oantolin made this comment:

Ha! I just added a text property storing the marker to my local copy of org-ql! I didn't do it to fix #350, but to enable embark actions on headings listed by org-ql-find. If this gets merged, and the org-ql-completing-read completion table gets a category metadatum, then I can easily write a tiny org-ql-embark package offering support for embark-collect and for using Org heading actions on org-ql-find candidates.

Omar, if you're still interested in working on that, please let me know how I can help. One feature I'd like to add is the equivalent of helm-org-ql-save, which in Embark terms would be like "exporting".

@alphapapa alphapapa added this to the 0.8 milestone Sep 7, 2023
@alphapapa alphapapa self-assigned this Sep 7, 2023
@oantolin
Copy link
Contributor

oantolin commented Sep 7, 2023

I am interested in working on Embark support.

For actions on the org-ql-find completion candidates I think the just-merged org-marker text properties are enough. With that I can have Embark jump to the heading before executing an action.

For export, can I just stick the strings that have the org-marker property into a buffer and put it in org-agenda mode? I'm not very familiar with org internals.

Oh, I just took a peek at my org agenda and boy do those lines have text properties! So maybe it's not as simple as inserting the org-q-find candidates into a buffer. Does org-ql have a helper function that can make an org-ql view buffer from a list of strings with org-marker text properties? Probably not.

@alphapapa
Copy link
Owner

alphapapa commented Sep 7, 2023

I am interested in working on Embark support.

Hurray! :)

For export, can I just stick the strings that have the org-marker property into a buffer and put it in org-agenda mode? I'm not very familiar with org internals.

No, the model to follow is here:

org-ql/helm-org-ql.el

Lines 162 to 168 in f65b1d9

(defun helm-org-ql-save ()
"Show `helm-org-ql' search in an `org-ql-search' buffer."
(interactive)
(let ((buffers-files (with-current-buffer (helm-buffer-get)
helm-org-ql-buffers-files))
(query (org-ql--query-string-to-sexp helm-pattern)))
(helm-run-after-exit #'org-ql-search buffers-files query)))
Basically, we want to take the search query string and pass it to org-ql-search; it will then show the same results in an org-ql-view buffer (similar to org-agenda).

@oantolin
Copy link
Contributor

oantolin commented Sep 7, 2023

Oh, that doesn't quite fit the Embark export model, which is supposed to be a function of the list of completion candidates, not of the minibuffer input used to produce them. But I guess we can cheat.

@oantolin
Copy link
Contributor

oantolin commented Sep 7, 2023

The cheating solution would be slightly limited compared to usual embark export: normally you can use embark-select to choose some subset of the completion candidates and export only those.

@oantolin
Copy link
Contributor

oantolin commented Sep 7, 2023

Mmh. Is there an analogue of the helm-org-ql-buffers-files dynamic variable for org-ql-find? I think that information is locked away in a lexically scoped function parameter, right?

@minad
Copy link

minad commented Sep 7, 2023

@oantolin The function helm-org-ql-save seems more like a normal command which can be bound in the minibuffer-local-map and not something necessarily related to Embark? This could be implemented via minibuffer-setup-hook without Embark. But maybe I am missing the point. @alphapapa Btw, thanks for merging #357 and fixing #350!

@oantolin
Copy link
Contributor

oantolin commented Sep 7, 2023

Yes, definitely, @minad. That kind of function wouldn't really take any advantage of Embark infrastructure. It would be better bound to some key in the minibuffer via a setup hook as you suggest. But then it should be just made a part of org-ql-find (which would make the problem with the buffer-files being lexically scoped ---if I'm right about that--- not a problem anymore).

@oantolin
Copy link
Contributor

oantolin commented Sep 7, 2023

Embark users, if they wished, could remap embark-export to that org-ql-save function while in org-ql-find, just to take advantage of muscle memory.

@minad
Copy link

minad commented Sep 7, 2023

It could still be that some org-ql-specific support is in order, leading to an embark-org-ql package, or a tiny bit of integration code in a (with-eval-after-load 'embark) block in org-ql? I suspect that we may need something similar like embark-consult-goto-grep.

@oantolin
Copy link
Contributor

oantolin commented Sep 7, 2023

Oh, yes, if we want embark-collect to work, definitely a default action override is needed. There's enough to warrant an embark-org-ql package even if this export-like functionality is more properly left inside org-ql:

  • To be able to target org-ql-find candidates we need either to change org-ql-completing-read to add a completion category, or, it can be solved externally in an embark-org-ql package via a transformer that refines general targets that have an org-marker text property to some org-ql-result type.
  • The embark-org-heading-map should be registered as the action map for the org-ql-result type.
  • A pre-action hook that uses the org-marker text propery to jump to the org heading before running the action would be needed.
  • For RET to work in embark-collect buffers a default action override is needed similar to the embark-consult-goto-grep function you mentioned.

@oantolin
Copy link
Contributor

oantolin commented Sep 7, 2023

(Clearly I forgot the first line of that comment by the time I wrote the last one: damn Chrome and its tiny-by-default text areas. 😄 It's actually pretty unusual that I wrote that in Chrome instead of copy-pasting from Emacs.)

@minad
Copy link

minad commented Sep 7, 2023

Yes, sounds good. I suggest to add a completion category here. I consider this good style because of completion-category-overrides, Embark, Marginalia, etc., and such that no guessing and transforming is needed.

@oantolin
Copy link
Contributor

oantolin commented Sep 7, 2023

I suggest to add a completion category here. I consider this good style

Oh, I agree, and proposed that a little over two months ago.

@oantolin
Copy link
Contributor

oantolin commented Sep 7, 2023

I just remembered there was a discussion on oantolin/embark#639 about how to best support acting on agenda items and you, @minad, pointed out that the current support for the org-agenda commands is pretty superfluous since those commands already have bindings in org-agenda-mode-map. I then suggested a different approach exactly like what is needed here: instead of using the agenda-item commands as actions, to support using the org heading actions via a pre-action hook that jumps to the heading. If I did that in embark-org, all that would be needed here for actions is just adding the completion category and they would work out of the box!

@oantolin
Copy link
Contributor

oantolin commented Sep 7, 2023

I'm inclined to make that change in embark-org and make a one-line PR here adding the completion category and no embark-org-ql package would be needed. Oh, and then someone would write the org-ql-save command to add here...

@oantolin
Copy link
Contributor

oantolin commented Sep 7, 2023

Woah! org-ql-completing-read overrides completion-styles! That seems crazy. Why not just provide a completion table? Overriding completion-styles totally messes with my recursive minibuffer habit.

@oantolin
Copy link
Contributor

oantolin commented Sep 7, 2023

OK. If anyone wants to try preliminary Embark support, just use the remote-org-heading branch of Embark and use this Marginalia configuration:

(add-to-list 'marginalia-command-categories '(org-ql-find . org-remote-heading))

Actions offered are the same as for the org heading at point in an org buffer, and when acting on an org-ql-find candidate you visibly jump to the actual heading before the action occurs. (I also implemented a version that jumps invisibly but feel that might be a little confusing; that version uses embark-org--at-remote-heading as an around action hook instead of using embark-org-goto-remote-heading as a pre-action hook).

You can currently run embark-collect (whose usual binding is shadowed 🙄) or embark-export to get the same result: a collect buffer that looks a little oddly formatted but works.

@minad
Copy link

minad commented Sep 7, 2023 via email

@oantolin
Copy link
Contributor

oantolin commented Sep 7, 2023

I realized that a little later, @minad. Even if you can get a completion table to produce the right candidates you definitely don't want to match against them in the normal way. I posted here in shock that my recursive minibuffer was broken without pausing to think. The correct thing to say was instead that setq-local should be used, not let.

@oantolin
Copy link
Contributor

oantolin commented Sep 8, 2023

Of the original use cases @akirak mentioned, the current Embark support takes care of clocking in. For refiling a special action function should be wrriten, because if you use org-refile on heading X it means refile X somewhere else, and I think @akirak wanted an action to refile the heading that was at point before running org-ql-find to X. That extra function would be just like org-ql-refile except non-interactive and it would take a string with an org-marker text property instead of directly taking the marker.

@alphapapa
Copy link
Owner

FWIW, the helm-org source's actions list should have some good examples of what's useful.

@oantolin
Copy link
Contributor

oantolin commented Sep 8, 2023

FWIW, the helm-org source's actions list should have some good examples of what's useful.

It seems to have four: goto heading, open heading in indirect buffer, refile to heading (the missing one I mentioned above), and insert link to heading. Funnily enough embark only has the first one bound to a key, but opening a heading in an indirect buffer and storing (not inserting, though) a link to a heading are org commands you can use as an action. I'll see about adding single-letter keybindings for them.

I'm a little surprised helm-org didn't have actions for clocking in, scheduling, adding a deadline, setting properties, etc., like embark-org does (I mean bindings for them: I didn't write them, they are the normal org commands).

@oantolin
Copy link
Contributor

oantolin commented Sep 8, 2023

Oh, I guess I'm not such an embark expert as I thought I was: there is already a key binding for org-store-link. But not for the indirect buffer thing.

@oantolin
Copy link
Contributor

oantolin commented Sep 8, 2023

One thing I can't decide on is whether to display the buffer containing the heading you act on from org-ql-find or not. I feel like promoting or demoting a heading you can't see is asking for trouble, or worse, killing a subtree you can't see! But storing a link to a heading you can't see is fine, as is refiling it, and probably also refiling to it. I should probably just keep both options in the code base and hand-pick which actions display the buffer and which one don't, right?

@oantolin
Copy link
Contributor

oantolin commented Sep 8, 2023

OK, so I made two PRs: #368 with the moral equivalent of helm-org-ql-save, and #369 which adds support for Embark actions and adds the refile to action that embark-org doesn't have (because it only makes sense in the context of org-ql).

@alphapapa
Copy link
Owner

I think this is ready for v0.8. Thanks to all!

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

No branches or pull requests

4 participants