GNU bug report logs - #70134
[PATCH] Show all date options when adding Gnus scores interactively

Previous Next

Package: emacs;

Reported by: Jakub Ječmínek <kuba <at> kubajecminek.cz>

Date: Mon, 1 Apr 2024 21:45:01 UTC

Severity: normal

Tags: patch

To reply to this bug, email your comments to 70134 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#70134; Package emacs. (Mon, 01 Apr 2024 21:45:01 GMT) Full text and rfc822 format available.

Acknowledgement sent to Jakub Ječmínek <kuba <at> kubajecminek.cz>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Mon, 01 Apr 2024 21:45:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jakub Ječmínek <kuba <at> kubajecminek.cz>
To: bug-gnu-emacs <at> gnu.org
Cc: Eric Abrahamsen <eric <at> ericabrahamsen.net>, larsi <at> gnus.org,
 Alex Bochannek <alex <at> bochannek.com>
Subject: [PATCH] Show all date options when adding Gnus scores interactively
Date: Mon, 01 Apr 2024 21:44:04 +0000
[Message part 1 (text/plain, inline)]
Tags: patch


Hello, I'm attaching a patch that enhances user experience when adding
Gnus scores interactively based on date header. This patch does multiple
things:

1. `char-to-type' variable is renamed to `char-to-types' and we allow it
to be mapped to multiple types. This is useful because date header
supports all these types for scoring - '<, '>, 'r, 'after, 'before, 'at
but only the last three were previously offered to the user when using
interactive scoring.

2. `gnus-summary-score-entry' now provides sensible defaults for each
date scoring type - if we're scoring based on integer value ('<, '>) we
offer number of days between now and entry at point (revised version of
what was introduced in bug#61002) and if we're scoring based on string
value ('after, 'before, 'at, 'r) we provide valid date string.

3. If the user scores articles based on date strings ('before, 'after,
'at, 'r), `gnus-summary-score-entry' will no longer cast the type to the
integer type.

