[PATCH] Org Habit fix + new feature

[PATCH] Org Habit fix + new feature

[Morgan Smith]

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

Hello!

There are two patches here.  The first one is a simple bug fix that
doesn't really have anything to do with the second patch.  It just
happens to be in the same spot of the code.

The second patch allows a habit to be considered done if time was logged
to it.  Imagine you have an org habit like shaving.  Chances are, if you
spend time doing it, it's done.  I like to set LOGGING to nil for these
kinds of habits since it's redundant to have all those state changes
that tell me exactly what the logbook already tells me.

Let me know what you guys think.  Any discussion about the second patch
though shouldn't stop the first one from being applied.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-org-habit.el-Use-time-as-a-history-cutoff-point.patch --]
[-- Type: text/x-patch, Size: 2025 bytes --]

From cc16dd6a8c59312a75b8e25669a7e4eb3d9f9ef4 Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Tue, 11 Oct 2022 11:44:26 -0400
Subject: [PATCH 1/2] lisp/org-habit.el: Use time as a history cutoff point

* lisp/org-habit.el (org-habit-parse-todo): Use time as a cutoff point
instead of using a count.

This allows viewing the full history of habits that are completed
multiple times a day.  Previously we would miss some days and show an
incorrect history
---
 lisp/org-habit.el | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lisp/org-habit.el b/lisp/org-habit.el
index 677b7adb6..fe5f4fe63 100644
--- a/lisp/org-habit.el
+++ b/lisp/org-habit.el
@@ -212,11 +212,11 @@ This list represents a \"habit\" for the rest of this module."
 		   habit-entry scheduled-repeat))
 	(setq deadline (+ scheduled (- dr-days sr-days))))
       (org-back-to-heading t)
-      (let* ((maxdays (+ org-habit-preceding-days org-habit-following-days))
+      (let* ((firstday (- (org-today) org-habit-preceding-days))
 	     (reversed org-log-states-order-reversed)
 	     (search (if reversed 're-search-forward 're-search-backward))
 	     (limit (if reversed end (point)))
-	     (count 0)
+	     (done nil)
 	     (re (format
 		  "^[ \t]*-[ \t]+\\(?:State \"%s\".*%s%s\\)"
 		  (regexp-opt org-done-keywords)
@@ -235,13 +235,14 @@ This list represents a \"habit\" for the rest of this module."
 				 ("%u" . ".*?")
 				 ("%U" . ".*?")))))))))
 	(unless reversed (goto-char end))
-	(while (and (< count maxdays) (funcall search re limit t))
+	(while (and (not done) (funcall search re limit t))
 	  (push (time-to-days
 		 (org-time-string-to-time
 		  (or (match-string-no-properties 1)
 		      (match-string-no-properties 2))))
 		closed-dates)
-	  (setq count (1+ count))))
+          (when (< (car closed-dates) firstday)
+            (setq done t))))
       (list scheduled sr-days deadline dr-days closed-dates sr-type))))
 
 (defsubst org-habit-scheduled (habit)
-- 
2.38.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-lisp-org-habit.el-Allow-clocking-time-to-complete-a-.patch --]
[-- Type: text/x-patch, Size: 3954 bytes --]

From 93929b6722e7171551f8390581593e8ea76f1340 Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Tue, 11 Oct 2022 11:46:39 -0400
Subject: [PATCH 2/2] lisp/org-habit.el: Allow clocking time to complete a
 habit

* lisp/org-habit.el:
(org-habit-clock-completes-habit): Add
(org-habit-parse-todo): Allow clocking time to complete a habit

* etc/ORG-NEWS: Add entry for `org-habit-clock-completes-habit'

* doc/org-manual.org: Add entry for `org-habit-clock-completes-habit'
---
 doc/org-manual.org |  6 ++++++
 etc/ORG-NEWS       |  5 +++++
 lisp/org-habit.el  | 18 ++++++++++++++----
 3 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/doc/org-manual.org b/doc/org-manual.org
index 428c79923..a3dcfb3f9 100644
--- a/doc/org-manual.org
+++ b/doc/org-manual.org
@@ -4423,6 +4423,12 @@ the way habits are displayed in the agenda.
   value is ~t~.  Pressing {{{kbd(C-u K)}}} in the agenda toggles this
   variable.
 
