Re: [PATCH] Fix bug assuming canonical duration units in org-agenda-format-items
Anders Johansson 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 ,
or support my work at
Re: [PATCH] Fix bug assuming canonical duration units in org-agenda-format-items
> >> 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
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
Re: [PATCH] Fix bug assuming canonical duration units in org-agenda-format-items
Hello, Anders Johansson 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
Re: [PATCH] Fix bug assuming canonical duration units in org-agenda-format-items
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.
Re: [PATCH] Fix bug assuming canonical duration units in org-agenda-format-items
Hi Anders, Did you have any thoughts on Nicolas’ comments? Nicolas Goaziou writes: > Hello, > > Anders Johansson 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
Re: [PATCH] Fix bug assuming canonical duration units in org-agenda-format-items
Hello, Anders Johansson 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