4. `gnus-score-date' catches 'r type as well.

I'm also Cc'ing Alex who was the original author of '< '> date scoring
types. Please let me know what you think.


In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.33, cairo version 1.16.0) of 2024-03-23 built on
 kuba-ThinkPad-T14-Gen-3
Repository revision: 7e32e8392ab77f9df08a1f11831cbba2242d721f
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12201001
System Description: Ubuntu 22.04.4 LTS

[0001-Show-all-date-options-when-adding-Gnus-scores-interactively.patch (text/patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70134; Package emacs. (Sat, 13 Apr 2024 07:59:03 GMT) Full text and rfc822 format available.

Message #8 received at 70134 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: eric <at> ericabrahamsen.net, Jakub Ječmínek
 <kuba <at> kubajecminek.cz>
Cc: 70134 <at> debbugs.gnu.org, larsi <at> gnus.org, alex <at> bochannek.com
Subject: Re: bug#70134: [PATCH] Show all date options when adding Gnus scores
 interactively
Date: Sat, 13 Apr 2024 10:58:37 +0300
Ping!  Eric, any comments?

> Cc: Eric Abrahamsen <eric <at> ericabrahamsen.net>, larsi <at> gnus.org,
>  Alex Bochannek <alex <at> bochannek.com>
> Date: Mon, 01 Apr 2024 21:44:04 +0000
> From:  Jakub Ječmínek via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> Hello, I'm attaching a patch that enhances user experience when adding
> Gnus scores interactively based on date header. This patch does multiple
> things:
> 
> 1. `char-to-type' variable is renamed to `char-to-types' and we allow it
> to be mapped to multiple types. This is useful because date header
> supports all these types for scoring - '<, '>, 'r, 'after, 'before, 'at
> but only the last three were previously offered to the user when using
> interactive scoring.
> 
> 2. `gnus-summary-score-entry' now provides sensible defaults for each
> date scoring type - if we're scoring based on integer value ('<, '>) we
> offer number of days between now and entry at point (revised version of
> what was introduced in bug#61002) and if we're scoring based on string
> value ('after, 'before, 'at, 'r) we provide valid date string.
> 
> 3. If the user scores articles based on date strings ('before, 'after,
> 'at, 'r), `gnus-summary-score-entry' will no longer cast the type to the
> integer type.
> 
> 4. `gnus-score-date' catches 'r type as well.
> 
> I'm also Cc'ing Alex who was the original author of '< '> date scoring
> types. Please let me know what you think.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70134; Package emacs. (Mon, 22 Apr 2024 03:34:02 GMT) Full text and rfc822 format available.

Message #11 received at 70134 <at> debbugs.gnu.org (full text, mbox):

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 70134 <at> debbugs.gnu.org,
 Jakub Ječmínek <kuba <at> kubajecminek.cz>, larsi <at> gnus.org,
 alex <at> bochannek.com
Subject: Re: bug#70134: [PATCH] Show all date options when adding Gnus
 scores interactively
Date: Sun, 21 Apr 2024 20:33:00 -0700
Eli Zaretskii <eliz <at> gnu.org> writes:

> Ping!  Eric, any comments?

I have no comments! I don't know the scoring code, and fifteen minutes
of looking at the patch and its surroundings has left me none the wiser.
I don't know what it means to add multiple types to each char; I don't
know what a "type" is in this context. Jakub, do you think you could
enlighten me within a short paragraph or so? I'm also okay with just
chucking it in, and letting people in gnus.general know that things have
changed. There are a few hardcore scoring users there.

Thanks,
Eric




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70134; Package emacs. (Wed, 24 Apr 2024 22:53:08 GMT) Full text and rfc822 format available.

Message #14 received at 70134 <at> debbugs.gnu.org (full text, mbox):

From: Richard Stallman <rms <at> gnu.org>
To: Eric Abrahamsen <eric <at> ericabrahamsen.net>
Cc: 70134 <at> debbugs.gnu.org, kuba <at> kubajecminek.cz, eliz <at> gnu.org, larsi <at> gnus.org,
 alex <at> bochannek.com
Subject: Re: bug#70134: [PATCH] Show all date options when adding Gnus scores
 interactively
Date: Wed, 24 Apr 2024 18:52:04 -0400
[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > I have no comments! I don't know the scoring code, and fifteen minutes
  > of looking at the patch and its surroundings has left me none the wiser.

This suggests to me that that code may need better comments to explain
its overall structure.  Do you think so?

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)






Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70134; Package emacs. (Wed, 24 Apr 2024 23:10:04 GMT) Full text and rfc822 format available.

Message #17 received at 70134 <at> debbugs.gnu.org (full text, mbox):

From: Alex Bochannek <alex <at> bochannek.com>
To: Richard Stallman <rms <at> gnu.org>
Cc: Eric Abrahamsen <eric <at> ericabrahamsen.net>, 70134 <at> debbugs.gnu.org,
 eliz <at> gnu.org, larsi <at> gnus.org, kuba <at> kubajecminek.cz
Subject: Re: bug#70134: [PATCH] Show all date options when adding Gnus
 scores interactively
Date: Wed, 24 Apr 2024 16:08:37 -0700
Richard Stallman <rms <at> gnu.org> writes:

> [[[ To any NSA and FBI agents reading my email: please consider    ]]]
> [[[ whether defending the US Constitution against all enemies,     ]]]
> [[[ foreign or domestic, requires you to follow Snowden's example. ]]]
>
>   > I have no comments! I don't know the scoring code, and fifteen minutes
>   > of looking at the patch and its surroundings has left me none the wiser.
>
> This suggests to me that that code may need better comments to explain
> its overall structure.  Do you think so?

As Jakub mentioned, I am the author of that part of the Gnus scoring
code. I have not had a chance to look at the proposed patch, but I agree
that some explanations would be useful. I can look it over this weekend.

-- 
Alex.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70134; Package emacs. (Wed, 01 May 2024 19:29:02 GMT) Full text and rfc822 format available.

Message #20 received at 70134 <at> debbugs.gnu.org (full text, mbox):

From: Jakub Ječmínek <kuba <at> kubajecminek.cz>
To: Alex Bochannek <alex <at> bochannek.com>
Cc: Eric Abrahamsen <eric <at> ericabrahamsen.net>, 70134 <at> debbugs.gnu.org,
 eliz <at> gnu.org, larsi <at> gnus.org, Richard Stallman <rms <at> gnu.org>
Subject: Re: bug#70134: [PATCH] Show all date options when adding Gnus scores
 interactively
Date: Wed, 01 May 2024 19:27:44 +0000
"Eric Abrahamsen" <eric <at> ericabrahamsen.net> writes:
> I have no comments! I don't know the scoring code, and fifteen minutes
> of looking at the patch and its surroundings has left me none the wiser.
> I don't know what it means to add multiple types to each char; I don't
> know what a "type" is in this context. Jakub, do you think you could
> enlighten me within a short paragraph or so? I'm also okay with just
> chucking it in, and letting people in gnus.general know that things have
> changed. There are a few hardcore scoring users there.

Sure, please let me know if need more context.

If you hit 'I' or 'L' on any article (in the Summary buffer) then Gnus
will walk you through the scoring process. Gnus first asks you which
header you're scoring on and presents you a bunch of characters (each
character represents one header: d - date, b - body, etc.). If you
choose date, for example, then Gnus asks you for 'match type' which is
basically a handler which will decide whether to lower/increase the
score. Which scoring types are presented depends on which header you're
scoring on but current implementation allows to bind only one character
to one type. But since Alex implemented integer scoring (currently bound
to `number' type) to date, I think we should extend it and present this
option to the user.


"Alex Bochannek" <alex <at> bochannek.com> writes:
> As Jakub mentioned, I am the author of that part of the Gnus scoring
> code. I have not had a chance to look at the proposed patch, but I agree
> that some explanations would be useful. I can look it over this weekend.

Thank you, your review is appreciated. Please do you happen to remember
why you used this form

(string= "date" (nth 0 (assoc header gnus-header-index)))

instead of this

(string= header "date")


Thanks!

Best

