Re: [PATCH] org-agenda: Make sure skipping warning/delay days never increases their number

[Tim Ruffing <crypto@timruffing.de>]

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

On Wed, 2024-02-14 at 14:20 +0000, Ihor Radchenko wrote:
> > 
> We can do this - I do not see any code in the wild using the optional
> arguments. However, we should do it carefully - (1) Move the function
> to
> org-compat and mark obsolete, recommending a new function instead;
> (2)
> Create a new function with a new name (say,
> org-timestamp-get-warning-days) and without ZERO-OR-DELAY argument
> and
> use it across Org mode.
> 

Hm, now that I think about it again, I'm not sure that defining a new
function is worth the hassle. There's nothing wrong with the current 
function or the optional argument, in the sense that callers can simply
choose not to use the optional argument.

If anything, I think it would rather add a comment that it's
obsolete/there for historical reasons and should not be used. Let me
know if you think that's desirable.

> > -	          (suppress-prewarning
> > +	          (max-wdays
> 
> May you also rename this variable to be more clear. Something like,
> max-warning-days? These short variable names make the code very
> difficult to read.
> 
> > -	          (wdays (or suppress-prewarning (org-get-wdays
> > s))))
> > +	          (wdays (min max-wdays (org-get-wdays s))))
> 
> warning-days
> 
> > -	          (suppress-delay
> > +	          (max-ddays
> 
> max-deadline-warning-days
> 

Done.

Tim

[-- Attachment #2: 0001-org-agenda-Make-sure-skipping-warning-delay-days-nev.patch --]
[-- Type: text/x-patch, Size: 7777 bytes --]

From 145b2526882264985d497ee094006b7144aa860b Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Tue, 13 Feb 2024 10:57:29 +0100
Subject: [PATCH] org-agenda: Make sure skipping warning/delay days never
 increases their number

* lisp/org-agenda.el (org-agenda-get-deadlines, org-agenda-get-scheduled):
Use minimum of warning/delay days specified in timestamp cookie and the
limit specified by `org-agenda-skip-deadline-prewarning-if-scheduled' or
`org-agenda-skip-scheduled-delay-if-deadline`, respectively.
* testing/lisp/test-org-agenda.el (test-org-agenda/skip-deadline-prewarning-if-scheduled):
New test
---
 lisp/org-agenda.el              | 25 +++++++++-----------
 testing/lisp/test-org-agenda.el | 41 +++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/lisp/org-agenda.el b/lisp/org-agenda.el
index 23ccea9df..83e1d1264 100644
--- a/lisp/org-agenda.el
+++ b/lisp/org-agenda.el
@@ -6399,14 +6399,14 @@ specification like [h]h:mm."
 		       (org-agenda--timestamp-to-absolute
 		        s base 'future (current-buffer) pos)))))
 	          (diff (- deadline current))
-	          (suppress-prewarning
+	          (max-warning-days
 		   (let ((scheduled
 		          (and org-agenda-skip-deadline-prewarning-if-scheduled
                                (org-element-property
                                 :raw-value
                                 (org-element-property :scheduled el)))))
 		     (cond
-		      ((not scheduled) nil)
+		      ((not scheduled) most-positive-fixnum)
 		      ;; The current item has a scheduled date, so
 		      ;; evaluate its prewarning lead time.
 		      ((integerp org-agenda-skip-deadline-prewarning-if-scheduled)
@@ -6420,15 +6420,15 @@ specification like [h]h:mm."
 			    org-deadline-warning-days))
 		      ;; Set pre-warning to deadline.
 		      (t 0))))
-	          (wdays (or suppress-prewarning (org-get-wdays s))))
+	          (warning-days (min max-warning-days (org-get-wdays s))))
 	     (cond
 	      ;; Only display deadlines at their base date, at future
 	      ;; repeat occurrences or in today agenda.
 	      ((= current deadline) nil)
 	      ((= current repeat) nil)
 	      ((not today?) (throw :skip nil))
-	      ;; Upcoming deadline: display within warning period WDAYS.
-	      ((> deadline current) (when (> diff wdays) (throw :skip nil)))
+	      ;; Upcoming deadline: display within warning period WARNING-DAYS.
+	      ((> deadline current) (when (> diff warning-days) (throw :skip nil)))
 	      ;; Overdue deadline: warn about it for
 	      ;; `org-deadline-past-days' duration.
 	      (t (when (< org-deadline-past-days (- diff)) (throw :skip nil))))
@@ -6481,7 +6481,7 @@ specification like [h]h:mm."
                           'effort-minutes effort-minutes)
                         level category tags time))
 		      (face (org-agenda-deadline-face
-			     (- 1 (/ (float diff) (max wdays 1)))))
+			     (- 1 (/ (float diff) (max warning-days 1)))))
 		      (upcoming? (and today? (> deadline today)))
 		      (warntime (org-entry-get (point) "APPT_WARNTIME" 'selective)))
 	         (org-add-props item props
@@ -6610,13 +6610,13 @@ scheduled items with an hour specification like [h]h:mm."
 	          (futureschedp (> schedule today))
 	          (habitp (and (fboundp 'org-is-habit-p)
                                (string= "habit" (org-element-property :STYLE el))))
-	          (suppress-delay
+	          (max-delay-days
 		   (let ((deadline (and org-agenda-skip-scheduled-delay-if-deadline
                                         (org-element-property
                                          :raw-value
                                          (org-element-property :deadline el)))))
 		     (cond
-		      ((not deadline) nil)
+		      ((not deadline) most-positive-fixnum)
 		      ;; The current item has a deadline date, so
 		      ;; evaluate its delay time.
 		      ((integerp org-agenda-skip-scheduled-delay-if-deadline)
@@ -6629,17 +6629,14 @@ scheduled items with an hour specification like [h]h:mm."
 			       (org-agenda--timestamp-to-absolute deadline))
 			    org-scheduled-delay-days))
 		      (t 0))))
-	          (ddays
+	          (delay-days
 		   (cond
 		    ;; Nullify delay when a repeater triggered already
 		    ;; and the delay is of the form --Xd.
 		    ((and (string-match-p "--[0-9]+[hdwmy]" s)
 		          (> schedule (org-agenda--timestamp-to-absolute s)))
 		     0)
-		    (suppress-delay
-		     (let ((org-scheduled-delay-days suppress-delay))
-		       (org-get-wdays s t t)))
-		    (t (org-get-wdays s t)))))
+		    (t (min max-delay-days (org-get-wdays s t))))))
 	     ;; Display scheduled items at base date (SCHEDULE), today if
 	     ;; scheduled before the current date, and at any repeat past
 	     ;; today.  However, skip delayed items and items that have