+- ~org-habit-clock-completes-habit~ ::
+
+  #+vindex: org-habit-clock-completes-habit
+  If non-~nil~, a habit is considered done on days that time has been
+  clocked to it.
+
 Lastly, pressing {{{kbd(K)}}} in the agenda buffer causes habits to
 temporarily be disabled and do not appear at all.  Press {{{kbd(K)}}}
 again to bring them back.  They are also subject to tag filtering, if
diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index c18c03725..1a3861f60 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -360,6 +360,11 @@ The new setting, when set to non-nil, makes Org create alarm at the
 event time when the alarm time is set to 0.  The default value is
 nil -- do not create alarms at the event time.
 
+*** New custom setting ~org-habit-clock-completes-habit~
+
+The new setting, when set to non-nil, makes Org consider a habit done
+on days that time has been clocked to it.
+
 ** New functions and changes in function arguments
 *** ~org-fold-show-entry~ does not fold drawers by default anymore
 
diff --git a/lisp/org-habit.el b/lisp/org-habit.el
index fe5f4fe63..66aad06c1 100644
--- a/lisp/org-habit.el
+++ b/lisp/org-habit.el
@@ -61,6 +61,11 @@ Note that consistency graphs will overwrite anything else in the buffer."
   :group 'org-habit
   :type 'boolean)
 
+(defcustom org-habit-clock-completes-habit nil
+  "If non-nil, a habit is considered done on days that time has been clocked to it."
+  :group 'org-habit
+  :type 'boolean)
+
 (defcustom org-habit-show-habits-only-for-today t
   "If non-nil, only show habits on today's agenda, and not for future days.
 Note that even when shown for future days, the graph is always
@@ -218,7 +223,7 @@ This list represents a \"habit\" for the rest of this module."
 	     (limit (if reversed end (point)))
 	     (done nil)
 	     (re (format
-		  "^[ \t]*-[ \t]+\\(?:State \"%s\".*%s%s\\)"
+		  "^[ \t]*-[ \t]+\\(?:State \"%s\".*%s%s\\)%s"
 		  (regexp-opt org-done-keywords)
 		  org-ts-regexp-inactive
 		  (let ((value (cdr (assq 'done org-log-note-headings))))
@@ -233,15 +238,20 @@ This list represents a \"habit\" for the rest of this module."
 				 ("%t" . ,org-ts-regexp-inactive)
 				 ("%T" . ,org-ts-regexp)
 				 ("%u" . ".*?")
-				 ("%U" . ".*?")))))))))
+				 ("%U" . ".*?"))))))
+                  (if org-habit-clock-completes-habit
+                      (concat
+                       "\\|^" org-clock-string ".*\\]--\\(\\[[^]]+\\]\\)")
+                    ""))))
 	(unless reversed (goto-char end))
 	(while (and (not done) (funcall search re limit t))
 	  (push (time-to-days
 		 (org-time-string-to-time
 		  (or (match-string-no-properties 1)
-		      (match-string-no-properties 2))))
+		      (match-string-no-properties 2)
+                      (match-string-no-properties 3))))
 		closed-dates)
-          (when (< (car closed-dates) firstday)
+          (when (<= (car closed-dates) firstday)
             (setq done t))))
       (list scheduled sr-days deadline dr-days closed-dates sr-type))))
 
-- 
2.38.0


