[PATCH] Timestamps: Handle sub-10-min ranges when updating timestamps

[PATCH] Timestamps: Handle sub-10-min ranges when updating timestamps

[Trevor Murphy]

* lisp/org.el (org-get-compact-tod): Pad with "0" if # of minutes is
  less than 10.

TINYCHANGE
---
 lisp/org.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index 26e653f..89e023c 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -16088,9 +16088,12 @@ with the current time without prompting the user."
       (if (not t2)
 	  t1
 	(setq dh (- h2 h1) dm (- m2 m1))
-	(if (< dm 0) (setq dm (+ dm 60) dh (1- dh)))
+	(when (< dm 0) (setq dm (+ dm 60) dh (1- dh)))
 	(concat t1 "+" (number-to-string dh)
-		(if (/= 0 dm) (concat ":" (number-to-string dm))))))))
+		(when (/= 0 dm) (concat ":"
+                                        (if (< dm 10)
+                                            (concat "0" (number-to-string dm))
+                                          (number-to-string dm)))))))))
 
 (defun org-time-stamp-inactive (&optional arg)
   "Insert an inactive time stamp.
-- 
1.8.3.4

Re: [PATCH] Timestamps: Handle sub-10-min ranges when updating timestamps

[Nicolas Goaziou]

Hello,

Trevor Murphy <trevor.m.murphy@gmail.com> writes:

> * lisp/org.el (org-get-compact-tod): Pad with "0" if # of minutes is
>   less than 10.

Thanks for your patch. Would you mind providing a test-case for it? I'm
not sure about the use of `org-get-compact-tod'.

> ---
>  lisp/org.el | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/lisp/org.el b/lisp/org.el
> index 26e653f..89e023c 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -16088,9 +16088,12 @@ with the current time without prompting the user."
>        (if (not t2)
>  	  t1
>  	(setq dh (- h2 h1) dm (- m2 m1))
> -	(if (< dm 0) (setq dm (+ dm 60) dh (1- dh)))
> +	(when (< dm 0) (setq dm (+ dm 60) dh (1- dh)))

Although I agree with this change, this is not strictly necessary here.

>  	(concat t1 "+" (number-to-string dh)
> -		(if (/= 0 dm) (concat ":" (number-to-string dm))))))))
> +		(when (/= 0 dm) (concat ":"
> +                                        (if (< dm 10)
> +                                            (concat "0" (number-to-string dm))
> +                                          (number-to-string dm)))))))))

It would be better to use a 0-padded format string, e.g.,

  (and (/= 0 dm) (format ":%02d" dm))


Regards,

-- 
Nicolas Goaziou

Re: [PATCH] Timestamps: Handle sub-10-min ranges when updating timestamps

[Trevor Murphy]

Wow, thanks for parsing that, Nicolas.  I didn't realize until just now
that my git skills were so poor - I neglected the --cover-letter option,
hence the utter lack of explanatory material.

To answer your points:

Nicolas Goaziou <n.goaziou@gmail.com> writes:

>
> Thanks for your patch. Would you mind providing a test-case for it? I'm
> not sure about the use of `org-get-compact-tod'.
>

Schedule an event for today with a five-minute duration.  E.g:

* TODO test out bug in `org-schedule'
SCHEDULED: <2013-08-07 Wed 17:00-17:05>

Then hit C-c C-s (or however you have `org-schedule' bound).  With the
default setup, you'd expect to see the following prompt in the
minibuffer:

Date+time [2013-08-07]: 17:00+0:05

however what you'll get instead is:

Date+time [2013-08-07]: 17:00+0:5

The latter is not a valid time spec.  If you simply accept it, then at
least on my install org reschedules the event to:

SCHEDULED: <2013-08-07 Wed-17:00>

Which is not what I intended.  I'll add that you can get the same buggy
behavior from any command that calls `org-time-stamp' on an
already-timestamped event with <10 minute duration.

>> -	(if (< dm 0) (setq dm (+ dm 60) dh (1- dh)))
>> +	(when (< dm 0) (setq dm (+ dm 60) dh (1- dh)))
>
> Although I agree with this change, this is not strictly necessary here.

Agree.  I just couldn't resist.  Since I'll likely be rewriting this
patch anyways, I'll revert this back.

>>  	(concat t1 "+" (number-to-string dh)
>> -		(if (/= 0 dm) (concat ":" (number-to-string dm))))))))
>> +		(when (/= 0 dm) (concat ":"
>> +                                        (if (< dm 10)
>> +                                            (concat "0" (number-to-string dm))
>> +                                          (number-to-string dm)))))))))
>
> It would be better to use a 0-padded format string, e.g.,
>
>   (and (/= 0 dm) (format ":%02d" dm))

I tested that and it felt noticeably slower when I called
`org-reschedule'.  The extra `if' and `concat' did not feel slower.  I
didn't do explicit timings because of the subjective feel (also because
I'm not really sure how to do those tests yet).  That being said, I
agree with you.

If you prefer, I'll resubmit the patch without the if => when and using
the format string.  Let me know if you'd prefer I do some timing tests
on format vs if/concat.

-- 
Trevor Murphy
GnuPG Key: 0xCB06EAAF

Re: [PATCH] Timestamps: Handle sub-10-min ranges when updating timestamps

[Nicolas Goaziou]

Trevor Murphy <trevor.m.murphy@gmail.com> writes:

> Schedule an event for today with a five-minute duration.  E.g:
>
> * TODO test out bug in `org-schedule'
> SCHEDULED: <2013-08-07 Wed 17:00-17:05>
>
> Then hit C-c C-s (or however you have `org-schedule' bound).  With the
> default setup, you'd expect to see the following prompt in the
> minibuffer:
>
> Date+time [2013-08-07]: 17:00+0:05
>
> however what you'll get instead is:
>
> Date+time [2013-08-07]: 17:00+0:5
>
> The latter is not a valid time spec.  If you simply accept it, then at
> least on my install org reschedules the event to:
>
> SCHEDULED: <2013-08-07 Wed-17:00>
>
> Which is not what I intended.  I'll add that you can get the same buggy
> behavior from any command that calls `org-time-stamp' on an
> already-timestamped event with <10 minute duration.

OK. Thank you for the explanation.

> I tested that and it felt noticeably slower when I called
> `org-reschedule'.  The extra `if' and `concat' did not feel slower.  I
> didn't do explicit timings because of the subjective feel (also because
> I'm not really sure how to do those tests yet).  That being said, I
> agree with you.

I doubt the difference between the two is noticeable. Something else
happened when calling `org-reschedule'.

> If you prefer, I'll resubmit the patch without the if => when and using
> the format string.  

Please do. I'll apply it then.


Regards,

-- 
Nicolas Goaziou