-- 
Kuba Ječmínek (http://kubajecminek.cz)





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70134; Package emacs. (Tue, 07 May 2024 02:55:02 GMT) Full text and rfc822 format available.

Message #23 received at 70134 <at> debbugs.gnu.org (full text, mbox):

From: Alex Bochannek <alex <at> bochannek.com>
To: Jakub Ječmínek <kuba <at> kubajecminek.cz>
Cc: Eric Abrahamsen <eric <at> ericabrahamsen.net>, 70134 <at> debbugs.gnu.org,
 eliz <at> gnu.org, larsi <at> gnus.org, Richard Stallman <rms <at> gnu.org>
Subject: Re: bug#70134: [PATCH] Show all date options when adding Gnus
 scores interactively
Date: Mon, 06 May 2024 19:53:35 -0700
Jakub,

I finally had some time to look at these changes, apologies for the
delay.

Jakub Ječmínek <kuba <at> kubajecminek.cz> writes:

> "Alex Bochannek" <alex <at> bochannek.com> writes:
>> As Jakub mentioned, I am the author of that part of the Gnus scoring
>> code. I have not had a chance to look at the proposed patch, but I agree
>> that some explanations would be useful. I can look it over this weekend.
>
> Thank you, your review is appreciated. Please do you happen to remember

I like the approach and tested them out. The legal-types change in
gnus-summary-increase-score is straightforward and makes sense to me. I
have one stylistic comment: (nthcdr 3 s) seems to be easier to read to
me than (cdddr s), but I have no strong opinions on that.

> why you used this form
>
> (string= "date" (nth 0 (assoc header gnus-header-index)))
>
> instead of this
>
> (string= header "date")

My suspicion is that this started out as a copy of the integer
comparison right above that code and I never cleaned it up. Yes, feel
free to simplify, I don't remember any good reason why it needs to pick
apart a list. It also seems perfectly fine to remove the (eq type
'after) etc. stuff, it's not necessary anymore.

The rest of the changes in gnus-summary-score-entry look good. I think
some more help text or additional documentation about the defaults would
be useful. It gets a bit confusing what you are prompted for. Having
said that, I like the idea of pulling a default date from the current
message, it just surprised me.

I also think I might have found a bug in how the dates are written out
to the SCORE file. I interactively increased the score in the order of
<, r, n, b, and n as you can see below. Only the b, a, and n entries get
converted to the list format with the un-evaluated gnus-time after
another entry is written. Meaning the second and third entry below, the
"before" and "at," looked just like the topmost "at" entry before the
following entry was written.

("date"
  ("20240501" nil nil at)
  (#("20240502" 0 1
     (gnus-time
      (26163 14832)))
   nil nil before)
  (#("20240503T145117" 0 1
     (gnus-time
      (26165 23637)))
   nil nil at)
  (".*T031840" nil nil r)
  (2 nil nil <)

> Thanks!

Hope this is useful!

-- 
Alex.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70134; Package emacs. (Thu, 09 May 2024 19:16:01 GMT) Full text and rfc822 format available.

Message #26 received at 70134 <at> debbugs.gnu.org (full text, mbox):

From: Jakub Ječmínek <kuba <at> kubajecminek.cz>
To: Alex Bochannek <alex <at> bochannek.com>
Cc: Eric Abrahamsen <eric <at> ericabrahamsen.net>, 70134 <at> debbugs.gnu.org,
 eliz <at> gnu.org, larsi <at> gnus.org, Richard Stallman <rms <at> gnu.org>
Subject: Re: bug#70134: [PATCH] Show all date options when adding Gnus scores
 interactively
Date: Thu, 09 May 2024 19:15:41 +0000
"Alex Bochannek" <alex <at> bochannek.com> writes:

> I finally had some time to look at these changes, apologies for the
> delay.

Thank you very much for your time and all your comments.

> I like the approach and tested them out. The legal-types change in
> gnus-summary-increase-score is straightforward and makes sense to me. I
> have one stylistic comment: (nthcdr 3 s) seems to be easier to read to
> me than (cdddr s), but I have no strong opinions on that.

Thank you, I used (nthcdr 3 s) instead.

> My suspicion is that this started out as a copy of the integer
> comparison right above that code and I never cleaned it up. Yes, feel
> free to simplify, I don't remember any good reason why it needs to pick
> apart a list. It also seems perfectly fine to remove the (eq type
> 'after) etc. stuff, it's not necessary anymore.

I've adjusted only the age scoring part (not the integer comparison)
because I did not studied that part of the code and there's still
possibility that it is needed somehow.

> The rest of the changes in gnus-summary-score-entry look good. I think
> some more help text or additional documentation about the defaults would
> be useful. It gets a bit confusing what you are prompted for. Having
> said that, I like the idea of pulling a default date from the current
> message, it just surprised me.

I've added a comment explaining the changes I made to the date
prompt. If you feel like the code needs more comments, please pinpoint
where and I will add them.

> I also think I might have found a bug in how the dates are written out
> to the SCORE file. I interactively increased the score in the order of
> <, r, n, b, and n as you can see below. Only the b, a, and n entries get
> converted to the list format with the un-evaluated gnus-time after
> another entry is written. Meaning the second and third entry below, the
> "before" and "at," looked just like the topmost "at" entry before the
> following entry was written.

I've found the reason why it happens and found a solution. The problem
is in the `gnus-date-get-time' macro. This macro accepts a single
argument - date - and returns a different one - time - with text
property added. However, this macro is written in such way that it
modifies the input argument as well. We can fix it by adding `copy-sequence'
function to the let form.

> Hope this is useful!

Very helpful yes.

I've also noticed that the change I proposed - moving the part of the
code which modifies match variable to the beginning of the
`gnus-summary-score-entry' function is a bad idea because we're
modifying input argument to the function. It would mean that if the user
called the function non interactively (without prompt):

(gnus-summary-score-entry "date" "55" '< 1000 nil)

then it would change "55" to different number based on article age. I've
moved that code back where it was. I attach the patch below.

Thank you,

Best

From 22c8bcc0b52b70baf9931d168a07fd69dbbd1e8c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jakub=20Je=C4=8Dm=C3=ADnek?= <kuba <at> kubajecminek.cz>
Date: Thu, 9 May 2024 20:33:58 +0200
Subject: [PATCH] Show all date options when adding Gnus scores interactively

* lisp/gnus/gnus-score.el (gnus-summary-increase-score): Rename
'char-to-type' variable to 'char-to-types' and bind all legal types
for date header.

* lisp/gnus/gnus-score.el (gnus-summary-score-entry): Provide better
default values for each scoring type and cast 'match' to number only
if necessary.
---
 lisp/gnus/gnus-score.el | 46 ++++++++++++++++++++---------------------
 lisp/gnus/gnus-util.el  |  2 +-
 2 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/lisp/gnus/gnus-score.el b/lisp/gnus/gnus-score.el
index 479b7496cf1..4e9e0083424 100644
--- a/lisp/gnus/gnus-score.el
+++ b/lisp/gnus/gnus-score.el
@@ -593,18 +593,18 @@ gnus-summary-increase-score
 	    (?d "date" nil nil date)
 	    (?f "followup" nil nil string)
 	    (?t "thread" "message-id" nil string)))
-	 (char-to-type
+	 (char-to-types
 	  '((?s s "substring" string)
 	    (?e e "exact string" string)
 	    (?f f "fuzzy string" string)
-	    (?r r "regexp string" string)
+	    (?r r "regexp string" string date)
 	    (?z s "substring" body-string)
 	    (?p r "regexp string" body-string)
 	    (?b before "before date" date)
 	    (?a after "after date" date)
 	    (?n at "this date" date)
-	    (?< < "less than number" number)
-	    (?> > "greater than number" number)
+	    (?< < "less than number" number date)
+	    (?> > "greater than number" number date)
 	    (?= = "equal to number" number)))
 	 (current-score-file gnus-current-score-file)
 	 (char-to-perm
@@ -652,10 +652,9 @@ gnus-summary-increase-score
 	  (let ((legal-types
 		 (delq nil
 		       (mapcar (lambda (s)
-				 (if (eq (nth 4 entry)
-					 (nth 3 s))
+				 (if (member (nth 4 entry) (nthcdr 3 s))
 				     s nil))
-			       char-to-type))))
+			       char-to-types))))
             (setq header-string
                   (format "%s header `%s' with match type (%s?): "
 			  (if increase "Increase" "Lower")
@@ -894,12 +893,16 @@ gnus-summary-score-entry
 			   header
 			   (if (< score 0) "lower" "raise"))
                    (cond ((numberp match) (int-to-string match))
+                         ;; Provide better defaults if we're scoring on date header
                          ((string= header "date")
-                          (int-to-string
-                           (-
-                            (/ (car (time-convert (current-time) 1)) 86400)
-                            (/ (car (time-convert (gnus-date-get-time match) 1))
-                               86400))))
+                          (if (or (eq type '<) (eq type '>))
+                              ;; Determine the time difference in days between today
+                              ;; and the article's date
+                              (format-seconds "%d"
+                                              (time-subtract
+                                               (current-time)
+                                               (gnus-date-get-time match)))
+                            (gnus-date-iso8601 match)))
                          (t match)))))
 
     ;; If this is an integer comparison, we transform from string to int.
