Re: [RFC PATCH] specify a time, not number of minutes to keep, with org-resolve-clock

[Dan Drake <dan.drake@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2523 bytes --]

Thank you, Nicolas, for the review of my first patch. I've updated my code
and have the new patch attached.

I didn't inline the "time to minutes to keep function"; yes, that function
is very short, but the function name makes the intent/purpose of the code
obvious, and the org-clock-resolve a bit more readable. If it's important
to inline that -- performance concerns about the function call overhead,
accepting org-mode/emacs coding style, etc. -- I'm willing to do that.

I added an entry to ORG-NEWS for version 9.4.

I didn't write any tests, as the change seems so simple, and UI-focused.
But again, if that's a dealbreaker, I can work on doing that.

Further comments welcome.

On Fri, Jan 24, 2020 at 5:31 PM Nicolas Goaziou <mail@nicolasgoaziou.fr>
wrote:

> Hello,
>
> Dan Drake <dan.drake@gmail.com> writes:
>
> > I asked about a way to specify a time when using org-resolve-clock
> instead
> > of a number of minutes:
> >
> > https://lists.gnu.org/archive/html/emacs-orgmode/2020-01/msg00010.html
> >
> > I've implemented this myself and a patch is attached. Comments welcome --
> > my change works, but I'm not sure about coding style, and right now
> there's
> > no error checking.
>
> Thank you. Some comments follow.
>
> > I marked the patch as a tiny change, but it does add a new menu option
> and
> > behavior to org-resolve-clock, so there may be an argument that it's not,
> > from a user perspective, a "tiny change", but code-wise it's quite
> simple:
> > the core logic really isn't more than "ask the user for a time and
> > subtract".
>
> TINYCHANGE is only about the number of non-trivial lines of code in your
> patch (15 or so).
>
> > +(defun time-to-mins-to-keep (start-time)
> > +  "Asks the user for a time and returns the number of minutes
> > +from START-TIME to that time."
> > +  (floor (/ (float-time
> > +          (time-subtract (org-read-date t t) start-time)) 60)))
>
> The name of the function is wrong, it should start with "org-clock".
> Also, docstring's first line must contain a full sentence.
>
> Anyway, this is used only once in your patch, I suggest to inline in
> instead.
> > +       (or (and (memq ch '(?k ?K))
> > +                (read-number "Keep how many minutes? " default))
> > +           (and (memq ch '(?t ?T))
> > +                (time-to-mins-to-keep last-valid))))
>
> See above about inlining.
>
> Would you mind adding an ORG-NEWS entry, and, if possible, writing
> a test?
>
> Regards,
>
> --
> Nicolas Goaziou
>


-- 
Ceci n'est pas une .signature.

[-- Attachment #1.2: Type: text/html, Size: 3541 bytes --]

[-- Attachment #2: 0001-org-clock.el-add-t-option-to-org-clock-resolve.patch --]
[-- Type: text/x-patch, Size: 5400 bytes --]

From 52d53de5f67e9ba16b6efe91ac79b370d89c9fea Mon Sep 17 00:00:00 2001
From: Dan Drake <dan.drake@gmail.com>
Date: Sun, 19 Jan 2020 08:24:12 -0600
Subject: [PATCH] org-clock.el: add `t' option to org-clock-resolve

* org-clock.el (org-clock-resolve): add `t' option. This works just like
`k', but asks the user to specify a time, instead of a number of
minutes.

Often when you are interrupted at a task and get back to it, you know
what time the interruption happened. This option makes it easy to tell
org-resolve-clocks about that. For example, say you clocked into task A
at, say, 9:37:

    * original task A
      :LOGBOOK:
      CLOCK: [2020-01-21 Mon 09:37]
      :END:

While working on task A, you get a phone call. When the call is done,
you'd like to update your time logging to reflect the phone call. Your
phone says the call was at 11:09.

With C-c C-x C-z, you can use the `K' option, but you need to figure out
the number of minutes to keep. It's easier to look at the phone, or to
mentally note the time when an interruption starts. With the new option,
you can select `T', and just specify a time of 11:09. The state is now:

    * original task A
      :LOGBOOK:
      CLOCK: [2020-01-21 Mon 09:37]--[2020-01-21 Mon 11:09] => 1:32
      :END:

You add the phone call to your org buffer and do C-c C-x C-i to clock
in. Org asks you to start the time from when the previous task ended,
you say yes, and the state is now:

    * original task A
      :LOGBOOK:
      CLOCK: [2020-01-21 Mon 09:37]--[2020-01-21 Mon 11:09] => 1:32
      :END:
    * task B, phone call
      :LOGBOOK:
      CLOCK: [2020-01-21 Mon 11:09]
      :END:

At this point, you can clock back into task A, or any other task.

The key feature here is to be able to just type in a time -- in any
format accepted by org-read-date -- instead of specifying a number of
minutes.

TINYCHANGE
---
 etc/ORG-NEWS      |  5 +++++
 lisp/org-clock.el | 22 ++++++++++++++++------
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 67c3ca2ed..00c1f453a 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -35,6 +35,11 @@ value in call to =java=.
 After editing a source block, Org will restore the window layout when
 ~org-src-window-setup~ is set to a value that modifies the layout.
 
+*** New option to resolve open clock at a provided time
+~org-resolve-clocks~ now has a `t' option, which works just like the
+`k' option, but the user specifies a time of day, not a number of
+minutes.
+
 ** New functions
 *** ~org-columns-toggle-or-columns-quit~
 =<C-c C-c>= bound to ~org-columns-toggle-or-columns-quit~ replaces the
diff --git a/lisp/org-clock.el b/lisp/org-clock.el
index 06df2d497..04e17696b 100644
--- a/lisp/org-clock.el
+++ b/lisp/org-clock.el
@@ -986,6 +986,11 @@ CLOCK is a cons cell of the form (MARKER START-TIME)."
 		     (org-flag-drawer nil element))
 		   (throw 'exit nil)))))))))))
 
+(defun org-clock-time-to-mins-to-keep (start-time)
+  "Ask the user for a time and return the number of minutes from START-TIME to that time."
+  (floor (/ (float-time
+	     (time-subtract (org-read-date t t) start-time)) 60)))
+
 (defun org-clock-resolve (clock &optional prompt-fn last-valid fail-quietly)
   "Resolve an open Org clock.
 An open clock was found, with `dangling' possibly being non-nil.
@@ -1022,6 +1027,9 @@ k/K      Keep X minutes of the idle time (default is all).  If this
          that many minutes after the time that idling began, and then
          clocked back in at the present time.
 
+t/T      Like `k', but will ask you to specify a time, instead of a
+         number of minutes.
+
 g/G      Indicate that you \"got back\" X minutes ago.  This is quite
          different from `k': it clocks you out from the beginning of
          the idle period and clock you back in X minutes ago.
@@ -1041,19 +1049,21 @@ to be CLOCKED OUT."))))
 		(while (or (null char-pressed)
 			   (and (not (memq char-pressed
 					   '(?k ?K ?g ?G ?s ?S ?C
-						?j ?J ?i ?q)))
+						?j ?J ?i ?q ?t ?T)))
 				(or (ding) t)))
 		  (setq char-pressed
 			(read-char (concat (funcall prompt-fn clock)
-					   " [jkKgGSscCiq]? ")
+					   " [jkKtTgGSscCiq]? ")
 				   nil 45)))
 		(and (not (memq char-pressed '(?i ?q))) char-pressed)))))
 	 (default
 	   (floor (org-time-convert-to-integer (org-time-since last-valid))
 		  60))
 	 (keep
-	  (and (memq ch '(?k ?K))
-	       (read-number "Keep how many minutes? " default)))
+	  (or (and (memq ch '(?k ?K))
+		   (read-number "Keep how many minutes? " default))
+	      (and (memq ch '(?t ?T))
+		   (org-clock-time-to-mins-to-keep last-valid))))
 	 (gotback
 	  (and (memq ch '(?g ?G))
 	       (read-number "Got back how many minutes ago? " default)))
@@ -1068,7 +1078,7 @@ to be CLOCKED OUT."))))
 	  (org-clock-resolve-clock clock 'now nil t nil fail-quietly))
       (org-clock-jump-to-current-clock clock))
      ((or (null ch)
-	  (not (memq ch '(?k ?K ?g ?G ?s ?S ?C))))
+	  (not (memq ch '(?k ?K ?g ?G ?s ?S ?C ?t ?T))))
       (message ""))
      (t
       (org-clock-resolve-clock
@@ -1092,7 +1102,7 @@ to be CLOCKED OUT."))))
 	      (t
 	       (error "Unexpected, please report this as a bug")))
        (and gotback last-valid)
-       (memq ch '(?K ?G ?S))
+       (memq ch '(?K ?G ?S ?T))
        (and start-over
 	    (not (memq ch '(?K ?G ?S ?C))))
        fail-quietly)))))
-- 
2.17.1