Re: [PATCH] Separate clocksum format for durations >= 1 day

[Toby Cubitt <tsc25@cantab.net>]

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

On Wed, Nov 14, 2012 at 05:20:14PM +0100, Toby Cubitt wrote:
> On Wed, Nov 14, 2012 at 05:09:38PM +0100, Nicolas Goaziou wrote:
> > Toby Cubitt <tsc25@cantab.net> writes:
> > 
> > > I can easily allow org-time-clocksum-fractional-format to be a list of
> > > formats. But "1d 3.4h" doesn't seem very useful to me. Probably it should
> > > work a bit differently: format the time as a fractional quantity, using
> > > the largest time unit which will give a non-zero integer part.
> > >
> > > Does that sound reasonable?
> > 
> > That's the idea, yes. Though, it will be the largest time unit _with
> > a format string_ which will give a non-zero integer part.
> 
> Yes, that's what I meant.
> 
> > > OK, but in this case I think the single-format-string option is still
> > > useful. It gives users a simpler way of customizing the format if they
> > > don't want to do anything fancy.
> > 
> > If they don't want to do anything fancy, they use the default value,
> > whatever it may be. ;) I don't mind keeping the single format string
> > option anyway.
> 
> I'll leave it in my patch. If you want to remove it for 8.0, it'll be a
> simple case of deleting some code.
> 
> > > I'll post an updated patch when I get time to make the changes.
> > 
> > Since it's for 8.0, there's no hurry. I'll wait for you to merge the two
> > patches and make subsequent changes.

Here's an updated patch. Now both org-time-clocksum-format and
org-time-clocksum-fractional-format can be plists, as discussed.

In the org-time-clocksum-format case, I made the values cons cells which
specify both a format string and a boolean. The latter indicates whether
the time component should always be included in the formatted duration,
even if its value is 0. This is needed for the hours component to
reproduce the current default format, and I figured I might as well make
it general.

I used a somewhat complex customization type in the defcustoms, instead
of a straight plist, in order to produce a better ui for the
customization interface. I'm still not completely satisfied with it.
E.g. it would be nice to get rid of the "Cons cell" tag entirely, and use
a checkbox for the boolean. But I can't figure out how to do that
(without defining new customization types/widgets, which I don't have the
patience for).

Toby
-- 
Dr T. S. Cubitt
Mathematics and Quantum Information group
Department of Mathematics
Complutense University
Madrid, Spain

email: tsc25@cantab.net
web:   www.dr-qubit.org