[-- Attachment #4: Type: text/plain, Size: 18 bytes --]



Thanks,

Morgan

Re: [PATCH] Org Habit fix + new feature

[Colin Baxter]

>>>>> Morgan Smith <Morgan.J.Smith@outlook.com> writes:

    > Hello!  There are two patches here.  The first one is a simple bug
    > fix that doesn't really have anything to do with the second patch.
    > It just happens to be in the same spot of the code.

    > The second patch allows a habit to be considered done if time was
    > logged to it.  Imagine you have an org habit like shaving.
    > Chances are, if you spend time doing it, it's done.  I like to set
    > LOGGING to nil for these kinds of habits since it's redundant to
    > have all those state changes that tell me exactly what the logbook
    > already tells me.

Please do not alter the default behaviour. When writing a paper or a
book I use and need both logging and state changes, and I would prefer
not to have to spend time changing my setup.

Re: [PATCH] Org Habit fix + new feature

[Morgan Smith]

Hello,

Colin Baxter <m43cap@yandex.com> writes:

> Please do not alter the default behaviour. When writing a paper or a
> book I use and need both logging and state changes, and I would prefer
> not to have to spend time changing my setup.

Don't worry, this shouldn't change the default behavior in the
slightest.

Morgan

Re: [PATCH] Org Habit fix + new feature

[Colin Baxter]

>>>>> Morgan Smith <Morgan.J.Smith@outlook.com> writes:

    > Hello,

    > Colin Baxter <m43cap@yandex.com> writes:

    >> Please do not alter the default behaviour. When writing a paper
    >> or a book I use and need both logging and state changes, and I
    >> would prefer not to have to spend time changing my setup.

    > Don't worry, this shouldn't change the default behavior in the
    > slightest.

    > Morgan

Thank you.

Re: [PATCH] Org Habit fix + new feature

[Ihor Radchenko]

Morgan Smith <Morgan.J.Smith@outlook.com> writes:

> The second patch allows a habit to be considered done if time was logged
> to it.  Imagine you have an org habit like shaving.  Chances are, if you
> spend time doing it, it's done.  I like to set LOGGING to nil for these
> kinds of habits since it's redundant to have all those state changes
> that tell me exactly what the logbook already tells me.

I am not against such feature. However, using clocking will break an
assumption that a single log record corresponds to a single habit
completion. This assumption is implied across org-habit code.  

> From cc16dd6a8c59312a75b8e25669a7e4eb3d9f9ef4 Mon Sep 17 00:00:00 2001
> From: Morgan Smith <Morgan.J.Smith@outlook.com>
> Date: Tue, 11 Oct 2022 11:44:26 -0400
> Subject: [PATCH 1/2] lisp/org-habit.el: Use time as a history cutoff point
>
> * lisp/org-habit.el (org-habit-parse-todo): Use time as a cutoff point
> instead of using a count.
>
> This allows viewing the full history of habits that are completed
> multiple times a day.  Previously we would miss some days and show an
> incorrect history

There is currently nothing in the manual or function docstring that
suggest supporting habits that are repeated multiple times a day.

5.3.3 Tracking your habits section of the manual says:

       What’s really useful about habits is that they are displayed along
    with a consistency graph, to show how consistent you’ve been at getting
    that task done in the past.  This graph shows every day that the task
    was done over the past three weeks, with colors for each day.  The
    colors used are:

Explicitly saying that each symbol in the graph corresponds to a single
day.

Further, org-habit-preceding-days and org-habit-following-days are
explicitly talking about days, not repetitions.

> -      (let* ((maxdays (+ org-habit-preceding-days org-habit-following-days))
> +      (let* ((firstday (- (org-today) org-habit-preceding-days))

What about org-habit-following-days? Why did you throw it away?
Also, (org-today) does not consider org-extend-today-until. (see
org-habit-insert-consistency-graphs).

>  	     (re (format
> -		  "^[ \t]*-[ \t]+\\(?:State \"%s\".*%s%s\\)"
> +		  "^[ \t]*-[ \t]+\\(?:State \"%s\".*%s%s\\)%s"
>  		  (regexp-opt org-done-keywords)
>  		  org-ts-regexp-inactive
>  		  (let ((value (cdr (assq 'done org-log-note-headings))))
> @@ -233,15 +238,20 @@ This list represents a \"habit\" for the rest of this module."
>  				 ("%t" . ,org-ts-regexp-inactive)
>  				 ("%T" . ,org-ts-regexp)
>  				 ("%u" . ".*?")
> -				 ("%U" . ".*?")))))))))
> +				 ("%U" . ".*?"))))))
> +                  (if org-habit-clock-completes-habit
> +                      (concat
> +                       "\\|^" org-clock-string ".*\\]--\\(\\[[^]]+\\]\\)")
> +                    ""))))

