[PATCH] Fix bug assuming canonical duration units in org-agenda-format-items

[PATCH] Fix bug assuming canonical duration units in org-agenda-format-items

[Anders Johansson]

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

Re: [PATCH] Fix bug assuming canonical duration units in org-agenda-format-items

[Nicolas Goaziou]

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

Re: [PATCH] Fix bug assuming canonical duration units in org-agenda-format-items

[Timothy]

[-- 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

Re: [PATCH] Fix bug assuming canonical duration units in org-agenda-format-items

[Anders Johansson]

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

[Nicolas Goaziou]

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

Re: [PATCH] Fix bug assuming canonical duration units in org-agenda-format-items

[Anders Johansson]

> >> 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

Re: [PATCH] Fix bug assuming canonical duration units in org-agenda-format-items

[Ihor Radchenko]

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>