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

Make :date behave as :deadline and :scheduled #96

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

zaeph
Copy link

@zaeph zaeph commented Jul 15, 2019

Hello.

This is a PR for making :date behave as :deadline and :scheduled.

In its current state, :date does not allow other time comparisons other than today, which is a shame because org-agenda-todo-ignore-timestamp handles past and future.

Originally, I only intended to adapt the defgroup for :date by using :deadline’s or :schedule’s, but seeing how similar and repetitive they were, I decided to tackle the creation of a macro which was mentioned in a comment.

I’ve also decided to deprecate/alias :date in favour of :timestamp for reasons described in the commit-message for a13c73f.

I’ve updated the documentation, but because I have a more recent version of makeinfo (6.6 vs 5.2), a lot of lines have been changed, notably with the inclusion of inverted commas. So, I’ve split the documentation update for README.info in two commits: the first one only updates the lines I’ve modified, and the second one updates the entire file.

I’ll also use this opportunity to thank you for the work you’ve been putting in org-super-agenda. I consider it to be as essential to my setup as org-agenda-skip-function and org-agenda-sorting-strategy, especially thanks to :pred.

HTH.

;; :habit selector used but `org-habit' not loaded
(user-error "Please `require' the `org-habit' library to use the :habit selector"))
;; Deprecated selector: raise warning
((when-let ((new-selector (alist-get selector
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m using alist-get, but I think this is a recent addition to Emacs. We can use (cdr (assoc …)) instead.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alist-get is fine, and I much prefer it over (cdr (assoc. :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect! I was in the process of rebasing the commits on top of master. Would you like me to still do it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up merging master.

Copy link
Owner

@alphapapa alphapapa Jul 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase it. PRs should generally, if not always, be rebased. You should never merge master into a PR branch; imagine the commit graph when the PR branch is then merged back into master! :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll do that instead.

@zaeph zaeph closed this Jul 24, 2019
@zaeph zaeph deleted the improve-date branch July 24, 2019 07:31
@zaeph zaeph restored the improve-date branch July 24, 2019 07:31
@zaeph zaeph reopened this Jul 24, 2019
@alphapapa
Copy link
Owner

alphapapa commented Jul 24, 2019

Hi,

Sorry for the delay. I've been working on some other things recently, as you can see.

This PR looks very well done, and I think it's basically a good idea. I think it's always been unclear exactly what the :date selector does, because it uses the ts-date text-property that Org Agenda makes, and it's not clear what kind of timestamp that represents. Even I forget the details, and I never use it.

Here are a few thoughts in no particular order:

  • Renaming it to :timestamp is probably a good idea. As you said, it's more consistent and accurate.

  • I think there should also be a shorter :ts alias, because :timestamp is a bit long.

  • I probably need to look at the code more carefully, but from looking at the readme, I'm still not clear what kind of timestamp :timestamp selects. If this PR doesn't resolve that issue, it probably should before we merge it.

  • On that note, it might be better to simply remove or deprecate the :date/:timestamp selector for now, rather than have an ambiguous selector that users don't know when to use.

    Eventually I would like to use ts.el for a :timestamp selector that would look at all timestamps in an entry, not just DEADLINE, SCHEDULED, or CLOSED. ts.el needs more polishing before it is ready to be used by other projects, and that might take a while.

What do you think?

By the way, I may not have time to look at this PR much further for a few weeks. I'll try to check on it if I have time, but I can't make any guarantees at the moment, as I'm going to be busy with some other things. I appreciate your patience, in advance. :) Please feel free to ping me if I forget for a while.

Thanks for your work on this! I think it will be a nice improvement to the code.

@alphapapa alphapapa self-assigned this Jul 24, 2019
zaeph added 12 commits July 24, 2019 09:42
The defgroup for SCHEDULED and DEADLINE are practically the same, save
for the special property they’re retrieving with ‘org-entry-get’.
However, the defgroup for matching timestamps is different, probably
due to an earlier conception.

For instance, compared to its SCHEDULED and DEADLINE equivalents, this
defgroup does not allow matching past or future timestamps with 'past
or 'future.

In order to address this issue, and for the sake of standardisation,
this commit reuses the template for SCHEDULED and DEADLINE, and only
modified the special property in ‘org-entry-get’.
Based on ‘org-super-agenda--defgroup’.

Instead of a :TEST argument, this macro offers :TEST-PROPERTY which
expect the name (as a string) of the property to use for the
comparison.  It then forwards the template to
‘org-super-agenda-defgroup’ for further expansion.
This allow emacs-lisp-mode to use the right face for the docstrings.
I think it’s a little clearer to use ‘timestamp’, for 3 reasons:

1) It’s the name of the special property we use.
   That’s why *we* want it.

2) In the doc, we refer to DATE as an argument which can be used after
   ‘after’ or ‘before’ for our defgroups-with-time-tests, so we can
   differentiate a timestamp as being a DATE without a special prefix
   word like SCHEDULED or DEADLINE.
   That’s why *everyone* might want it.

3) org-mode also uses the word ‘timestamp’ to describe them in the
   ‘org-agenda-todo-ignore-…’  functions.
   That’s why the users *might* want it.

Do note that this is far from being a consensus within org-mode
itself.  For instance, the docstring for ‘org-agenda-skip-if’
implies that timestamps should also match deadline and scheduled:

> timestamp    Check if there is a timestamp (also deadline or
>              scheduled)
- Adjust ‘date’ to ‘timestamp’.
- Add forgotten verbatim sign around ‘DATE’.
Lots of changes caused by makeinfo 5.2 → 6.6.
Deprecate/alias didn’t work because we use a built-in function to
validate the selector (‘org-super-agenda--get-selector-fn’).

This commit addresses this problem by creating the variable
‘org-super-agenda-deprecated-selectors-alist’ in which each cons is
written as (deprecated-selector . new-selector).  We then use this
variable to validate the selector before we throwing an error in
‘org-super-agenda--get-selector-fn’.
This reverts commit a13c73f.

Note that this isn’t true revert, since we’re keeping the new selector
name.
@alphapapa
Copy link
Owner

By the way, I'm leaning toward simply removing the :date group selector altogether for now, rather than trying to deprecate it with special code to check for deprecated selectors. There may be someone out there using it, but I haven't seen it "in the wild" anywhere, even though I've spent some time looking at configs on GitHub. If anyone is using it, it should be easy to replace with one of the other group selectors. And if that's too much trouble at the time, they could switch to the MELPA Stable version.

@zaeph
Copy link
Author

zaeph commented Jul 24, 2019

Hi there,

(Sorry for closing the PR for a bit; I renamed the wrong branch.)

This PR looks very well done, and I think it's basically a good idea.

Thank you!

I think it's always been unclear exactly what the :date selector does, because it uses the ts-date text-property that Org Agenda makes, and it's not clear what kind of timestamp that represents. Even I forget the details, and I never use it.

You’re not the only one. Different parts of org-mode deal differently with timestamps, which is the chiefest reason for the confusion.

Renaming it to :timestamp is probably a good idea. As you said, it's more consistent and accurate.

I think it’s better than :date, but I’m not sure if that’s the best we could find. I’m open to suggestions, though.

I think there should also be a shorter :ts alias, because :timestamp is a bit long.

I don’t see any problem with that. I’ll rename the selector to :ts in this PR.

I probably need to look at the code more carefully, but from looking at the readme, I'm still not clear what kind of timestamp :timestamp selects. If this PR doesn't resolve that issue, it probably should before we merge it.

Yeah, I felt that way too upon re-reading it today. Since it’s not something that is clearly defined in org-mode’s doc, I think we should take it upon ourselves to define the terms.

On that note, it might be better to simply remove or deprecate the :date/:timestamp selector for now, rather than have an ambiguous selector that users don't know when to use.

I was actually prompted to review the :date selector because I wanted to experiment with TODOs with plain timestamps rather than SCHEDULED:. The rationale behind it was that the TODO was something that I had to do at that timestamp, and that scheduling it was not enough. I was happy to find that org-agenda offered a selector only for those, which comforted me in the belief that this was a valid option, but I now think that it was a relic of the past more than anything else.

I’m on the fence about deprecating the selector entirely, though. Even if you’ve looked at other people’s config files, I think it’s hard to gauge whether the selector is actually in use. That said, this could bring more brain to the topic, which would be a welcome addition.

Eventually I would like to use ts.el for a :timestamp selector that would look at all timestamps in an entry, not just DEADLINE, SCHEDULED, or CLOSED. ts.el needs more polishing before it is ready to be used by other projects, and that might take a while.

I’ve never used ts.el, although I know of it. I’m going to look into to see if we could optimise the code with it, and I’ll keep you posted.

By the way, I may not have time to look at this PR much further for a few weeks. I'll try to check on it if I have time, but I can't make any guarantees at the moment, as I'm going to be busy with some other things. I appreciate your patience, in advance. :) Please feel free to ping me if I forget for a while.

No problem whatsoever. I’ll probably be fairly busy on my end too, so we can mutually ping one another to remind ourselves periodically. :)

By the way, I'm leaning toward simply removing the :date group selector altogether for now, rather than trying to deprecate it with special code to check for deprecated selectors. There may be someone out there using it, but I haven't seen it "in the wild" anywhere, even though I've spent some time looking at configs on GitHub. If anyone is using it, it should be easy to replace with one of the other group selectors. And if that's too much trouble at the time, they could switch to the MELPA Stable version.

As I’ve mentioned above, I’m on the fence, but you can go for it. We can always direct them to this PR if they want the behaviour back (or to MELPA Stable, as you've mentioned).

On a related note, I’ll be putting my config files online in the near future, so feel free to explore them when they are.

Thanks for your work on this! I think it will be a nice improvement to the code.

I hope so. :)

@alphapapa
Copy link
Owner

BTW, I have seen :date in some public configs recently, so it shouldn't be removed. Deprecation might be messy, but if we want to change or remove it, I think that's the only way. I don't want to mysteriously break anyone's agenda config.

Also, I cleaned up ts.el recently and it was published on MELPA today, so we can use it now.

@zaeph
Copy link
Author

zaeph commented Aug 10, 2019

BTW, I have seen :date in some public configs recently, so it shouldn't be removed. Deprecation might be messy, but if we want to change or remove it, I think that's the only way. I don't want to mysteriously break anyone's agenda config.

I was worried about this, which is why I wanted to implement a basic setup for deprecating selectors and warning the user, and I think we could work from there.

Also, I cleaned up ts.el recently and it was published on MELPA today, so we can use it now.

Still haven't had the time to look at it. I'm brushing my Elisp skills at the moment, notably by having a go at the cl functions. I don't expect to be finished by the end of the month because it's a lot to take in, but this PR will be my next project.

@alphapapa
Copy link
Owner

BTW, to be consistent with :auto-planning and with similar code in org-ql, there should be a :planning group selector that selects based on deadline or scheduled date, and the :date selector should be deprecated. That could probably be implemented with minimal changes to existing code. But before doing that, the timestamp-based selectors should probably be reworked to use ts.el. I plan to do that eventually.

I don't want to dismiss your work, but in the context of those plans, I'm not sure where this PR fits.

@zaeph
Copy link
Author

zaeph commented Sep 1, 2019

Hello,

Feel free to close the PR if you don't think it's relevant anymore. I'm a lot busier than I expected to be, but if I get a lull, I'll work with from whichever point you've reached.

Sorry for not having addressed this project earlier. Best of luck for getting it to where you want it to be. :)

@alphapapa
Copy link
Owner

Not at all, we all do this in our free time. I'll leave this until I make the changes I mentioned, then we can decide what to do from there. Thanks.

@alphapapa
Copy link
Owner

FYI, looking at PRs again, I'd like to tag a 1.2 release soon, so I'm going to mark this for the future.

@alphapapa alphapapa added this to the Future milestone Nov 13, 2020
@zaeph
Copy link
Author

zaeph commented Nov 13, 2020

I've changed my workflow with plain timestamps, so I've had little interest in revisiting this issue. I'll try to review the PR soon-ish to settle the matter; sorry for the hold-up.

@alphapapa
Copy link
Owner

No problem. It might be best to hold off on this feature until the package is updated to use ts for the timestamp-related selectors. On the other hand, I don't know when I'll get to that. :)

I'm curious, what is your workflow with regard to timestamps now?

@zaeph
Copy link
Author

zaeph commented Nov 13, 2020

I'm curious, what is your workflow with regard to timestamps now?

Sadly, I've lost some of the gradient. I used to consider tasks which had a plain timestamps differently from those which are scheduled, with the former being prioritised in my agenda and considered less movable. Since then, I've reverted to scheduling everything, and I the gradient is somewhat covered by the priorities.

Frankly, my workflow is only a shadow of what it used to be, and I've grown quite complacent. I'll revisit it soon, and I'll include a review of this PR.

@danilevy1212
Copy link

danilevy1212 commented Sep 8, 2022

I know it has been a while but I have a huge interest in this PR since my workflow that uses a lot of timestamps. I actually use a custom selector for getting today timestamps that is badly hacked together:

  (defsubst dan/ts-doe (ts)
    "Number of days since the epoch \"1970-01-01 00:00:00\" contained in the TS
  struct."
    (days-between (format-time-string "%Y-%m-%d 00:00:00" (ts-unix ts))
                  "1970-01-01 00:00:00"))

  (defun dan/org-super-agenda--date-today (item)
    "Pick agenda ITEM whose date is today or with a daterange that includes today."
    (let ((this-marker (org-find-text-property-in-string 'org-marker item)))
      (when-let ((date (org-entry-get this-marker "TIMESTAMP")))
        (if (string-match-p "--" date)
            (cl-destructuring-bind (date-start date-end) (split-string date "--" t)
              (ts-in (ts-parse-org date-start) (ts-parse-org date-end) (ts-now))))
        (>= (dan/ts-doe (ts-now)) (dan/ts-doe (ts-parse-org date))))))

Any way this PR can be finished, @zaeph? If not, could I take over it? Not sure how though.

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.

3 participants