This logic will fail for non-default combinations of org-log-into-drawer
+ org-clock-into-drawer + org-log-states-order-reversed.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] Org Habit fix + new feature

[Morgan Smith]

Ihor Radchenko <yantar92@gmail.com> writes:
>
> I am not against such feature. However, using clocking will break an
> assumption that a single log record corresponds to a single habit
> completion. This assumption is implied across org-habit code.  
>
Oh that's a good point.  I'll have to go back through the code and see
if that's an issue.



As for your comments on the first patch, let me explain where the
current logic falls short.  Imagine you complete a habit multiple times
in a day because you're using org wrong (yep this also violates that
previous thing that I gotta look into but let's not worry about that for
now).  The current logic simply looks at the previous '(+
org-habit-preceding-days org-habit-following-days)' log records which by
default would be 28.  First of all, why are we looking into the future
at all?  I don't think the habits graph currently supports looking at it
from the perspective of a different day and I think marking things as
complete in the future is pretty odd.  Second of all, if we use org
wrong, then we will start loosing days at the beginning of the graph if
we have more then 28 log records in our period.

Now my patch calculates the first day of the graph and simply looks at
all log records before that date.  This is more robust if we want to use
org wrong. Also it's more intuitive I think.  In many cases I think it
will also be a performance boost since then we likely won't loop the
full 28 times.  Furthermore, this method would support looking at the
habits graph from the perspective of a different day (which blindly
looping 28 times does not).

This patch does not do a good job at adding support for repetitions.
The graph and logic still works in days, not repetitions.  It simply
makes the current code more robust.

> Also, (org-today) does not consider org-extend-today-until. (see
> org-habit-insert-consistency-graphs).

Thanks for catching that!

>
> This logic will fail for non-default combinations of org-log-into-drawer
> + org-clock-into-drawer + org-log-states-order-reversed.

Well shoot.  That's a bummer.  So why are we using a regex here anyways?
It feels not super robust.  Don't we have an AST we could use instead?
Also even if we do want to use regex, pulling out log records and clock
records seems like a pretty common thing to do that should be in a
core library function right?


Thanks for the review!
I appreciate your feedback


Morgan

Re: [PATCH] Org Habit fix + new feature

[Ihor Radchenko]

Morgan Smith <Morgan.J.Smith@outlook.com> writes:

> ...  The current logic simply looks at the previous '(+
> org-habit-preceding-days org-habit-following-days)' log records which by
> default would be 28.  First of all, why are we looking into the future
> at all?  I don't think the habits graph currently supports looking at it
> from the perspective of a different day and I think marking things as
> complete in the future is pretty odd.

Well. Ideally, the habit graph should use current agenda day. Not the
actual current date.

> Second of all, if we use org
> wrong, then we will start loosing days at the beginning of the graph if
> we have more then 28 log records in our period.

Agree.

> Now my patch calculates the first day of the graph and simply looks at
> all log records before that date.  This is more robust if we want to use
> org wrong. Also it's more intuitive I think.  In many cases I think it
> will also be a performance boost since then we likely won't loop the
> full 28 times.  Furthermore, this method would support looking at the
> habits graph from the perspective of a different day (which blindly
> looping 28 times does not).
> This patch does not do a good job at adding support for repetitions.
> The graph and logic still works in days, not repetitions.  It simply
> makes the current code more robust.

Agree.

>> This logic will fail for non-default combinations of org-log-into-drawer
>> + org-clock-into-drawer + org-log-states-order-reversed.
>
> Well shoot.  That's a bummer.  So why are we using a regex here anyways?
> It feels not super robust.  Don't we have an AST we could use instead?
> Also even if we do want to use regex, pulling out log records and clock
> records seems like a pretty common thing to do that should be in a
> core library function right?

Because the last serious change in org-habit was 8 years ago (de51e1aef)
and the main implementation dates back to 13 years ago. org-element has
been introduced 10 years ago. It was simply not a thing when the
original org-habit had been developed.

Similar history goes over clock and log parsing. Nobody bothered to
update them for the new element API.

And notes format is only regexp-based. There is no fixed AST element for
notes. Just hard-coded conventions.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>