@@ -6647,7 +6644,7 @@ scheduled items with an hour specification like [h]h:mm."
 	     (unless (and todayp
 		          habitp
 		          (bound-and-true-p org-habit-show-all-today))
-	       (when (or (and (> ddays 0) (< diff ddays))
+	       (when (or (and (> delay-days 0) (< diff delay-days))
 		         (> diff (or (and habitp org-habit-scheduled-past-days)
 				     org-scheduled-past-days))
 		         (> schedule current)
diff --git a/testing/lisp/test-org-agenda.el b/testing/lisp/test-org-agenda.el
index f98214b25..3348db473 100644
--- a/testing/lisp/test-org-agenda.el
+++ b/testing/lisp/test-org-agenda.el
@@ -687,6 +687,47 @@ Sunday      7 January 2024
     (should-not (org-agenda-files)))
   (org-test-agenda--kill-all-agendas))
 
+(ert-deftest test-org-agenda/skip-deadline-prewarning-if-scheduled ()
+  "Test `org-agenda-skip-deadline-prewarning-if-scheduled'."
+  (org-test-at-time
+   "2024-01-15"
+   (let ((org-agenda-skip-deadline-prewarning-if-scheduled t))
+     (org-test-agenda-with-agenda
+      "* TODO foo\nDEADLINE: <2024-01-20 Sat> SCHEDULED: <2024-01-19 Fri>"
+      (org-agenda-list nil nil 1)
+      (should-not (search-forward "In " nil t))))
+   (let ((org-agenda-skip-deadline-prewarning-if-scheduled 10))
+     (org-test-agenda-with-agenda
+      "* TODO foo\nDEADLINE: <2024-01-20 Sat> SCHEDULED: <2024-01-19 Fri>"
+      (org-agenda-list nil nil 1)
+      (should (search-forward "In " nil t))))
+   ;; Custom prewarning cookie "-3d", so there should be no warning anyway.
+   (let ((org-agenda-skip-deadline-prewarning-if-scheduled 10))
+     (org-test-agenda-with-agenda
+      "* TODO foo\nDEADLINE: <2024-01-20 Sat -3d> SCHEDULED: <2024-01-19 Fri>"
+      (org-agenda-list nil nil 1)
+      (should-not (search-forward "In " nil t))))
+   (let ((org-agenda-skip-deadline-prewarning-if-scheduled 3))
+     (org-test-agenda-with-agenda
+      "* TODO foo\nDEADLINE: <2024-01-20 Sat> SCHEDULED: <2024-01-19 Fri>"
+      (org-agenda-list nil nil 1)
+      (should-not (search-forward "In " nil t))))
+   (let ((org-agenda-skip-deadline-prewarning-if-scheduled nil))
+     (org-test-agenda-with-agenda
+      "* TODO foo\nDEADLINE: <2024-01-20 Sat> SCHEDULED: <2024-01-19 Fri>"
+      (org-agenda-list nil nil 1)
+      (should (search-forward "In " nil t))))
+   (let ((org-agenda-skip-deadline-prewarning-if-scheduled 'pre-scheduled))
+     (org-test-agenda-with-agenda
+      "* TODO foo\nDEADLINE: <2024-01-20 Sat> SCHEDULED: <2024-01-16 Tue>"
+      (org-agenda-list nil nil 1)
+      (should-not (search-forward "In " nil t))))
+   (let ((org-agenda-skip-deadline-prewarning-if-scheduled 'pre-scheduled))
+     (org-test-agenda-with-agenda
+      "* TODO foo\nDEADLINE: <2024-01-20 Sat> SCHEDULED: <2024-01-15 Mon>"
+      (org-agenda-list nil nil 1)
+      (should (search-forward "In " nil t))))))
+
 \f
 ;; agenda redo
 
-- 
2.44.0