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

[Toby Cubitt <tsc25@cantab.net>]

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

On Sat, Nov 17, 2012 at 09:48:09AM +0100, Nicolas Goaziou wrote:
> Toby Cubitt <tsc25@cantab.net> writes:
> 
> > Here's an updated patch. Now both org-time-clocksum-format and
> > org-time-clocksum-fractional-format can be plists, as discussed.
> 
> That was quick. Thank you.
> 
> > 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 understand. It is a necessary evil. Though, instead of asking for cons
> cells, maybe the boolean could be provided as another property. I.e.
> 
>     '(:hour "..." :persistent-hour t)
> 
> would be a replacement for:
> 
>     '(:hour ("..." . t))
> 
> And, better,
> 
>     '(:hour "...")
> 
> would the become a replacement for
> 
>     '(:hour ("..." . nil))
> 
> What do you think about it? The name of the property is only a
> suggestion.

Good idea. I agree, additional keys are cleaner than cons cells.

> > 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).
> 
> The advantage of the method above it that it would /de facto/ get rid of
> the "Cons cell" tag.

I've replaced the cons cells with additional plist properties, as you
suggested. The resulting customization ui still isn't wonderful in my
opinion. But it does the job, and I'm not sure how much scope there is
for improving it further. If you see a way, by all means feel free to
make the changes yourself. I really don't mind what format you go with
for org-time-clocksum-format, as long as it supports the new formatting
features implemented in the patch.

> > +		     (org-add-props (concat (format "%s " (make-string l ?*))
> > +					    (org-minutes-to-clocksum-string time)
> > +					    (format "%s" (make-string (- 16 l) ?\ )))
> 
> You forgot to change that.

Ooops. Fixed in the attached version.

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: 17260 bytes --]

From 6e87864c125676093d8072111519c37cf9dd126c 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         |  190 ++++++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 198 insertions(+), 46 deletions(-)

diff --git a/lisp/org-clock.el b/lisp/org-clock.el
index 84eb2fd..54e4018 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 (make-string l ?*) " "
+					    (org-minutes-to-clocksum-string time)
+					    (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..b43de9d 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -2714,11 +2714,79 @@ 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 " :hours "%d" :require-hours t :minutes ":%02d" :require-minutes 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
+format strings. 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 keys :require-years, :require-months, :require-days,
+:require-weeks, :require-hours, minutes-required are also
+meaningful. A non-nil value for these keys 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)
+			     (string :tag "Format string"))
+		      (group :inline t
+			     (const :tag "Always show years" :require-years)
+			     (const t))
+		      (group :inline t (const :tag "Months" :months)
+			     (string :tag "Format string"))
+		      (group :inline t
+			     (const :tag "Always show months" :require-months)
+			     (const t))
+		      (group :inline t (const :tag "Weeks" :weeks)
+			     (string :tag "Format string"))
+		      (group :inline t
+			     (const :tag "Always show weeks" :require-weeks)
+			     (const t))
+		      (group :inline t (const :tag "Days" :days)
+			     (string :tag "Format string"))
+		      (group :inline t
+			     (const :tag "Always show days" :require-days)
+			     (const t))
+		      (group :inline t (const :tag "Hours" :hours)
+			     (string :tag "Format string"))
+		      (group :inline t
+			     (const :tag "Always show hours" :require-hours)
+			     (const t))
+		      (group :inline t (const :tag "Minutes" :minutes)
+			     (string :tag "Format string"))
+		      (group :inline t
+			     (const :tag "Always show minutes" :require-minutes)
+			     (const t))
+		      )))
 
 (defcustom org-time-clocksum-use-fractional nil
   "If non-nil, \\[org-clock-display] uses fractional times.
@@ -2727,10 +2795,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 +16758,88 @@ 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 fractional 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, with single format string
+      (if (stringp org-time-clocksum-format)
+	  (format org-time-clocksum-format (setq n (/ m 60)) (- m (* 60 n)))
+	;; separate formats components
+	(and (setq fmt (plist-get org-time-clocksum-format :years))
+	     (or (> (setq n (/ m (* 365 24 60))) 0)
+		 (plist-get org-time-clocksum-format :require-years))
+	     (setq clocksum (concat clocksum (format 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)
+		 (plist-get org-time-clocksum-format :require-months))
+	     (setq clocksum (concat clocksum (format 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)
+		 (plist-get org-time-clocksum-format :require-weeks))
+	     (setq clocksum (concat clocksum (format 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)
+		 (plist-get org-time-clocksum-format :require-days))
+	     (setq clocksum (concat clocksum (format fmt n))
+		   m (- m (* n 24 60))))
+	(and (setq fmt (plist-get org-time-clocksum-format :hours))
+	     (or (> (setq n (/ m 60)) 0)
+		 (plist-get org-time-clocksum-format :require-hours))
+	     (setq clocksum (concat clocksum (format fmt n))
+		   m (- m (* n 60))))
+	(and (setq fmt (plist-get org-time-clocksum-format :minutes))
+	     (or (> m 0) (plist-get org-time-clocksum-format :require-minutes))
+	     (setq clocksum (concat clocksum (format 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