Re: [O] [PATCH] Timestamps: Handle sub-10-min ranges when updating timestamps
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.goaz...@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: [O] [PATCH] Timestamps: Handle sub-10-min ranges when updating timestamps
Trevor Murphy trevor.m.mur...@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
Re: [O] [PATCH] Timestamps: Handle sub-10-min ranges when updating timestamps
Hello, Trevor Murphy trevor.m.mur...@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