[-- Attachment #2: 0001-Allow-more-flexible-customization-of-clocksum-format.patch --]
[-- Type: text/x-patch, Size: 16616 bytes --]

From 639baf9c942df97e7355f402a9df38e6c9b6ef88 Mon Sep 17 00:00:00 2001
From: "Toby S. Cubitt" <tsc25@cantab.net>
Date: Sun, 11 Nov 2012 22:20:24 +0000
Subject: [PATCH] Allow more flexible customization of clocksum format

* lisp/org.el (org-time-clocksum-format, org-time-clocksum-fractional-format):
in addition to a single format string, the clocksum formats can now be
plists specifying separate formats for different time units.

* lisp/org.el (org-minutes-to-clocksum-string): new function to
replace org-minutes-to-hh:mm-string, which converts a number of
minutes to a string according to the customization options.

* lisp/org-colview.el (org-columns-number-to-string): use new
org-minutes-to-clocksum-string function to format clocksum durations.

* lisp/org-clock.el: always call new org-minutes-to-clocksum-string
function when formatting time durations, instead of calling
org-minutes-to-hh:mm-string or passing org-time-clocksum-format
directly to format.
---
 lisp/org-clock.el   |   51 +++++----------
 lisp/org-colview.el |    3 +-
 lisp/org.el         |  175 +++++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 183 insertions(+), 46 deletions(-)

diff --git a/lisp/org-clock.el b/lisp/org-clock.el
index 84eb2fd..c768491 100644
--- a/lisp/org-clock.el
+++ b/lisp/org-clock.el
@@ -556,28 +556,23 @@ pointing to it."
 If an effort estimate was defined for the current item, use
 01:30/01:50 format (clocked/estimated).
 If not, show simply the clocked time like 01:50."
-  (let* ((clocked-time (org-clock-get-clocked-time))
-	 (h (floor clocked-time 60))
-	 (m (- clocked-time (* 60 h))))
+  (let ((clocked-time (org-clock-get-clocked-time)))
     (if org-clock-effort
 	(let* ((effort-in-minutes
 		(org-duration-string-to-minutes org-clock-effort))
-	       (effort-h (floor effort-in-minutes 60))
-	       (effort-m (- effort-in-minutes (* effort-h 60)))
 	       (work-done-str
 		(org-propertize
-		 (format org-time-clocksum-format h m)
+		 (org-minutes-to-clocksum-string clocked-time)
 		 'face (if (and org-clock-task-overrun (not org-clock-task-overrun-text))
 			   'org-mode-line-clock-overrun 'org-mode-line-clock)))
-	       (effort-str (format org-time-clocksum-format effort-h effort-m))
+	       (effort-str (org-minutes-to-clocksum-string effort-in-minutes))
 	       (clockstr (org-propertize
 			  (concat  " [%s/" effort-str
 				   "] (" (replace-regexp-in-string "%" "%%" org-clock-heading) ")")
 			  'face 'org-mode-line-clock)))
 	  (format clockstr work-done-str))
-      (org-propertize (format
-		       (concat "[" org-time-clocksum-format " (%s)]")
-		       h m org-clock-heading)
+      (org-propertize (concat "[" (org-minutes-to-clocksum-string clocked-time)
+			      (format " (%s)" org-clock-heading) "]")
 		      'face 'org-mode-line-clock))))
 
 (defun org-clock-get-last-clock-out-time ()
@@ -650,7 +645,7 @@ the mode line."
 	      (setq value (- current value))
 	    (if (equal ?+ sign) (setq value (+ current value)))))
 	(setq value (max 0 value)
-	      org-clock-effort (org-minutes-to-hh:mm-string value))
+	      org-clock-effort (org-minutes-to-clocksum-string value))
 	(org-entry-put org-clock-marker "Effort" org-clock-effort)
 	(org-clock-update-mode-line)
 	(message "Effort is now %s" org-clock-effort))
@@ -1528,8 +1523,9 @@ to, overriding the existing value of `org-clock-out-switch-to-state'."
 						"\\>"))))
 		  (org-todo org-clock-out-switch-to-state))))))
 	  (force-mode-line-update)
-	  (message (concat "Clock stopped at %s after HH:MM = " org-time-clocksum-format "%s") te h m
-		   (if remove " => LINE REMOVED" ""))
+	  (message (concat "Clock stopped at %s after "
+			   (org-minutes-to-clocksum-string (+ (* 60 h) m)) "%s")
+		   te (if remove " => LINE REMOVED" ""))
           (run-hooks 'org-clock-out-hook)
 	  (unless (org-clocking-p)
 	    (org-clock-delete-current)))))))
@@ -1797,12 +1793,9 @@ Use \\[org-clock-remove-overlays] to remove the subtree times."
 	(when org-remove-highlights-with-change
 	  (org-add-hook 'before-change-functions 'org-clock-remove-overlays
 			nil 'local))))
-    (if org-time-clocksum-use-fractional
-	(message (concat "Total file time: " org-time-clocksum-fractional-format
-			 " (%d hours and %d minutes)")
-		 (/ (+ (* h 60.0) m) 60.0) h m)
-      (message (concat "Total file time: " org-time-clocksum-format
-		       " (%d hours and %d minutes)") h m h m))))
+      (message (concat "Total file time: "
+		       (org-minutes-to-clocksum-string org-clock-file-total-minutes)
+		       " (%d hours and %d minutes)") h m)))
 
 (defvar org-clock-overlays nil)
 (make-variable-buffer-local 'org-clock-overlays)
@@ -1814,9 +1807,6 @@ This creates a new overlay and stores it in `org-clock-overlays', so that it
 will be easy to remove."
   (let* ((c 60) (h (floor (/ time 60))) (m (- time (* 60 h)))
 	 (l (if level (org-get-valid-level level 0) 0))
-	 (fmt (concat "%s " (if org-time-clocksum-use-fractional
-				org-time-clocksum-fractional-format
-			      org-time-clocksum-format) "%s"))
 	 (off 0)
 	 ov tx)
     (org-move-to-column c)
@@ -1825,14 +1815,9 @@ will be easy to remove."
     (setq ov (make-overlay (point-at-bol) (point-at-eol))
     	  tx (concat (buffer-substring (point-at-bol) (point))
 		     (make-string (+ off (max 0 (- c (current-column)))) ?.)
-		     (org-add-props (if org-time-clocksum-use-fractional
-					(format fmt
-						(make-string l ?*)
-						(/ (+ (* h 60.0) m) 60.0)
-						(make-string (- 16 l) ?\ ))
-				      (format fmt
-					      (make-string l ?*) h m
-					      (make-string (- 16 l) ?\ )))
+		     (org-add-props (concat (format "%s " (make-string l ?*))
+					    (org-minutes-to-clocksum-string time)
+					    (format "%s" (make-string (- 16 l) ?\ )))
 			 (list 'face 'org-clock-overlay))
 		     ""))
     (if (not (featurep 'xemacs))
@@ -2392,7 +2377,7 @@ from the dynamic block definition."
      (if properties (make-string (length properties) ?|) "")  ; properties columns, maybe
      (concat (format org-clock-total-time-cell-format (nth 7 lwords))  "| ") ; instead of a headline
      (format org-clock-total-time-cell-format
-	     (org-minutes-to-hh:mm-string (or total-time 0))) ; the time
+	     (org-minutes-to-clocksum-string (or total-time 0))) ; the time
      "|\n")                          ; close line
 
     ;; Now iterate over the tables and insert the data
@@ -2416,7 +2401,7 @@ from the dynamic block definition."
 		     (if level-p   "| " "") ; level column, maybe
 		     (if timestamp "| " "") ; timestamp column, maybe
 		     (if properties (make-string (length properties) ?|) "")  ;properties columns, maybe
-		     (org-minutes-to-hh:mm-string (nth 1 tbl))))) ; the time
+		     (org-minutes-to-clocksum-string (nth 1 tbl))))) ; the time
 
 	  ;; Get the list of node entries and iterate over it
 	  (setq entries (nth 2 tbl))
