[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [O] [PATCH] Timestamps: Handle sub-10-min ranges when updating times
From: |
Trevor Murphy |
Subject: |
Re: [O] [PATCH] Timestamps: Handle sub-10-min ranges when updating timestamps |
Date: |
Wed, 07 Aug 2013 23:31:18 -0700 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
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 <address@hidden> 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