From mboxrd@z Thu Jan 1 00:00:00 1970 From: Trevor Murphy Subject: Re: [PATCH] Timestamps: Handle sub-10-min ranges when updating timestamps Date: Wed, 07 Aug 2013 23:31:18 -0700 Message-ID: <877gfwvgll.fsf@gmail.com> References: <1375321732-26199-1-git-send-email-trevor.m.murphy@gmail.com> <87wqnxa9kj.fsf@gmail.com> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:59854) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V7JlB-0006kd-Bc for emacs-orgmode@gnu.org; Thu, 08 Aug 2013 02:31:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V7Jl3-0002zR-QG for emacs-orgmode@gnu.org; Thu, 08 Aug 2013 02:31:45 -0400 Received: from plane.gmane.org ([80.91.229.3]:52458) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V7Jl3-0002zJ-JQ for emacs-orgmode@gnu.org; Thu, 08 Aug 2013 02:31:37 -0400 Received: from list by plane.gmane.org with local (Exim 4.69) (envelope-from ) id 1V7Jl1-00016c-2g for emacs-orgmode@gnu.org; Thu, 08 Aug 2013 08:31:35 +0200 Received: from 216.87.225.193 ([216.87.225.193]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Thu, 08 Aug 2013 08:31:35 +0200 Received: from trevor.m.murphy by 216.87.225.193 with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Thu, 08 Aug 2013 08:31:35 +0200 List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org Sender: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org To: emacs-orgmode@gnu.org 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 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