@@ -2449,7 +2434,7 @@ from the dynamic block definition."
 	     hlc headline hlc "|"                                ; headline
 	     (make-string (min (1- ntcol) (or (- level 1))) ?|)
 					; empty fields for higher levels
-	     hlc (org-minutes-to-hh:mm-string (nth 3 entry)) hlc ; time
+	     hlc (org-minutes-to-clocksum-string (nth 3 entry)) hlc ; time
 	     "|\n"                                               ; close line
 	     )))))
     ;; When exporting subtrees or regions the region might be
diff --git a/lisp/org-colview.el b/lisp/org-colview.el
index 9d58b5f..d9afbd1 100644
--- a/lisp/org-colview.el
+++ b/lisp/org-colview.el
@@ -1058,8 +1058,7 @@ Don't set this, this is meant for dynamic scoping.")
    ((memq fmt '(estimate)) (org-estimate-print n printf))
    ((not (numberp n)) "")
    ((memq fmt '(add_times max_times min_times mean_times))
-    (let* ((h (floor n)) (m (floor (+ 0.5 (* 60 (- n h))))))
-      (format org-time-clocksum-format h m)))
+    (org-hours-to-clocksum-string n))
    ((eq fmt 'checkbox)
     (cond ((= n (floor n)) "[X]")
 	  ((> n 1.) "[-]")
diff --git a/lisp/org.el b/lisp/org.el
index 080b527..d347b95 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -2714,11 +2714,67 @@ commands, if custom time display is turned on at the time of export."
 	(concat "[" (substring f 1 -1) "]")
       f)))
 
-(defcustom org-time-clocksum-format "%d:%02d"
+(defcustom org-time-clocksum-format
+  '(:days ("%dd " . nil) :hours ("%d" . t) :minutes (":%02d" . t))
   "The format string used when creating CLOCKSUM lines.
-This is also used when org-mode generates a time duration."
+This is also used when Org mode generates a time duration.
+
+The value can be a single format string containing two
+%-sequences, which will be filled with the number of hours and
+minutes in that order.
+
+Alternatively, the value can be a plist associating any of the
+keys :years, :months, :weeks, :days, :hours or :minutes with a
+format. The time duration is formatted using only the time
+components that are needed and concatenating the results. If a
+time unit in absent, it falls back to the next smallest unit.
+
+The format values in the plist must be cons cells, containing a
+format string in the car and either t or nil in the cdr. To
+format a time component, the value is passed to the corresponding
+format string. A non-nil value in the cdr indicates that the
+corresponding time component should always be included, even if
+its value is 0.
+
+
+For example,
+
+  (:days (\"%dd\" . nil) :hours (\"%d\" . t)
+   :minutes (\":%02d\" . t))
+
+means durations longer than a day will be expressed in days,
+hours and minutes, and durations less than a day will always be
+expressed in hours and minutes (even for durations less than an
+hour).
+
+The value
+
+  (:days (\"%dd\" . nil) :minutes (\"%dm\" . nil))
+
+means durations longer than a day will be expressed in days and
+minutes, and durations less than a day will be expressed entirely
+in minutes (even for durations longer than an hour)."
   :group 'org-time
-  :type 'string)
+  :type '(choice (string :tag "Format string")
+		 (set :tag "Plist"
+		      (group :inline t (const :tag "Years" :years)
+			    (cons (string :tag "Format string")
+				  (boolean :tag "Required")))
+		      (group :inline t (const :tag "Months" :months)
+			    (cons (string :tag "Format string")
+				  (boolean :tag "Required" t)))
+		      (group :inline t (const :tag "Weeks" :weeks)
+			    (cons (string :tag "Format string")
+				  (boolean :tag "Required" t)))
+		      (group :inline t (const :tag "Days" :days)
+			    (cons (string :tag "Format string")
+				  (boolean :tag "Required" t)))
+		      (group :inline t (const :tag "Hours" :hours)
+			    (cons (string :tag "Format string")
+				  (boolean :tag "Required" t)))
+		      (group :inline t (const :tag "Minutes" :minutes)
+			    (cons (string :tag "Format string")
+				  (boolean :tag "Required" t))))))
 
 (defcustom org-time-clocksum-use-fractional nil
   "If non-nil, \\[org-clock-display] uses fractional times.
@@ -2727,10 +2783,33 @@ org-mode generates a time duration."
   :type 'boolean)
 
 (defcustom org-time-clocksum-fractional-format "%.2f"
-  "The format string used when creating CLOCKSUM lines, or when
-org-mode generates a time duration."
+  "The format string used when creating CLOCKSUM lines,
+or when Org mode generates a time duration, if
+`org-time-clocksum-use-fractional' is enabled.
+
+The value can be a single format string containing one
+%-sequence, which will be filled with the number of hours as a
+float.
+
+Alternatively, the value can be a plist associating any of the
+keys :years, :months, :weeks, :days, :hours or :minutes with a
+format string. The time duration is formatted using the largest
+time unit which gives a non-zero integer part. If all specified
+formats have zero integer part, the smallest time unit is used."
   :group 'org-time
-  :type 'string)
+  :type '(choice (string :tag "Format string")
+		 (set (group :inline t (const :tag "Years" :years)
+			    (string :tag "Format string"))
+		      (group :inline t (const :tag "Months" :months)
+			    (string :tag "Format string"))
+		      (group :inline t (const :tag "Weeks" :weeks)
+			     (string :tag "Format string"))
+		      (group :inline t (const :tag "Days" :days)
+			     (string :tag "Format string"))
+		      (group :inline t (const :tag "Hours" :hours)
+			     (string :tag "Format string"))
+		      (group :inline t (const :tag "Minutes" :minutes)
+			     (string :tag "Format string")))))
 
 (defcustom org-deadline-warning-days 14
   "No. of days before expiration during which a deadline becomes active.
@@ -16667,11 +16746,85 @@ If there is already a time stamp at the cursor position, update it."
       (org-insert-time-stamp
        (encode-time 0 0 0 (nth 1 cal-date) (car cal-date) (nth 2 cal-date))))))
 
-(defun org-minutes-to-hh:mm-string (m)
-  "Compute H:MM from a number of minutes."
-  (let ((h (/ m 60)))
-    (setq m (- m (* 60 h)))
-    (format org-time-clocksum-format h m)))
+(defun org-minutes-to-clocksum-string (m)
+  "Format number of minutes as a clocksum string.
+The format is determined by `org-time-clocksum-format',
+`org-time-clocksum-use-fractional' and
+`org-time-clocksum-fractional-format'."
+  (let ((clocksum "") fmt n)
+    ;; fractional format
+    (if org-time-clocksum-use-fractional
+	(cond
+	 ;; single format string
+	 ((stringp org-time-clocksum-fractional-format)
+	  (format org-time-clocksum-fractional-format (/ m 60.0)))
+	 ;; choice of formats for different time units
+	 ((and (setq fmt (plist-get org-time-clocksum-fractional-format :years))
+	       (> (/ m (* 365 24 60)) 0))
+	  (format fmt (/ m (* 365 24 60.0))))
+	 ((and (setq fmt (plist-get org-time-clocksum-fractional-format :months))
+	       (> (/ m (* 30 24 60)) 0))
+	  (format fmt (/ m (* 30 24 60.0))))
+	 ((and (setq fmt (plist-get org-time-clocksum-fractional-format :weeks))
+	       (> (/ m (* 7 24 60)) 0))
+	  (format fmt (/ m (* 7 24 60.0))))
+	 ((and (setq fmt (plist-get org-time-clocksum-fractional-format :days))
+	       (> (/ m (* 24 60)) 0))
+	  (format fmt (/ m (* 24 60.0))))
+	 ((and (setq fmt (plist-get org-time-clocksum-fractional-format :hours))
+	       (> (/ m 60) 0))
+	  (format fmt (/ m 60.0)))
+	 ((setq fmt (plist-get org-time-clocksum-fractional-format :minutes))
+	  (format fmt m))
+	 ;; fall back to smallest time unit with a format
+	 ((setq fmt (plist-get org-time-clocksum-fractional-format :hours))
+	  (format fmt (/ m 60.0)))
+	 ((setq fmt (plist-get org-time-clocksum-fractional-format :days))
+	  (format fmt (/ m (* 24 60.0))))
+	 ((setq fmt (plist-get org-time-clocksum-fractional-format :weeks))
+	  (format fmt (/ m (* 7 24 60.0))))
+	 ((setq fmt (plist-get org-time-clocksum-fractional-format :months))
+	  (format fmt (/ m (* 30 24 60.0))))
+	 ((setq fmt (plist-get org-time-clocksum-fractional-format :years))
+	  (format fmt (/ m (* 365 24 60.0)))))
+      ;; standard (non-fractional) format
+      (and (setq fmt (plist-get org-time-clocksum-format :years))
+	   (or (> (setq n (/ m (* 365 24 60))) 0)
+	       (eq (cdr fmt) t))
+	   (setq clocksum (concat clocksum (format (car fmt) n))
+		 m (- m (* n 365 24 60))))
+      (and (setq fmt (plist-get org-time-clocksum-format :months))
+	   (or (> (setq n (/ m (* 30 24 60))) 0)
+	       (eq (cdr fmt) t))
+	   (setq clocksum (concat clocksum (format (car fmt) n))
+		 m (- m (* n 30 24 60))))
+      (and (setq fmt (plist-get org-time-clocksum-format :weeks))
+	   (or (> (setq n (/ m (* 7 24 60))) 0)
+	       (eq (cdr fmt) t))
+	   (setq clocksum (concat clocksum (format (car fmt) n))
+		 m (- m (* n 7 24 60))))
+      (and (setq fmt (plist-get org-time-clocksum-format :days))
+	   (or (> (setq n (/ m (* 24 60))) 0)
+	       (eq (cdr fmt) t))
+	   (setq clocksum (concat clocksum (format (car fmt) n))
+		 m (- m (* n 24 60))))
+      (and (setq fmt (plist-get org-time-clocksum-format :hours))
+	   (or (> (setq n (/ m 60)) 0)
+	       (eq (cdr fmt) t))
+	   (setq clocksum (concat clocksum (format (car fmt) n))
+		 m (- m (* n 60))))
+      (and (setq fmt (plist-get org-time-clocksum-format :minutes))
+	   (or (> m 0) (eq (cdr fmt) t))
+	   (setq clocksum (concat clocksum (format (car fmt) m))))
+      ;; return formatted time duration
+      clocksum)))
+
+(defalias 'org-minutes-to-hh:mm-string 'org-minutes-to-clocksum-string)
+(make-obsolete 'org-minutes-to-hh:mm-string 'org-minutes-to-clocksum-string
+	       "org-mode version 7.9.3")
+
+(defun org-hours-to-clocksum-string (n)
+  (org-minutes-to-clocksum-string (* n 60)))
 
 (defun org-hh:mm-string-to-minutes (s)
   "Convert a string H:MM to a number of minutes.
-- 
1.7.8.6