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

Add: {not-}done option for :todo selector #268

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Walheimat
Copy link

@Walheimat Walheimat commented Oct 3, 2024

This is in reference to #267.

Users can now set :todo {not-}done to match to-do keywords that are part of org-{not-}done-keywords in that agenda item.

This means that when using "TODO | ACHIEVED" in one item but "TODO | DONE" in another, :todo done will match both "ACHIEVED" and "DONE".

Notes

When I run ./makem.sh test I get errors with and without my changes and I'm not quite sure how to write a test for this new feature. But I'd first like to know if this is going in the right direction in your opinion.

@alphapapa
Copy link
Owner

Hi Krister,

Thanks for working on this. I don't necessarily dislike the :todo done/:todo not-done API, but since Org QL already provides similar functionality, and since I intend for them to be interoperable to some extent, I think I'd prefer to follow its API more closely. So, e.g. since Org QL provides this via (todo) for not-done items, and (done) for done ones, I'd suggest that in org-super-agenda we add a new :done selector that mirrors the :todo selector but only matching done keywords.

Then we might need to consider making the :todo selector only match not-done keywords (but that could be a breaking change, so it'd have to be considered carefully).

What do you think?

@alphapapa alphapapa linked an issue Oct 3, 2024 that may be closed by this pull request
@alphapapa alphapapa added this to the 1.4 milestone Oct 3, 2024
@alphapapa alphapapa self-assigned this Oct 3, 2024
@Walheimat
Copy link
Author

Thanks for taking a look.

So you'd express "not done" then with :done nil? I think having a new selector could also be nice, and maybe make it easier to extend that with something like :done today or :done past (although I'm not sure you can even get agenda items that are done in the past to show up in the first place, but I'm no expert).

I did take a look at org-ql but I couldn't easily find what I'd take from there implementation-wise (but maybe you just meant the semantics?). It seemed to me that using the existing macro to be in the context of the item's buffer and (cl-member (org-find-text-property-in-string 'todo-state item) org-done-keywords :test 'string=) was sufficient for our purposes. If you'd like a change there I'd have to dig a bit deeper.

@alphapapa
Copy link
Owner

alphapapa commented Oct 4, 2024

Thanks for taking a look.

So you'd express "not done" then with :done nil?

No, with :todo t. This should be explained in the documentation.

I think having a new selector could also be nice, and maybe make it easier to extend that with something like :done today or :done past

That's a nice idea too.

(although I'm not sure you can even get agenda items that are done in the past to show up in the first place, but I'm no expert).

You can do pretty much whatever you want. Note that there's no such thing as an "agenda item". Org headings represent "entries," and those entries can be searched with org-agenda or org-ql and then shown in an "Agenda buffer" or an org-ql-view buffer. (Forgive any pedantry, but using "correct" terminology helps understand how the pieces work together.)

I did take a look at org-ql but I couldn't easily find what I'd take from there implementation-wise (but maybe you just meant the semantics?). It seemed to me that using the existing macro to be in the context of the item's buffer and (cl-member (org-find-text-property-in-string 'todo-state item) org-done-keywords :test 'string=) was sufficient for our purposes. If you'd like a change there I'd have to dig a bit deeper.

Please see https://github.com/alphapapa/org-ql/blob/b6f8a315e966123fbfd1ac240d35da5c2b48d6ac/org-ql.el#L2077 and https://github.com/alphapapa/org-ql/blob/b6f8a315e966123fbfd1ac240d35da5c2b48d6ac/org-ql.el#L1386 for what I basically mean. org-super-agenda doesn't work exactly the same way, so some reference to org-super-agenda code will be necessary also. I mean that the org-super-agenda user API should be close to the org-ql one, i.e. since org-ql uses (done), org-super-agenda should use :done.

Adds selector `:done` that groups items that are done according to
`org-done-keywords`.
@Walheimat Walheimat force-pushed the feature/todo-done-and-not-done-matchers branch from f184c49 to df7b07e Compare October 8, 2024 17:53
@Walheimat
Copy link
Author

I hope I captured your desire for consistency now. Now there's just plain :done that will just do (member state org-done-keywords).

@alphapapa
Copy link
Owner

Thanks. The next step would be to add tests for the new selector. Would you be willing to work on that?

@Walheimat
Copy link
Author

Of course. Maybe I can squeeze in some more time tomorrow after work.

@Walheimat
Copy link
Author

How is the hash table in test/result.el updated? I added an entry that would be selected in test.org but that predictably breaks every single test, 😆

@alphapapa
Copy link
Owner

Yeah, this all uses a custom test framework I made for this package (which I might factor out someday...).

First, you should avoid modifying the test data file if at all possible; and if you have to, you should try to make the changes as minimal as possible (i.e. better to modify an entry than add a new one). If you really do have to, you have to, but if not...

For updating the tests, there are a few interactive commands in the test.el file. One command shows the test at point; then when you've confirmed it's correct, you can call the command to save that test's result. There are also commands to update all of the test results if necessary.

You can probably figure it out from that explanation and the code, but if you need more details, let me know. Probably the most important thing is to be sure to review the changes to the files before committing them, to catch any unexpected changes in the results.

Thanks for your work on this.

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

Successfully merging this pull request may close these issues.

Matching "(not) done" when using :todo
2 participants