emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
* [RFC/PATCH] Fixes for org-timer
@ 2014-12-03  6:05 Kyle Meyer
  2014-12-03 17:16 ` Nicolas Goaziou
  0 siblings, 1 reply; 9+ messages in thread
From: Kyle Meyer @ 2014-12-03  6:05 UTC (permalink / raw)
  To: Org-mode

[-- Attachment #1: Type: text/plain, Size: 1130 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-timer.el-org-timer-Recognize-double-prefix.patch --]
[-- Type: text/x-diff, Size: 1220 bytes --]

From 1f4dd0e57401558a0cace1b421b1d37bca37fd15 Mon Sep 17 00:00:00 2001
From: Kyle Meyer <kyle@kyleam.com>
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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-org-timer.el-org-timer-start-Reset-on-pause.patch --]
[-- Type: text/x-diff, Size: 963 bytes --]

From 96b50113f8ddba96488bac8eefdc90e31df23415 Mon Sep 17 00:00:00 2001
From: Kyle Meyer <kyle@kyleam.com>
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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-org-timer.el-Reset-properly-after-countdown-timer.patch --]
[-- Type: text/x-diff, Size: 1282 bytes --]

From 9914c187c3b5ff1dcabfd15a4588e7dbb676231d Mon Sep 17 00:00:00 2001
From: Kyle Meyer <kyle@kyleam.com>
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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-org-timer.el-Isolate-commands-of-different-timers.patch --]
[-- Type: text/x-diff, Size: 5424 bytes --]

From 573fd67baaf17a5344fe0827641e5edb271dd599 Mon Sep 17 00:00:00 2001
From: Kyle Meyer <kyle@kyleam.com>
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 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.

Another issue was due to a countdown timer function trying to work with
a relative timer.

- 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.

To prevent all these issues, refuse to execute relative timer commands
if a countdown timer is running, and vice versa.
---
 lisp/org-timer.el | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/lisp/org-timer.el b/lisp/org-timer.el
index 6cecd7e..147528e 100644
--- a/lisp/org-timer.el
+++ b/lisp/org-timer.el
@@ -93,6 +93,8 @@ (defvar org-timer-done-hook nil
 (defvar org-timer-cancel-hook nil
   "Hook run before countdown timer is canceled.")
 
+(defvar org-timer-timer-is-countdown nil)
+
 ;;;###autoload
 (defun org-timer-start (&optional offset)
   "Set the starting time for the relative timer to now.
@@ -105,8 +107,12 @@ (defun org-timer-start (&optional offset)
 the amount, with the default to make the first timer string in
 the region 0:00:00."
   (interactive "P")
-  (if (equal offset '(16))
-      (call-interactively 'org-timer-change-times-in-region)
+  (cond
+   ((equal offset '(16))
+    (call-interactively 'org-timer-change-times-in-region))
+   (org-timer-timer-is-countdown
+    (user-error "Countdown timer is running.  Cancel first"))
+   (t
     (let (delta def s)
       (if (not offset)
 	  (setq org-timer-start-time (current-time))
@@ -129,7 +135,7 @@ (defun org-timer-start (&optional offset)
       (message "Timer start time set to %s, current value is %s"
 	       (format-time-string "%T" org-timer-start-time)
 	       (org-timer-secs-to-hms (or delta 0)))
-      (run-hooks 'org-timer-start-hook))))
+      (run-hooks 'org-timer-start-hook)))))
 
 (defun org-timer-pause-or-continue (&optional stop)
   "Pause or continue the relative timer.
@@ -137,7 +143,10 @@ (defun org-timer-pause-or-continue (&optional stop)
   (interactive "P")
   (cond
    (stop (org-timer-stop))
-   ((not org-timer-start-time) (error "No timer is running"))
+   (org-timer-timer-is-countdown
+    (user-error "Countdown timer is running"))
+   ((not org-timer-start-time)
+    (user-error "No relative timer is running"))
    (org-timer-pause-time
     ;; timer is paused, continue
     (setq org-timer-start-time
@@ -160,6 +169,8 @@ (defun org-timer-pause-or-continue (&optional stop)
 (defun org-timer-stop ()
   "Stop the relative timer."
   (interactive)
+  (when org-timer-timer-is-countdown
+    (user-error "Countdown timer is running"))
   (run-hooks 'org-timer-stop-hook)
   (setq org-timer-start-time nil
 	org-timer-pause-time nil)
@@ -192,7 +203,6 @@ (defun org-timer-value-string ()
 	  (org-timer-secs-to-hms
 	   (abs (floor (org-timer-seconds))))))
 
-(defvar org-timer-timer-is-countdown nil)
 (defun org-timer-seconds ()
   (if org-timer-timer-is-countdown
       (- (org-float-time org-timer-start-time)
@@ -354,8 +364,8 @@ (defvar org-timer-current-timer nil)
 (defun org-timer-cancel-timer ()
   "Cancel the current timer."
   (interactive)
-  (if (not org-timer-current-timer)
-      (message "No timer to cancel")
+  (if (not org-timer-timer-is-countdown)
+      (message "No countdown timer to cancel")
     (run-hooks 'org-timer-cancel-hook)
     (cancel-timer org-timer-current-timer)
     (setq org-timer-current-timer nil
@@ -402,6 +412,9 @@ (defun org-timer-set-timer (&optional opt)
 minutes in the Effort property, if any.  You can ignore this by
 using three `C-u' prefix arguments."
   (interactive "P")
+  (when (and org-timer-start-time
+	     (not org-timer-timer-is-countdown))
+    (user-error "Relative timer is running.  Stop first"))
   (let* ((effort-minutes (org-get-at-eol 'effort-minutes 1))
 	 (minutes (or (and (not (equal opt '(64)))
 			   effort-minutes
@@ -435,10 +448,9 @@ (defun org-timer-set-timer (&optional opt)
 		       (concat "File:" (file-name-nondirectory (buffer-file-name)))))
 		  (t (error "Not in an Org buffer"))))
 	     timer-set)
-	(if (or (and org-timer-current-timer
-		     (or (equal opt '(16))
-			 (y-or-n-p "Replace current timer? ")))
-		(not org-timer-current-timer))
+	(if (or (not org-timer-current-timer)
+		(or (equal opt '(16))
+		    (y-or-n-p "Replace current countdown timer? ")))
 	    (progn
 	      (require 'org-clock)
 	      (when org-timer-current-timer
-- 
2.1.3


^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2014-12-13 16:16 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-03  6:05 [RFC/PATCH] Fixes for org-timer Kyle Meyer
2014-12-03 17:16 ` Nicolas Goaziou
2014-12-03 18:54   ` Kyle Meyer
2014-12-03 21:39     ` Nicolas Goaziou
2014-12-07  8:11       ` Kyle Meyer
2014-12-09  9:18         ` Nicolas Goaziou
2014-12-13  9:46           ` Achim Gratz
2014-12-13 16:03             ` Nicolas Goaziou
2014-12-13 16:15               ` Achim Gratz

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).