@@ -909,16 +912,13 @@ gnus-summary-score-entry
       (set-text-properties 0 (length match) nil match))
 
     ;; Modify match and type for article age scoring.
-    (if (string= "date" (nth 0 (assoc header gnus-header-index)))
-	(let ((age (string-to-number match)))
-	  (if (or (< age 0)
-		  (string= "0" match))
-	      (user-error "Article age must be a positive number"))
-	  (setq match age
-		type (cond ((eq type 'after)
-			    '<)
-			   ((eq type 'before)
-			    '>)))))
+    (when (and (string= header "date")
+               (or (eq type '<) (eq type '>)))
+      (let ((age (string-to-number match)))
+        (if (or (< age 0)
+                (string= "0" match))
+            (user-error "Article age must be a positive number"))
+        (setq match age)))
 
     (unless (eq date 'now)
       ;; Add the score entry to the score file.
@@ -1806,7 +1806,7 @@ gnus-score-date
 	   ((eq type 'at)
 	    (setq match-func 'string=
 		  match (gnus-date-iso8601 (nth 0 kill))))
-	   ((eq type 'regexp)
+	   ((or (eq type 'regexp) (eq type 'r))
 	    (setq match-func 'string-match
 		  match (nth 0 kill)))
 	   (t (error "Invalid match type: %s" type)))
diff --git a/lisp/gnus/gnus-util.el b/lisp/gnus/gnus-util.el
index 0b0a9bbfc1d..6097f517be0 100644
--- a/lisp/gnus/gnus-util.el
+++ b/lisp/gnus/gnus-util.el
@@ -377,7 +377,7 @@ gnus-date-get-time
   "Convert DATE string to Emacs time.
 Cache the result as a text property stored in DATE."
   ;; Either return the cached value...
-  `(let ((d ,date))
+  `(let ((d (copy-sequence ,date)))
      (if (equal "" d)
 	 0
        (or (get-text-property 0 'gnus-time d)
-- 
2.34.1

-- 
Kuba Ječmínek (http://kubajecminek.cz)





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70134; Package emacs. (Fri, 10 May 2024 00:00:01 GMT) Full text and rfc822 format available.

Message #29 received at 70134 <at> debbugs.gnu.org (full text, mbox):

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: Alex Bochannek <alex <at> bochannek.com>
Cc: 70134 <at> debbugs.gnu.org,
 Jakub Ječmínek <kuba <at> kubajecminek.cz>, eliz <at> gnu.org,
 larsi <at> gnus.org, Richard Stallman <rms <at> gnu.org>
Subject: Re: bug#70134: [PATCH] Show all date options when adding Gnus
 scores interactively
Date: Thu, 09 May 2024 16:59:33 -0700
Jakub Ječmínek via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs <at> gnu.org> writes:

> "Alex Bochannek" <alex <at> bochannek.com> writes:
>
>> I finally had some time to look at these changes, apologies for the
>> delay.
>
> Thank you very much for your time and all your comments.

Thanks both to Alex for this response, and Jakub for your earlier
explanation of what's going on. I'll try to add some more docstrings
after this is resolved.

>> I like the approach and tested them out. The legal-types change in
>> gnus-summary-increase-score is straightforward and makes sense to me. I
>> have one stylistic comment: (nthcdr 3 s) seems to be easier to read to
>> me than (cdddr s), but I have no strong opinions on that.
>
> Thank you, I used (nthcdr 3 s) instead.
>
>> My suspicion is that this started out as a copy of the integer
>> comparison right above that code and I never cleaned it up. Yes, feel
>> free to simplify, I don't remember any good reason why it needs to pick
>> apart a list. It also seems perfectly fine to remove the (eq type
>> 'after) etc. stuff, it's not necessary anymore.
>
> I've adjusted only the age scoring part (not the integer comparison)
> because I did not studied that part of the code and there's still
> possibility that it is needed somehow.
>
>> The rest of the changes in gnus-summary-score-entry look good. I think
>> some more help text or additional documentation about the defaults would
>> be useful. It gets a bit confusing what you are prompted for. Having
>> said that, I like the idea of pulling a default date from the current
>> message, it just surprised me.
>
> I've added a comment explaining the changes I made to the date
> prompt. If you feel like the code needs more comments, please pinpoint
> where and I will add them.
>
>> I also think I might have found a bug in how the dates are written out
>> to the SCORE file. I interactively increased the score in the order of
>> <, r, n, b, and n as you can see below. Only the b, a, and n entries get
>> converted to the list format with the un-evaluated gnus-time after
>> another entry is written. Meaning the second and third entry below, the
>> "before" and "at," looked just like the topmost "at" entry before the
>> following entry was written.
>
> I've found the reason why it happens and found a solution. The problem
> is in the `gnus-date-get-time' macro. This macro accepts a single
> argument - date - and returns a different one - time - with text
> property added. However, this macro is written in such way that it
> modifies the input argument as well. We can fix it by adding `copy-sequence'
> function to the let form.

This is grim, thanks for finding it. I'm inclined to fix this first in a
stand-alone commit.

>> Hope this is useful!
>
> Very helpful yes.
>
> I've also noticed that the change I proposed - moving the part of the
> code which modifies match variable to the beginning of the
> `gnus-summary-score-entry' function is a bad idea because we're
> modifying input argument to the function. It would mean that if the user
> called the function non interactively (without prompt):
>
> (gnus-summary-score-entry "date" "55" '< 1000 nil)
>
> then it would change "55" to different number based on article age. I've
> moved that code back where it was. I attach the patch below.

Alex has commit access to Emacs -- Alex would you please commit and
close the bug once the two of you are happy with this?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70134; Package emacs. (Fri, 10 May 2024 20:05:02 GMT) Full text and rfc822 format available.

Message #32 received at 70134 <at> debbugs.gnu.org (full text, mbox):

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: Alex Bochannek <alex <at> bochannek.com>
Cc: 70134 <at> debbugs.gnu.org,
 Jakub Ječmínek <kuba <at> kubajecminek.cz>, eliz <at> gnu.org,
 larsi <at> gnus.org, Richard Stallman <rms <at> gnu.org>
Subject: Re: bug#70134: [PATCH] Show all date options when adding Gnus
 scores interactively
Date: Fri, 10 May 2024 13:04:39 -0700
Eric Abrahamsen <eric <at> ericabrahamsen.net> writes:

> Jakub Ječmínek via "Bug reports for GNU Emacs, the Swiss army knife of
> text editors" <bug-gnu-emacs <at> gnu.org> writes:
>
>> "Alex Bochannek" <alex <at> bochannek.com> writes:
>>
>>> I finally had some time to look at these changes, apologies for the
>>> delay.
>>
>> Thank you very much for your time and all your comments.
>
> Thanks both to Alex for this response, and Jakub for your earlier
> explanation of what's going on. I'll try to add some more docstrings
> after this is resolved.
>
>>> I like the approach and tested them out. The legal-types change in
>>> gnus-summary-increase-score is straightforward and makes sense to me. I
>>> have one stylistic comment: (nthcdr 3 s) seems to be easier to read to
>>> me than (cdddr s), but I have no strong opinions on that.
>>
>> Thank you, I used (nthcdr 3 s) instead.
>>
>>> My suspicion is that this started out as a copy of the integer
>>> comparison right above that code and I never cleaned it up. Yes, feel
>>> free to simplify, I don't remember any good reason why it needs to pick
>>> apart a list. It also seems perfectly fine to remove the (eq type
>>> 'after) etc. stuff, it's not necessary anymore.
>>
>> I've adjusted only the age scoring part (not the integer comparison)
>> because I did not studied that part of the code and there's still
>> possibility that it is needed somehow.
>>
>>> The rest of the changes in gnus-summary-score-entry look good. I think
>>> some more help text or additional documentation about the defaults would
>>> be useful. It gets a bit confusing what you are prompted for. Having
>>> said that, I like the idea of pulling a default date from the current
>>> message, it just surprised me.
>>
>> I've added a comment explaining the changes I made to the date
>> prompt. If you feel like the code needs more comments, please pinpoint
>> where and I will add them.
>>
>>> I also think I might have found a bug in how the dates are written out
>>> to the SCORE file. I interactively increased the score in the order of
>>> <, r, n, b, and n as you can see below. Only the b, a, and n entries get
>>> converted to the list format with the un-evaluated gnus-time after
>>> another entry is written. Meaning the second and third entry below, the
>>> "before" and "at," looked just like the topmost "at" entry before the
>>> following entry was written.
>>
>> I've found the reason why it happens and found a solution. The problem
>> is in the `gnus-date-get-time' macro. This macro accepts a single
>> argument - date - and returns a different one - time - with text
>> property added. However, this macro is written in such way that it
>> modifies the input argument as well. We can fix it by adding `copy-sequence'
>> function to the let form.
>
> This is grim, thanks for finding it. I'm inclined to fix this first in a
> stand-alone commit.

And sure enough, the modification of the string is the point -- it's a
cache! From gnus-sum.el:

; Since this is called not only to sort the top-level threads, but
; also in recursive sorts to order the articles within a thread, each
; article will be processed many times.  Thus it speeds things up
; quite a bit to use gnus-date-get-time, which caches the time value.
(defun gnus-thread-latest-date (thread)
  "Return the highest article date in THREAD."
  (apply #'max
	 (mapcar (lambda (header) (float-time
				   (gnus-date-get-time
				    (mail-header-date header))))
		 (flatten-tree thread))))

Can we strip properties around the call, maybe?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70134; Package emacs. (Fri, 10 May 2024 20:39:01 GMT) Full text and rfc822 format available.

Message #35 received at 70134 <at> debbugs.gnu.org (full text, mbox):

From: Jakub Ječmínek <kuba <at> kubajecminek.cz>
To: Eric Abrahamsen <eric <at> ericabrahamsen.net>
Cc: "70134 <at> debbugs.gnu.org" <70134 <at> debbugs.gnu.org>,
 Eli Zaretskii <eliz <at> gnu.org>, "larsi <at> gnus.org" <larsi <at> gnus.org>,
 Alex Bochannek <alex <at> bochannek.com>, Richard Stallman <rms <at> gnu.org>
Subject: Re: bug#70134: [PATCH] Show all date options when adding Gnus scores
 interactively
Date: Fri, 10 May 2024 20:38:07 +0000
[Message part 1 (text/plain, inline)]
Apologies for the formatting, I’m writing this reply from my phone.

> And sure enough, the modification of the string is the point -- it's a
> cache! From gnus-sum.el:
>
> ; Since this is called not only to sort the top-level threads, but
> ; also in recursive sorts to order the articles within a thread, each
> ; article will be processed many times. Thus it speeds things up
> ; quite a bit to use gnus-date-get-time, which caches the time value.
> (defun gnus-thread-latest-date (thread)
> "Return the highest article date in THREAD."
> (apply #'max
> (mapcar (lambda (header) (float-time
> (gnus-date-get-time
> (mail-header-date header))))
> (flatten-tree thread))))
>
> Can we strip properties around the call, maybe?

I don’t think there’s need to do that. The change that I proposed in the patch should remove the unintended side effect (and thus fix the bug pointed by Alex) while keeping the caching behaviour intact (‘gnus-thread-latest-date’ code above will work as expected).
[Message part 2 (text/html, inline)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70134; Package emacs. (Fri, 10 May 2024 21:29:02 GMT) Full text and rfc822 format available.

Message #38 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: bug#70134: [PATCH] Show all date options when adding Gnus scores
 interactively
Date: Fri, 10 May 2024 14:27:58 -0700
Jakub Ječmínek via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs <at> gnu.org> writes:

> Apologies for the formatting, I’m writing this reply from my phone.
>
>> And sure enough, the modification of the string is the point -- it's a
>> cache! From gnus-sum.el:
>>
>> ; Since this is called not only to sort the top-level threads, but
>> ; also in recursive sorts to order the articles within a thread, each
>> ; article will be processed many times. Thus it speeds things up
>> ; quite a bit to use gnus-date-get-time, which caches the time value.
>> (defun gnus-thread-latest-date (thread)
>> "Return the highest article date in THREAD."
>> (apply #'max
>> (mapcar (lambda (header) (float-time
>> (gnus-date-get-time
>> (mail-header-date header))))
>> (flatten-tree thread))))
>>
>> Can we strip properties around the call, maybe?
>
> I don’t think there’s need to do that. The change that I proposed in
> the patch should remove the unintended side effect (and thus fix the
> bug pointed by Alex) while keeping the caching behaviour intact
> (‘gnus-thread-latest-date’ code above will work as expected).

Okay, thanks. I won't mess with it, then.





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70134; Package emacs. (Tue, 14 May 2024 02:02:01 GMT) Full text and rfc822 format available.

Message #41 received at 70134 <at> debbugs.gnu.org (full text, mbox):

From: Alex Bochannek <alex <at> bochannek.com>
To: Jakub Ječmínek <kuba <at> kubajecminek.cz>
Cc: Eric Abrahamsen <eric <at> ericabrahamsen.net>,
 "70134 <at> debbugs.gnu.org" <70134 <at> debbugs.gnu.org>, Eli Zaretskii <eliz <at> gnu.org>,
 "larsi <at> gnus.org" <larsi <at> gnus.org>, Richard Stallman <rms <at> gnu.org>
Subject: Re: bug#70134: [PATCH] Show all date options when adding Gnus
 scores interactively
Date: Mon, 13 May 2024 19:00:01 -0700
Jakub,

I see that your changes to gnus-score.el have been committed, but not
the one you proposed to gnus-util.el yet. Let me know if you need me to
test anything!

-- 
Alex.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70134; Package emacs. (Tue, 14 May 2024 14:53:01 GMT) Full text and rfc822 format available.

Message #44 received at 70134 <at> debbugs.gnu.org (full text, mbox):

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: Alex Bochannek <alex <at> bochannek.com>
Cc: "70134 <at> debbugs.gnu.org" <70134 <at> debbugs.gnu.org>,
 Jakub Ječmínek <kuba <at> kubajecminek.cz>,
 Eli Zaretskii <eliz <at> gnu.org>, "larsi <at> gnus.org" <larsi <at> gnus.org>,
 Richard Stallman <rms <at> gnu.org>
Subject: Re: bug#70134: [PATCH] Show all date options when adding Gnus
 scores interactively
Date: Tue, 14 May 2024 07:52:06 -0700
Alex Bochannek <alex <at> bochannek.com> writes:

> Jakub,
>
> I see that your changes to gnus-score.el have been committed, but not
> the one you proposed to gnus-util.el yet. Let me know if you need me to
> test anything!

The commit from February was a different issue in the same area --
nothing from this bug report has been committed yet.

Thanks,
Eric




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70134; Package emacs. (Tue, 14 May 2024 18:45:01 GMT) Full text and rfc822 format available.

Message #47 received at 70134 <at> debbugs.gnu.org (full text, mbox):

From: Alex Bochannek <alex <at> bochannek.com>
To: Eric Abrahamsen <eric <at> ericabrahamsen.net>
Cc: "70134 <at> debbugs.gnu.org" <70134 <at> debbugs.gnu.org>,
 Jakub Ječmínek <kuba <at> kubajecminek.cz>,
 Eli Zaretskii <eliz <at> gnu.org>, "larsi <at> gnus.org" <larsi <at> gnus.org>,
 Richard Stallman <rms <at> gnu.org>
Subject: Re: bug#70134: [PATCH] Show all date options when adding Gnus
 scores interactively
Date: Tue, 14 May 2024 11:43:54 -0700
Eric,

Eric Abrahamsen <eric <at> ericabrahamsen.net> writes:

> Alex Bochannek <alex <at> bochannek.com> writes:
>
>> Jakub,
>>
>> I see that your changes to gnus-score.el have been committed, but not
>> the one you proposed to gnus-util.el yet. Let me know if you need me to
>> test anything!
>
> The commit from February was a different issue in the same area --
> nothing from this bug report has been committed yet.

You are absolutely correct, of course, I mistook the log entry and
changes in that commit for what we are discussing here.

I have done some more testing with a fresh build and the most recent
patch works as Jakub intends. I would like to suggest to add a change to
(gnus) Summary Score Commands to include the new match types. Otherwise,
I believe this patch is good to go.

I don't think I have commit rights, by the way (usually larsi or eliz
did them for me), but I would be more than happy to do so if someone can
point me at instructions.

diff --git a/doc/misc/gnus.texi b/doc/misc/gnus.texi
index c5e4c885ccf..56f259db9a1 100644
--- a/doc/misc/gnus.texi
+++ b/doc/misc/gnus.texi
@@ -20093,6 +20093,9 @@ Summary Score Commands
 @item date
 @table @kbd
 
+@item r
+Regexp matching.
+
 @item b
 Before date.
 
@@ -20101,6 +20104,12 @@ Summary Score Commands
 
 @item n
 This date.
+
+@item <
+Less than days.
+
+@item >
+Greater than days.
 @end table
 
 @item number


-- 
Alex.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70134; Package emacs. (Tue, 14 May 2024 19:58:03 GMT) Full text and rfc822 format available.

Message #50 received at 70134 <at> debbugs.gnu.org (full text, mbox):

From: Jakub Ječmínek <kuba <at> kubajecminek.cz>
To: Alex Bochannek <alex <at> bochannek.com>
Cc: Eric Abrahamsen <eric <at> ericabrahamsen.net>,
 "70134 <at> debbugs.gnu.org" <70134 <at> debbugs.gnu.org>, Eli Zaretskii <eliz <at> gnu.org>,
 "larsi <at> gnus.org" <larsi <at> gnus.org>, Richard Stallman <rms <at> gnu.org>
Subject: Re: bug#70134: [PATCH] Show all date options when adding Gnus scores
 interactively
Date: Tue, 14 May 2024 19:57:20 +0000
"Alex Bochannek" <alex <at> bochannek.com> writes:
> I have done some more testing with a fresh build and the most recent
> patch works as Jakub intends. I would like to suggest to add a change to
> (gnus) Summary Score Commands to include the new match types. Otherwise,
> I believe this patch is good to go.

Thank you very much!

> diff --git a/doc/misc/gnus.texi b/doc/misc/gnus.texi
> index c5e4c885ccf..56f259db9a1 100644
> --- a/doc/misc/gnus.texi
> +++ b/doc/misc/gnus.texi
> @@ -20093,6 +20093,9 @@ Summary Score Commands
>  @item date
>  @table @kbd
>
> +@item r
> +Regexp matching.
> +
>  @item b
>  Before date.
>
> @@ -20101,6 +20104,12 @@ Summary Score Commands
>
>  @item n
>  This date.
> +
> +@item <
> +Less than days.
> +
> +@item >
> +Greater than days.
>  @end table
>
>  @item number
>
>
> --

I've applied the patch and added co-authored-by message.

From d69cf006f2e42da61714a179eb48a345a082fcf9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jakub=20Je=C4=8Dm=C3=ADnek?= <kuba <at> kubajecminek.cz>
Date: Thu, 9 May 2024 20:33:58 +0200
Subject: [PATCH] Show all date options when adding Gnus scores interactively

* lisp/gnus/gnus-score.el (gnus-summary-increase-score): Rename
'char-to-type' variable to 'char-to-types' and bind all legal types
for date header.

* lisp/gnus/gnus-score.el (gnus-summary-score-entry): Provide better
default values for each scoring type and cast 'match' to number only
if necessary.

Co-authored-by: Alex Bochannek <alex <at> bochannek.com>
---
 doc/misc/gnus.texi      |  9 ++++++++
 lisp/gnus/gnus-score.el | 46 ++++++++++++++++++++---------------------
 lisp/gnus/gnus-util.el  |  2 +-
 3 files changed, 33 insertions(+), 24 deletions(-)

diff --git a/doc/misc/gnus.texi b/doc/misc/gnus.texi
index c5e4c885ccf..56f259db9a1 100644
--- a/doc/misc/gnus.texi
+++ b/doc/misc/gnus.texi
@@ -20093,6 +20093,9 @@ Summary Score Commands
 @item date
 @table @kbd
 
+@item r
+Regexp matching.
+
 @item b
 Before date.
 
@@ -20101,6 +20104,12 @@ Summary Score Commands
 
 @item n
 This date.
+
+@item <
+Less than days.
+
+@item >
+Greater than days.
 @end table
 
 @item number
diff --git a/lisp/gnus/gnus-score.el b/lisp/gnus/gnus-score.el
index 479b7496cf1..4e9e0083424 100644
--- a/lisp/gnus/gnus-score.el
+++ b/lisp/gnus/gnus-score.el
@@ -593,18 +593,18 @@ gnus-summary-increase-score
 	    (?d "date" nil nil date)
 	    (?f "followup" nil nil string)
 	    (?t "thread" "message-id" nil string)))
-	 (char-to-type
+	 (char-to-types
 	  '((?s s "substring" string)
 	    (?e e "exact string" string)
 	    (?f f "fuzzy string" string)
-	    (?r r "regexp string" string)
+	    (?r r "regexp string" string date)
 	    (?z s "substring" body-string)
 	    (?p r "regexp string" body-string)
 	    (?b before "before date" date)
 	    (?a after "after date" date)
 	    (?n at "this date" date)
-	    (?< < "less than number" number)
-	    (?> > "greater than number" number)
+	    (?< < "less than number" number date)
+	    (?> > "greater than number" number date)
 	    (?= = "equal to number" number)))
 	 (current-score-file gnus-current-score-file)
 	 (char-to-perm
@@ -652,10 +652,9 @@ gnus-summary-increase-score
 	  (let ((legal-types
 		 (delq nil
 		       (mapcar (lambda (s)
-				 (if (eq (nth 4 entry)
-					 (nth 3 s))
+				 (if (member (nth 4 entry) (nthcdr 3 s))
 				     s nil))
-			       char-to-type))))
+			       char-to-types))))
             (setq header-string
                   (format "%s header `%s' with match type (%s?): "
 			  (if increase "Increase" "Lower")
@@ -894,12 +893,16 @@ gnus-summary-score-entry
 			   header
 			   (if (< score 0) "lower" "raise"))
                    (cond ((numberp match) (int-to-string match))
+                         ;; Provide better defaults if we're scoring on date header
                          ((string= header "date")
-                          (int-to-string
-                           (-
-                            (/ (car (time-convert (current-time) 1)) 86400)
-                            (/ (car (time-convert (gnus-date-get-time match) 1))
-                               86400))))
+                          (if (or (eq type '<) (eq type '>))
+                              ;; Determine the time difference in days between today
+                              ;; and the article's date
+                              (format-seconds "%d"
+                                              (time-subtract
+                                               (current-time)
+                                               (gnus-date-get-time match)))
+                            (gnus-date-iso8601 match)))
                          (t match)))))
 
     ;; If this is an integer comparison, we transform from string to int.
@@ -909,16 +912,13 @@ gnus-summary-score-entry
       (set-text-properties 0 (length match) nil match))
 
     ;; Modify match and type for article age scoring.
-    (if (string= "date" (nth 0 (assoc header gnus-header-index)))
-	(let ((age (string-to-number match)))
-	  (if (or (< age 0)
-		  (string= "0" match))
-	      (user-error "Article age must be a positive number"))
-	  (setq match age
-		type (cond ((eq type 'after)
-			    '<)
-			   ((eq type 'before)
-			    '>)))))
+    (when (and (string= header "date")
+               (or (eq type '<) (eq type '>)))
+      (let ((age (string-to-number match)))
+        (if (or (< age 0)
+                (string= "0" match))
+            (user-error "Article age must be a positive number"))
+        (setq match age)))
 
     (unless (eq date 'now)
       ;; Add the score entry to the score file.
@@ -1806,7 +1806,7 @@ gnus-score-date
 	   ((eq type 'at)
 	    (setq match-func 'string=
 		  match (gnus-date-iso8601 (nth 0 kill))))
-	   ((eq type 'regexp)
+	   ((or (eq type 'regexp) (eq type 'r))
 	    (setq match-func 'string-match
 		  match (nth 0 kill)))
 	   (t (error "Invalid match type: %s" type)))
diff --git a/lisp/gnus/gnus-util.el b/lisp/gnus/gnus-util.el
index 0b0a9bbfc1d..6097f517be0 100644
--- a/lisp/gnus/gnus-util.el
+++ b/lisp/gnus/gnus-util.el
@@ -377,7 +377,7 @@ gnus-date-get-time
   "Convert DATE string to Emacs time.
 Cache the result as a text property stored in DATE."
   ;; Either return the cached value...
-  `(let ((d ,date))
+  `(let ((d (copy-sequence ,date)))
      (if (equal "" d)
 	 0
        (or (get-text-property 0 'gnus-time d)
-- 
2.34.1

Best

-- 
Kuba Ječmínek (http://kubajecminek.cz)





This bug report was last modified 1 day ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.