Morgan Smith <[email protected]> writes: > Tests pass after each patch on emacs 30.2. > Tests pass after final patch on emacs 28 and 29. > Tests pass after final patch with TZ set to UTC, Europe/Istanbul, and > America/New_York. > ... > Subject: [PATCH 2/3] Fix agenda sorting based on multiple types of timestamps > > Previously, attempting to sort on multiple types of timestamps > (eg. SCHEDULED and DEADLINE) would result in only sorting on one of > them. > > * lisp/org-agenda.el (org-cmp-ts): Try harder to get a timestamp > (org-agenda-entry-get-agenda-timestamp): Add comment explaining the > functions shortcomings. > * testing/lisp/test-org-agenda.el (test-org-agenda/sorting/time): Test > now passes. > (cl-flet ((get-timestamp (entry) > (or (and (string-match type (or (get-text-property 1 'type > entry) "")) > (get-text-property 1 'ts-date entry)) > + (when-let* ((epom (get-text-property 1 'org-hd-marker > entry)) > + ;; Abort if we are in a diary buffer > + (epom (org-with-base-buffer (marker-buffer > epom) > + (and (derived-mode-p 'org-mode) > + (org-element-at-point epom)))) > + (timestamp > + (cond > + ((string-equal type "") > + (or (org-entry-get epom "SCHEDULED") > + (org-entry-get epom "DEADLINE") > + (org-entry-get epom "TIMESTAMP") > + (org-entry-get epom "TIMESTAMP_IA"))) > + ((string-equal type "scheduled") > (org-entry-get epom "SCHEDULED")) > + ((string-equal type "deadline") > (org-entry-get epom "DEADLINE")) > + ((string-equal type "timestamp_ia") > (org-entry-get epom "TIMESTAMP_IA")) > + ((string-equal type "timestamp") > (org-entry-get epom "TIMESTAMP"))))) > + (org-time-string-to-absolute timestamp))
I am afraid that this is a wrong approach that will backfire given the org-agenda design. Here are the issues I can foresee with this patch: 1. TIMESTAMP and TIMESTAMP_IA will always collect the first timestamp in the entry. But entries may have multiple timestamps 2. All types of timestamps may contain a repeater. Usually, agenda handles repeaters specially, looking for the closest repeater date. Your approach will fail to account for that. 3. There is very special handling of time in agenda - you can put time directly into the heading, and it will apply to timestamps. I suspect that those aspects may be still broken with your patch. 4. What you are doing is copying part of the logic from `org-agenda-entry-get-agenda-timestamp', which creates code duplication. Thinking about this bug, I am afraid that a more substantial re-design will be necessary in agenda (probably, need to be postponed after ongoing refactoring). Fixing straightforward cases is possible, and your patch does that, but more complex scenarios (as with repeaters and multiple timestamps) may need to be discussed. I suspect that we will need to re-design agenda to account for everything without missing edge cases. -- Ihor Radchenko // yantar92, Org mode maintainer, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92>
