-
Notifications
You must be signed in to change notification settings - Fork 109
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
[WIP] Add timestamp selector. #154
base: master
Are you sure you want to change the base?
[WIP] Add timestamp selector. #154
Conversation
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 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. |
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. |
132dfc1
to
d2d5f28
Compare
Updated what I think should be a working implementation. I am not quite happy with having to use a Documentation is up next. |
I would suggest copying the relevant code from |
d2d5f28
to
d68dcfd
Compare
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. |
Why did you only support
|
768cd70
to
82e546d
Compare
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 🤷♂️. I've added the same options as DEADLINE now, hope that's sufficient. (Also included some docstring highlighting). The |
:) 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 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. |
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.
Done.
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. |
82e546d
to
a23d9cb
Compare
I plan to merge #149, so this PR should probably implement that as well. @Alexander-Miller Would you mind doing that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
README.org
Outdated
@@ -193,6 +193,7 @@ These selectors take one argument alone, or multiple arguments in a list. | |||
+ =:children= :: Select any item that has child entries. Argument may be ~t~ to match if it has any children, ~nil~ to match if it has no children, ~todo~ to match if it has children with any to-do keywords, or a string to match if it has children with certain to-do keywords. You might use this to select items that are project top-level headings. Be aware that this may be very slow in non-daily/weekly agenda views because of its recursive nature. | |||
+ =:date= :: Group items that have a date associated. Argument can be =t= to match items with any date, =nil= to match items without a date, or =today= to match items with today’s date. The =ts-date= text-property is matched against. | |||
+ =: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. | |||
+ =:timestamp= :: Group items whose bodies contain an active timestamp. Argument can be ~t~ (to match items with any timestamp), ~past~ (to match items with timestamps in the past), ~today~ (to match items with timestamps set to today), or ~future~ (to match items with timestamps 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Please ensure two spaces between sentences in docs and docstrings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
org-super-agenda.el
Outdated
:let* ((now (ts-now)) | ||
(target-date (pcase (car args) | ||
((or 'before 'on 'after) | ||
(make-ts :internal (org-time-string-to-absolute (cadr args)))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In line with #149, we should probably use org-read-date
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
org-super-agenda.el
Outdated
(test (pcase-exhaustive (car args) | ||
('t (lambda (_) t)) | ||
('future (lambda (it) (ts>= it now))) | ||
('past (lambda (it) (ts<= it now))) | ||
('before (lambda (it) (ts<= it target-date))) | ||
('after (lambda (it) (ts>= it target-date))) | ||
('on (lambda (it) (and (= (ts-year it) (ts-year target-date)) | ||
(= (ts-month it) (ts-month target-date)) | ||
(= (ts-day it) (ts-day target-date)))))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice, but what about using org-super-agenda--compare-dates
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
org-super-agenda.info
Outdated
@@ -1,4 +1,4 @@ | |||
This is README.info, produced by makeinfo version 5.2 from README.texi. | |||
This is README.info, produced by makeinfo version 6.7 from README.texi. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate your being through, but if you don't mind, please omit the updates to the Info manual from the PR, and I can regenerate it myself using the same makeinfo version, to minimize differences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
a23d9cb
to
afae290
Compare
@alphapapa Updated changes from your comments for now, next I'll look into #149 and adding tests. |
b84b744
to
3ce439e
Compare
Draft for a selector that AFAICS is unavailable so far. It lets you match for timestamps in headlines like
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.