org-duration-from-minutes was called with canonical = t, but without providing a corresponding format only using the canonical units. This broke if the user’s org-duration-format used other than the canonical units. --- lisp/org-agenda.el | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lisp/org-agenda.el b/lisp/org-agenda.el index b4e5547d7..e78ab6b72 100644 --- a/lisp/org-agenda.el +++ b/lisp/org-agenda.el @@ -6749,7 +6749,7 @@ (defun org-agenda-format-item (extra txt &optional with-level with-category tags (org-duration-from-minutes (+ (org-duration-to-minutes s1 t) org-agenda-default-appointment-duration) - nil t))) + '(("d" (special . h:mm))) t))) ;; Compute the duration (when s2 (setq duration (- (org-duration-to-minutes s2) -- 2.32.0
Hello,
Anders Johansson <mejlaandersj@gmail.com> writes:
> org-duration-from-minutes was called with canonical = t, but without
> providing a corresponding format only using the canonical units. This
> broke if the user’s org-duration-format used other than the canonical
> units.
I think a proper fix would be to change `org-duration-from-minutes' so
it removes any unknown unit from what is provided from fmt or
`org-duration-format', and defaults to (special . h:mm) if nothing is
left.
WDYT?
Regards,
--
Nicolas Goaziou
[-- Attachment #1: Type: text/plain, Size: 688 bytes --] Hi Anders, Did you have any thoughts on Nicolas’ comments? Nicolas Goaziou <mail@nicolasgoaziou.fr> writes: > Hello, > > Anders Johansson <mejlaandersj@gmail.com> writes: > >> org-duration-from-minutes was called with canonical = t, but without >> providing a corresponding format only using the canonical units. This >> broke if the user’s org-duration-format used other than the canonical >> units. > > I think a proper fix would be to change `org-duration-from-minutes’ so > it removes any unknown unit from what is provided from fmt or > `org-duration-format’, and defaults to (special . h:mm) if nothing is > left. > > WDYT? All the best, Timothy
Hi,
Oh, I sent a response but failed to send it to the list and only to
Nicolas (hard to do things right using the gmail web interface)
I include it below:
> I think a proper fix would be to change `org-duration-from-minutes' so
> it removes any unknown unit from what is provided from fmt or
> `org-duration-format', and defaults to (special . h:mm) if nothing is
> left.
>
> WDYT?
>
Perhaps. I don't understand `org-duration-from-minutes` well enough to
change it. I guess the stripping of unknown units from fmt or
`org-duration-format` would then have to compare either against
`org-duration-units` or `org-duration-canonical-units` depending on
the canonical argument. But the full logic of
org-duration-from-minutes escapes me.
I also noted that the only other invocations of
`org-duration-from-minutes` with the canonical argument is in
`org-columns--format-age` (where a format is provided).
Overall the interaction of these functions across the codebase seems
quite complex now.
Hello, Anders Johansson <mejlaandersj@gmail.com> writes: >> I think a proper fix would be to change `org-duration-from-minutes' so >> it removes any unknown unit from what is provided from fmt or >> `org-duration-format', and defaults to (special . h:mm) if nothing is >> left. >> >> WDYT? >> > Perhaps. I don't understand `org-duration-from-minutes` well enough to > change it. You don't really need to understand it. I suggest using a filter as the very first step of the function. > I guess the stripping of unknown units from fmt or > `org-duration-format` would then have to compare either against > `org-duration-units` or `org-duration-canonical-units` depending on > the canonical argument. Exactly. Regards, -- Nicolas Goaziou
> >> I think a proper fix would be to change `org-duration-from-minutes' so > >> it removes any unknown unit from what is provided from fmt or > >> `org-duration-format', and defaults to (special . h:mm) if nothing is > >> left. > >> > >> WDYT? > >> > > Perhaps. I don't understand `org-duration-from-minutes` well enough to > > change it. > > You don't really need to understand it. I suggest using a filter as the > very first step of the function. > > > I guess the stripping of unknown units from fmt or > > `org-duration-format` would then have to compare either against > > `org-duration-units` or `org-duration-canonical-units` depending on > > the canonical argument. > > Exactly. > > > Regards, > -- > Nicolas Goaziou Included is a patch for the filtering (I assumed cl-intersection was reasonable to use since cl-lib is a requirement). However, I do not think this is enough, since it can cause quite unexpected results when canonical is used without specifying the format, Hence, I do think the previous patch should also be applied. For my case, org-duration-format is (("m") ("w") ("d") (special . h:mm)). This call to org-duration-from-minutes (like in org-agenda-format-item): (org-duration-from-minutes MINUTES nil t) will then result in a full format amounting to (("d")), (since "special" is not part of the canonical units), which is hardly what is expected for the agenda. Best, Anders Johansson [PATCH] Ensure valid duration format in org-duration-from-minutes Filter out any elements of the duration format that is not in org-duration-units or org-duration-canonical-units. --- lisp/org-duration.el | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lisp/org-duration.el b/lisp/org-duration.el index e627d0936..eb4b6075f 100644 --- a/lisp/org-duration.el +++ b/lisp/org-duration.el @@ -314,13 +314,19 @@ (defun org-duration-from-minutes (minutes &optional fmt canonical) "Return duration string for a given number of MINUTES. Format duration according to `org-duration-format' or FMT, when -non-nil. +non-nil. Invalid units in the duration format are discarded. When optional argument CANONICAL is non-nil, ignore `org-duration-units' and use standard time units value. Raise an error if expected format is unknown." - (pcase (or fmt org-duration-format) + (pcase (cl-intersection + (or fmt org-duration-format) + (if canonical + org-duration-canonical-units + org-duration-units) + :key #'car + :test #'equal) (`h:mm (format "%d:%02d" (/ minutes 60) (mod minutes 60))) (`h:mm:ss -- 2.33.0
Anders Johansson <mejlaandersj@gmail.com> writes: > Included is a patch for the filtering (I assumed cl-intersection was > reasonable to use since cl-lib is a requirement). > > However, I do not think this is enough, since it can cause quite > unexpected results when canonical is used without specifying the > format, Hence, I do think the previous patch should also be applied. > > For my case, org-duration-format is (("m") ("w") ("d") (special . h:mm)). > This call to org-duration-from-minutes (like in org-agenda-format-item): > > (org-duration-from-minutes MINUTES nil t) > > will then result in a full format amounting to (("d")), (since > "special" is not part of the canonical units), which is hardly what is > expected for the agenda. The patch breaks tests and thus cannot be applied. May you please provide more details about the original problem you were trying to solve? Preferably with a reproducer. -- Ihor Radchenko // yantar92, Org mode contributor, 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>