Re: [O] [RFC/PATCH] Fixes for org-timer
Nicolas Goaziou writes: >> The new tests don't work on Emacs23, mainly due to the use of cl-letf. > > I fixed the `cl-letf' issue, thank you. Is there anything else to do? Not with respect to this change, the tests are now all passing. However, Emacs master seems to have changed the signature for call-process-shell-command, so Org might need to work around this with a new compatibility macro. In toplevel form: org-clock.el:1094:13:Warning: call-process-shell-command called with 6 arguments, but accepts only 1-4 Regards, Achim. -- +<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+ Wavetables for the Terratec KOMPLEXER: http://Synth.Stromeko.net/Downloads.html#KomplexerWaves
Re: [O] [RFC/PATCH] Fixes for org-timer
Hello, Achim Gratz writes: > The new tests don't work on Emacs23, mainly due to the use of cl-letf. I fixed the `cl-letf' issue, thank you. Is there anything else to do? Regards, -- Nicolas Goaziou
Re: [O] [RFC/PATCH] Fixes for org-timer
Nicolas Goaziou writes: > Applied (with a minor change in `org-timer--get-timer-title'). Thank you. The new tests don't work on Emacs23, mainly due to the use of cl-letf. Regards, Achim. -- +<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+ Waldorf MIDI Implementation & additional documentation: http://Synth.Stromeko.net/Downloads.html#WaldorfDocs
Re: [O] [RFC/PATCH] Fixes for org-timer
Kyle Meyer writes: > I've attached updated patches. Applied (with a minor change in `org-timer--get-timer-title'). Thank you. Regards,
Re: [O] [RFC/PATCH] Fixes for org-timer
Nicolas Goaziou wrote:
> Kyle Meyer writes:
>
>> Do you prefer to add fixes for the current design, or just move
>> directly to merging the two APIs?
>
> The latter sounds better.
I've attached updated patches.
>From da0454ea110de0b4effb59ec220d8e15960fbf84 Mon Sep 17 00:00:00 2001
From: Kyle Meyer
Date: Mon, 1 Dec 2014 02:02:19 -0500
Subject: [PATCH 1/2] org-timer.el (org-timer): Recognize double prefix
* lisp/org-timer.el (org-timer): Follow the behavior described in the
docstring for a double prefix argument.
---
lisp/org-timer.el | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/lisp/org-timer.el b/lisp/org-timer.el
index d02362c..c9710ca 100644
--- a/lisp/org-timer.el
+++ b/lisp/org-timer.el
@@ -177,11 +177,13 @@ (defun org-timer (&optional restart no-insert-p)
If NO-INSERT-P is non-nil, return the string instead of inserting
it in the buffer."
(interactive "P")
- (when (or (equal restart '(4)) (not org-timer-start-time))
-(org-timer-start))
- (if no-insert-p
- (org-timer-value-string)
-(insert (org-timer-value-string
+ (if (equal restart '(16))
+ (org-timer-start restart)
+(when (or (equal restart '(4)) (not org-timer-start-time))
+ (org-timer-start))
+(if no-insert-p
+ (org-timer-value-string)
+ (insert (org-timer-value-string)
(defun org-timer-value-string ()
"Set the timer string."
--
2.1.3
>From 981cee236e91cd40a006eb5eef43095e449b6cc9 Mon Sep 17 00:00:00 2001
From: Kyle Meyer
Date: Sun, 7 Dec 2014 02:24:54 -0500
Subject: [PATCH 2/2] org-timer.el: Merge API for the two timers
* lisp/org-timer.el (org-timer-stop): Support countdown timers in addition
to relative timers.
* lisp/org-timer.el (org-timer-cancel-timer): Remove function.
* lisp/org-timer.el (org-timer-pause-or-continue): Support countdown
timers in addition to relative timers.
* testing/lisp/test-org-timer.el: New file.
* doc/org.texi: Merge relative and countdown timer nodes.
Several previous issues are fixed with these changes.
- org-timer-set-timer and org-timer-cancel-timer did not reset
org-timer-start-time after countdown completed.
- Because org-timer-start did not return org-timer-pause-time to nil,
the modeline remained stuck at the paused time.
- When org-timer-start was called with a countdown timer, the modeline
was updated for the new relative timer, but the countdown timer
remained scheduled.
- When org-timer-pause-or-continue was called with a countdown timer
running, the modeline was put in a paused state, but the countdown
timer remained scheduled.
- When org-timer-stop was called with a countdown timer running, the
timer was removed from the modeline, but the countdown timer remained
scheduled.
- When org-timer-set-timer was called with a paused relative timer, the
relative timer was not reset properly (org-timer-pause-time was still
non-nil) and the modeline remained in the paused state of the relative
timer, even though the countdown timer was scheduled with
run-with-timer.
- Running org-timer-set-timer at the beginning of an empty buffer
resulted in an args-out-of-range error (due to the org-get-at-eol
call).
---
doc/org.texi | 84 ++---
etc/ORG-NEWS | 5 +
lisp/org-timer.el | 207 ++-
testing/lisp/test-org-timer.el | 273 +
4 files changed, 438 insertions(+), 131 deletions(-)
create mode 100644 testing/lisp/test-org-timer.el
diff --git a/doc/org.texi b/doc/org.texi
index f9436b3..f7444ce 100644
--- a/doc/org.texi
+++ b/doc/org.texi
@@ -462,8 +462,7 @@ Dates and times
* Deadlines and scheduling::Planning your work
* Clocking work time:: Tracking how long you spend on a task
* Effort estimates::Planning work effort in advance
-* Relative timer:: Notes with a running timer
-* Countdown timer:: Starting a countdown timer for a task
+* Timers:: Notes with a running timer
Creating timestamps
@@ -5790,8 +5789,7 @@ is used in a much wider sense.
* Deadlines and scheduling::Planning your work
* Clocking work time:: Tracking how long you spend on a task
* Effort estimates::Planning work effort in advance
-* Relative timer:: Notes with a running timer
-* Countdown timer:: Starting a countdown timer for a task
+* Timers:: Notes with a running timer
@end menu
@@ -6794,60 +6792,56 @@ with the @kbd{/} key in the agenda (@pxref{Agenda commands}). If you have
these estimates defined consistently, two or three key presses will narrow
down the list to stuff that fits into an available time slot.
-@node Relative timer
-@section Taking notes with a relative timer
+@node Timers
+@section Taking notes with a timer
@cindex relative timer
+@cindex countdown timer
+@kindex ;
+
+Org provides provides two types of t
Re: [O] [RFC/PATCH] Fixes for org-timer
Kyle Meyer writes: > Do you prefer to add fixes for the current design, or just move directly > to merging the two APIs? The latter sounds better. >> Also, some comments at the beginning of the library could help. >> >> AFAIK, there are no tests for timers. Adding a new "test-timer.el" with >> a few tests would be nice, if you have some spare time and energy. > > Yes, both of those would be good to have, and I'm happy to add them. > > Thanks for your comments. Thank you for your work. Regards,
Re: [O] [RFC/PATCH] Fixes for org-timer
Nicolas Goaziou wrote: [...] >> However, I'd actually be in favor of removing this functionality >> from org-timer (and org-timer-start) since it's already available >> directly through the interactive command >> org-timer-change-times-in-region. > > It saves a keybinding (`org-timer' has one, but not > `org-timer-change-times-in-region'). Fair enough (I don't think it's likely to be used regularly, so I wasn't giving that much weight). [...] >> Perhaps it would alsbo be helpful to make it clearer which >> commands belong to the two different timers. This is clear in the >> manual (the relative and countdown timer have different >> documentation pages), but what do you think about renaming >> countdown timer commands to have 'countdown' after 'org-timer' >> (e.g., org-timer-countdown-set-timer)? > > When I fixed a bug there recently, I was indeed surprised the code was > very confusing. For example, `org-timer-stop' is for relative timers > whereas `org-timer-cancel' is for countdown ones. Agreed. I initially went there hunting one bug. > Instead of renaming, I think we could make both timer types use the same > API (minus, maybe, start functions). Yes, I think this is a good way to go. As you mention, the start functions (org-timer, org-timer-start, and org-timer-set-timer) would probably be difficult to merge given the arguments they take. org-timer-stop and org-timer-cancel should be simple to merge. Currently, there is no function to pause the countdown timer, so I'll need to teach org-timer-pause-or-continue how to do that. Do you prefer to add fixes for the current design, or just move directly to merging the two APIs? > Also, some comments at the beginning of the library could help. > > AFAIK, there are no tests for timers. Adding a new "test-timer.el" with > a few tests would be nice, if you have some spare time and energy. Yes, both of those would be good to have, and I'm happy to add them. Thanks for your comments. -- Kyle
Re: [O] [RFC/PATCH] Fixes for org-timer
Hello, Kyle Meyer writes: > I've attached a few patches for org-timer.el. Thank you. > Some additional comments on two of the patches: > > [PATCH 1/4] org-timer.el (org-timer): Recognize double prefix > > This patch makes org-timer behave as described in the docstring when > it is given a double prefix argument. However, I'd actually be in > favor of removing this functionality from org-timer (and > org-timer-start) since it's already available directly through the > interactive command org-timer-change-times-in-region. It saves a keybinding (`org-timer' has one, but not `org-timer-change-times-in-region'). > [PATCH 4/4] org-timer.el: Isolate commands of different timers > > This patch fixes several issues that were caused by executing a > relative timer command when the countdown timer was running, or vice > versa (see log message for details). Great. > Perhaps it would alsbo be helpful to make it clearer which > commands belong to the two different timers. This is clear in the > manual (the relative and countdown timer have different > documentation pages), but what do you think about renaming > countdown timer commands to have 'countdown' after 'org-timer' > (e.g., org-timer-countdown-set-timer)? When I fixed a bug there recently, I was indeed surprised the code was very confusing. For example, `org-timer-stop' is for relative timers whereas `org-timer-cancel' is for countdown ones. Instead of renaming, I think we could make both timer types use the same API (minus, maybe, start functions). Also, some comments at the beginning of the library could help. AFAIK, there are no tests for timers. Adding a new "test-timer.el" with a few tests would be nice, if you have some spare time and energy. WDYT? Regards, -- Nicolas Goaziou
[O] [RFC/PATCH] Fixes for org-timer
Hello, I've attached a few patches for org-timer.el. Some additional comments on two of the patches: [PATCH 1/4] org-timer.el (org-timer): Recognize double prefix This patch makes org-timer behave as described in the docstring when it is given a double prefix argument. However, I'd actually be in favor of removing this functionality from org-timer (and org-timer-start) since it's already available directly through the interactive command org-timer-change-times-in-region. [PATCH 4/4] org-timer.el: Isolate commands of different timers This patch fixes several issues that were caused by executing a relative timer command when the countdown timer was running, or vice versa (see log message for details). Perhaps it would also be helpful to make it clearer which commands belong to the two different timers. This is clear in the manual (the relative and countdown timer have different documentation pages), but what do you think about renaming countdown timer commands to have 'countdown' after 'org-timer' (e.g., org-timer-countdown-set-timer)? Thanks. -- Kyle >From 1f4dd0e57401558a0cace1b421b1d37bca37fd15 Mon Sep 17 00:00:00 2001 From: Kyle Meyer Date: Mon, 1 Dec 2014 02:02:19 -0500 Subject: [PATCH 1/4] org-timer.el (org-timer): Recognize double prefix * lisp/org-timer.el (org-timer): Follow the behavior described in the docstring for a double prefix argument. --- lisp/org-timer.el | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lisp/org-timer.el b/lisp/org-timer.el index d02362c..c9710ca 100644 --- a/lisp/org-timer.el +++ b/lisp/org-timer.el @@ -177,11 +177,13 @@ (defun org-timer (&optional restart no-insert-p) If NO-INSERT-P is non-nil, return the string instead of inserting it in the buffer." (interactive "P") - (when (or (equal restart '(4)) (not org-timer-start-time)) -(org-timer-start)) - (if no-insert-p - (org-timer-value-string) -(insert (org-timer-value-string + (if (equal restart '(16)) + (org-timer-start restart) +(when (or (equal restart '(4)) (not org-timer-start-time)) + (org-timer-start)) +(if no-insert-p + (org-timer-value-string) + (insert (org-timer-value-string) (defun org-timer-value-string () "Set the timer string." -- 2.1.3 >From 96b50113f8ddba96488bac8eefdc90e31df23415 Mon Sep 17 00:00:00 2001 From: Kyle Meyer Date: Tue, 2 Dec 2014 22:05:08 -0500 Subject: [PATCH 2/4] org-timer.el (org-timer-start): Reset on pause * lisp/org-timer.el (org-timer-start): Reset to correct state when called with a paused timer. Since org-timer-pause-time was not returned to nil, the modeline remained stuck at the paused time. --- lisp/org-timer.el | 1 + 1 file changed, 1 insertion(+) diff --git a/lisp/org-timer.el b/lisp/org-timer.el index c9710ca..1c4153c 100644 --- a/lisp/org-timer.el +++ b/lisp/org-timer.el @@ -124,6 +124,7 @@ (defun org-timer-start (&optional offset) (setq org-timer-start-time (seconds-to-time (- (org-float-time) delta + (setq org-timer-pause-time nil) (org-timer-set-mode-line 'on) (message "Timer start time set to %s, current value is %s" (format-time-string "%T" org-timer-start-time) -- 2.1.3 >From 9914c187c3b5ff1dcabfd15a4588e7dbb676231d Mon Sep 17 00:00:00 2001 From: Kyle Meyer Date: Tue, 2 Dec 2014 23:35:49 -0500 Subject: [PATCH 3/4] org-timer.el: Reset properly after countdown timer * lisp/org-timer.el (org-timer-set-timer): Reset org-timer-start-time after countdown completes. * lisp/org-timer.el (org-timer-cancel-timer): Reset org-timer-start-time when canceling countdown timer. --- lisp/org-timer.el | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lisp/org-timer.el b/lisp/org-timer.el index 1c4153c..6cecd7e 100644 --- a/lisp/org-timer.el +++ b/lisp/org-timer.el @@ -359,6 +359,7 @@ (defun org-timer-cancel-timer () (run-hooks 'org-timer-cancel-hook) (cancel-timer org-timer-current-timer) (setq org-timer-current-timer nil + org-timer-start-time nil org-timer-timer-is-countdown nil) (org-timer-set-mode-line 'off) (message "Last timer canceled"))) @@ -446,6 +447,7 @@ (defun org-timer-set-timer (&optional opt) (run-with-timer secs nil `(lambda () (setq org-timer-current-timer nil) + (setq org-timer-start-time nil) (org-notify ,(format "%s: time out" hl) ,org-clock-sound) (setq org-timer-timer-is-countdown nil) (org-timer-set-mode-line 'off) -- 2.1.3 >From 573fd67baaf17a5344fe0827641e5edb271dd599 Mon Sep 17 00:00:00 2001 From: Kyle Meyer Date: Tue, 2 Dec 2014 01:34:38 -0500 Subject: [PATCH 4/4] org-timer.el: Isolate commands of different timers * lisp/org-timer.el: Fix timer bugs resulting from crosstalk between commands for relative and countdown timers. Several issues were caused by relative timer functions trying to work with countdown timers. - When org-timer-start was called with a cou
