search

alphapapa / org-super-agenda

Supercharge your Org daily/weekly agenda by grouping items
GNU General Public License v3.0
1.35k stars 107 forks source link

[WIP] Add timestamp selector. #154

Open Alexander-Miller opened 4 years ago

Alexander-Miller commented 4 years ago

Draft for a selector that AFAICS is unavailable so far. It lets you match for timestamps in headlines like

*** Some Appointment
<2020-05-17 So 14:00>

I am using this selector to show me a focused list of all upcoming appointments in my agenda since my timeline is usually only 2 weeks long and choke-full with habits. So far it only matches future timestamps, but that can be easily expanded.

If you give your blessing for the feature (and I haven't just missed it) I'll finish this PR with all the necessary cleanup/options/documentation.

alphapapa commented 4 years ago

Hi,

Thanks for your patience. I've been taking a little break from working on my Emacs packages.

This seems like a good idea. It seems to mirror the ts predicate from org-ql.

You may already plan this optimization, but in case you haven't: rather than gathering all of the timestamps into a list (which does a lot of consing), the comparison could be done in the loop, and it could return as soon as a matching one is found. I'm guessing that would improve performance noticeably, at least in cases with hundreds or thousands of results.

Alexander-Miller commented 4 years ago

I haven't quite thought so far ahead, I had only starting groking how it works when I had the working version you see here. I'll take it into account for the next iteration.

Alexander-Miller commented 4 years ago

Updated what I think should be a working implementation.

I am not quite happy with having to use a catch, but cannot think of a better option - doing a (cl-loop ... thereis (test it)) doesn't work because without an exit condition it infinitely loops ove the nils returned by re-search-forward. Maybe you have a better idea?

Documentation is up next.

alphapapa commented 4 years ago

I am not quite happy with having to use a catch, but cannot think of a better option - doing a (cl-loop ... thereis (test it)) doesn't work because without an exit condition it infinitely loops ove the nils returned by re-search-forward. Maybe you have a better idea?

I would suggest copying the relevant code from org-ql, i.e. https://github.com/alphapapa/org-ql/blob/c847afe0b538a1a44c73e40b067831cbea132ba7/org-ql.el#L1556

Alexander-Miller commented 4 years ago

Got things clean up and documented now.

There's quite a few changes to the .info file, probably due to the version difference, so I think you should regenerate the file on your end, or the changes will just be reverted on your next commit anyway.

alphapapa commented 4 years ago

Why did you only support any and future? Other time/date-related group selectors in this package support a wider variety of options, like:

:deadline Group items that have a deadline. Argument can be t (to match items with any deadline), nil (to match items that have no deadline), past (to match items with a deadline in the past), today (to match items whose deadline is today), or future (to match items with a deadline in the future). Argument may also be given like before DATE or after DATE where DATE is a date string that org-time-string-to-absolute can process.

Alexander-Miller commented 4 years ago

Because I don't know the feature list all that well and was only looking at whatever what be useful with my own use-case :man_shrugging:.

I've added the same options as DEADLINE now, hope that's sufficient. (Also included some docstring highlighting). The nil case has gotten a bit ugly due to the while next-ts in the loop, but I don't know all that much about cl-loop either.

alphapapa commented 4 years ago

:) Thanks.

I think we can probably omit the nil case, because I don't imagine that would be very useful. If someone requests it, we can consider adding it.

What do you think about also supporting inactive timestamps? I guess they should probably also be supported.

The binding of the lambda forms and use of funcall is probably acceptably fast, but I think those bindings should be done with the :let* argument to org-super-agenda--defgroup so they aren't done for every tested item.

Would you be willing to also add tests for this feature? The test harness is somewhat bespoke and unintuitive (and mostly undocumented), so if you don't want to bother with that, I can't complain. But it will have to be done before the branch can be merged.

Thanks for your work on this.

Alexander-Miller commented 4 years ago

What do you think about also supporting inactive timestamps?

Not something I want to get into in this PR (don't personally see much of a use-case either). But easy to do: it'll just be a 99% copy of this commit, once it's done.

Mind you I did just notice that I used the wrong regex, currently both active and inactive timestamps are matched. Will be corrected with the next push.

I think those bindings should be done with the :let* argument to org-super-agenda--defgroup so they aren't done for every tested item.

Done.

Would you be willing to also add tests for this feature?

I had a look at the tests, but they looked all sorts of non-trivial, so I haven't bothered till now. I've got 2 days until a new project starts at work and I'll be busy, so if you tell me how to go about this I'll see how far I can come in that time.

alphapapa commented 3 years ago

I plan to merge #149, so this PR should probably implement that as well. @Alexander-Miller Would you mind doing that?

Alexander-Miller commented 3 years ago

@alphapapa Updated changes from your comments for now, next I'll look into #149 and adding tests.