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

[Ihor Radchenko]

Bastien Guerry  writes:

> Ihor Radchenko  writes:
>
>> Bastien, may you please check FSF records?
>
> Done, and confirmed things are in order.  Welcome!

Recorded.
https://git.sr.ht/~bzg/worg/commit/4760468a

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

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

[Bastien Guerry]

Ihor Radchenko  writes:

> Bastien, may you please check FSF records?

Done, and confirmed things are in order.  Welcome!

-- 
 Bastien Guerry

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

[Ihor Radchenko]

Tim Ruffing  writes:

> On Tue, 2024-02-27 at 12:49 +, Ihor Radchenko wrote:
>> However, your total contributions are now at the limit we can legally
>> accept without FSF copyright assignment.
>> Would you consider doing copyright paperwork as described in
>> ?
>> 
>
> Oh, I had completed the process on 2023-05-03, after my previous patch.
> But yeah, I never told you. :)

Thanks for letting me know!
Bastien, may you please check FSF records?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

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

[Tim Ruffing]

On Tue, 2024-02-27 at 12:49 +, Ihor Radchenko wrote:
> However, your total contributions are now at the limit we can legally
> accept without FSF copyright assignment.
> Would you consider doing copyright paperwork as described in
> ?
> 

Oh, I had completed the process on 2023-05-03, after my previous patch.
But yeah, I never told you. :)

Best,
Tim

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

[Ihor Radchenko]

Tim Ruffing  writes:

> 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.

Yes, we can add such a comment to the docstring.
However, your total contributions are now at the limit we can legally
accept without FSF copyright assignment.
Would you consider doing copyright paperwork as described in
<https://orgmode.org/worg/org-contribute.html#copyright>?

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

Applied, onto main, with minor amendments to the commit message.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=356072c1d
Thanks!

-- 
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-agenda: Make sure skipping warning/delay days never increases their number

[Tim Ruffing]

On Wed, 2024-02-14 at 14:20 +, 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
From 145b2526882264985d497ee094006b7144aa860b Mon Sep 17 00:00:00 2001
From: Tim Ruffing 
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
-	  (supp

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

[Ihor Radchenko]

Tim Ruffing  writes:

>> Please do not remove arguments from the public functions. This may
>> break
>> code outside Org mode.
>> 
> Hm, sure, I assumed it's okay for this niche thing. Can we deprecate
> the argument somehow?

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.

> -   (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

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

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

[Tim Ruffing]

Added a test.

> Please do not remove arguments from the public functions. This may
> break
> code outside Org mode.
> 

Hm, sure, I assumed it's okay for this niche thing. Can we deprecate
the argument somehow?

Best,
Tim
From b886446be8ea02f38d9be6cccf6899d6de396d06 Mon Sep 17 00:00:00 2001
From: Tim Ruffing 
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  | 15 +---
 testing/lisp/test-org-agenda.el | 41 +
 2 files changed, 47 insertions(+), 9 deletions(-)

diff --git a/lisp/org-agenda.el b/lisp/org-agenda.el
index 514359b62..53878e441 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-wdays
 		   (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,7 +6420,7 @@ specification like [h]h:mm."
 			org-deadline-warning-days))
 		  ;; Set pre-warning to deadline.
 		  (t 0
-	  (wdays (or suppress-prewarning (org-get-wdays s
+	  (wdays (min max-wdays (org-get-wdays s
 	 (cond
 	  ;; Only display deadlines at their base date, at future
 	  ;; repeat occurrences or in today agenda.
@@ -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-ddays
 		   (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)
@@ -6636,10 +6636,7 @@ scheduled items with an hour specification like [h]h:mm."
 		((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-ddays (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
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-

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

[Ihor Radchenko]

Tim Ruffing  writes:

> The attached patch fixes the following bug:
>
>  * Have a scheduled item that has also has a deadline with a cusom
>prewarning cookie, e.g. <... -3d>.
>  * Set `org-agenda-skip-deadline-prewarning-if-scheduled' to 7
>
> Then in the agenda, the item is shown already 7 days before the
> deadline (instead of 3). Judging from their name and documentation, the
> "skip" variables `org-agenda-skip-deadline-prewarning-if-scheduled' and
> `org-agenda-skip-scheduled-delay-if-deadline` should only ever skip
> items in the agenda. In other words, they should only shorten the
> prewarning or delay period, but never extend it. 

Thanks!
May you also add a test?

> -(defun org-get-wdays (ts  delay zero-delay)
> +(defun org-get-wdays (ts  delay)

Please do not remove arguments from the public functions. This may break
code outside Org mode.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

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

[Tim Ruffing]

Hello,

The attached patch fixes the following bug:

 * Have a scheduled item that has also has a deadline with a cusom
   prewarning cookie, e.g. <... -3d>.
 * Set `org-agenda-skip-deadline-prewarning-if-scheduled' to 7

Then in the agenda, the item is shown already 7 days before the
deadline (instead of 3). Judging from their name and documentation, the
"skip" variables `org-agenda-skip-deadline-prewarning-if-scheduled' and
`org-agenda-skip-scheduled-delay-if-deadline` should only ever skip
items in the agenda. In other words, they should only shorten the
prewarning or delay period, but never extend it. 

Best,
Tim
From 5db0ac7115b86a12d1a2106eb54b07d339f69ed6 Mon Sep 17 00:00:00 2001
From: Tim Ruffing 
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.
* lisp/org.el (org-get-wdays): Remove unused (and confusing) ZERO-DELAY
argument.
---
 lisp/org-agenda.el | 15 ++-
 lisp/org.el|  7 ++-
 2 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/lisp/org-agenda.el b/lisp/org-agenda.el
index 514359b62..53878e441 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-wdays
 		   (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,7 +6420,7 @@ specification like [h]h:mm."
 			org-deadline-warning-days))
 		  ;; Set pre-warning to deadline.
 		  (t 0
-	  (wdays (or suppress-prewarning (org-get-wdays s
+	  (wdays (min max-wdays (org-get-wdays s
 	 (cond
 	  ;; Only display deadlines at their base date, at future
 	  ;; repeat occurrences or in today agenda.
@@ -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-ddays
 		   (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)
@@ -6636,10 +6636,7 @@ scheduled items with an hour specification like [h]h:mm."
 		((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-ddays (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
diff --git a/lisp/org.el b/lisp/org.el
index 7da23310d..75f062508 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -14454,17 +14454,14 @@ If SECONDS is non-nil, return the difference in seconds."
   (and (<= (org-timestamp-to-now timestamp-string) ndays)
(not (org-entry-is-done-p
 
-(defun org-get-wdays (ts  delay zero-delay)
+(defun org-get-wdays (ts  delay)
   "Get the deadline lead time appropriate for timestring TS.
 When DELAY is non-nil, get the delay time for scheduled items
-instead of the deadline lead time.  When ZERO-DELAY is non-nil
-and `org-scheduled-delay-days' is 0, enforce 0 as the delay,
-don't try to find the delay cookie in the scheduled timestamp."
+instead of the deadline lead time."
   (let ((tv (if delay org-scheduled-delay-days
 	  org-deadline-warning-days)))
 (cond
  ((or