bug#66395: 28.2; Todo-mode locks up when trying to edit an entry

[Stephen Berman <stephen.berman@gmx.net>]

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

merge 55284 66395
thanks

On Sat, 07 Oct 2023 23:27:46 +0200 Stephen Berman <stephen.berman@gmx.net> wrote:

> On Sat, 7 Oct 2023 19:06:55 +0000 "Nathan R. DeGruchy" <nathan@degruchy.org> wrote:
>
>> I am trying to explore using 'todo-mode' as a todo list and while I can
>> see and create entries in todo-mode using the normal functions, trying
>> to edit an item seems to cause emacs to soft-lock. I can reproduce this
>> in a config-less emacs via `emacs -Q`.
>>
>> Basically, I have a todo-file at $HOME/.config/emacs/todo/tasks.todo,
>> this was created when using `todo-show` initially. The contents are not
>> very complex:
>>
>> (("Emacs" . [1 0 0 0]) ("Home" . [1 0 0 0]))
>> --==-- Emacs
>> [2023-10-07] Learn Todo Mode
>>
>> ==--== DONE
>> --==-- Home
>> [2023-10-07] Get new wiper blades
>>
>> ==--== DONE
>>
>> When on either of the items, if I hit 'e' to edit them, it causes emacs
>> to lock up, specifically around `todo-done-item-p()`. I found this out
>> by enabling `toggle-debug-on-quit`, reproducing the error, and the
>> C-g'ing out of the loop/lockup. I also tried to trace through the
>> todo-edit-item with edebug-defun. Stepping through, it seems to reach
>> the same predicate function and ... stop.
>>
>> I'm not sure where to go from here.
>
> On Sat, 07 Oct 2023 14:37:00 -0500 LdBeth <andpuke@foxmail.com> wrote:
>
>> As we have discussed on IRC, the nonstandard timestamp format is the cause.
>
> Yes.  This is a duplicate of bug#55284.  At the time that bug was
> reported, I didn't have time to try fixing it (being the todo-mode
> maintainer), and later, unfortunately, I forgot about it.  I'll try to
> look into it soon, but, as I noted in that bug thread, I think it's not
> easy to fix.  One issue is that todo-mode basically employs the same
> handling of date formats as diary-lib.el, and the ISO date style causes
> problems there too, see bug#55286.

I've finally attended to this bug.  The problem is due to using a faulty
regular expression for matching todo date headers in the ISO format.
There was a related bug report years ago about a diary display problem
with the ISO format (bug#8583), and the fix for that (whose author
conceded it was "ugly") points the way to a fix for todo-mode, see the
attached patch (which I have to concede is even uglier and more
complicated than the fix for bug#8583, but I couldn't find a better
way).  (I actually had been aware of the diary bug and the fix, but
didn't try to apply it to todo-mode back then; I don't remember exactly
why but I guess because I concluded it was not straightforward to adapt
the fix to todo-mode and since I didn't use the ISO format myself I
lacked motivation, also because there had been no todo-mode bug reports
about it -- until bug#55286 (and now the current bug), but by then I had
forgotten about bug#8583.)

>> The hang is cause by the while loop `todo-item-start' not properly handle fail
>> case, however.
>>
>> This patch would at least fix the hang.
>>
>> ---
>> LdBeth
>>
>> --- todo-mode.el.old	2023-10-07 14:28:59.000000000 -0500
>> +++ todo-mode.el	2023-10-07 14:30:20.000000000 -0500
>> @@ -5242,8 +5242,8 @@
>>  	   ;; Buffer is widened.
>>  	   (looking-at (regexp-quote todo-category-beg)))
>>      (goto-char (line-beginning-position))
>> -    (while (not (looking-at todo-item-start))
>> -      (forward-line -1))
>> +    (while (and (not (looking-at todo-item-start))
>> +                (= (forward-line -1) 0)))
>>      (point)))
>>
>>  (defun todo-item-end ()
>
> Thanks.  Even though this doesn't eliminate other problems with using
> the ISO date style in todo-mode (or in the Emacs Diary), since it does
> prevent the infinite loop here, it may be a good stopgap.

With the attached patch I think this stopgap should be unnecessary: the
while-loop should no longer fail when the ISO date format is used
(barring any bugs in the patch, and if there are, it should fail so they
can be found and fixed).

However, using the ISO date format in todo-mode does necessitate
additional changes, in the code for editing todo item headers, which the
attached patch also includes.

Although with this patch todo-mode now supports the ISO date format,
there remains a problem.  Todo mode (like the Emacs Diary) uses the date
format specified by calendar-date-display-form, which is a user option
that has three default styles (American, European and ISO), which you
can change by customizing `calendar-date-style' or by executing
`calendar-set-date-style'.  But if you change it, that breaks using
todo-mode with any existing todo file that was created using a different
date format, in the same way that trying to use the ISO format with
todo-mode fails without the attached patch (i.e., the current bug).  (As
noted above with reference to bug#55286, the diary is subject to similar
breakage.)

After I came up with the attached patch to support the ISO date format
in todo-mode, I started tackling the issue of changing the date format,
and I now have what I think is a viable solution.  However, to implement
it I've had to make extensive changes in the todo-mode code, and also to
part of the internal structure of todo files (but not to the UI), and
I'm still testing these changes.  They include essentially the changes
in the attached patch (though partly in a more general form).
Therefore, what I'd like to do first is install this patch, after
waiting several days for any feedback.  I'll open a new bug to post and
discuss the more extensive changes I'd like to make.

Steve Berman


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Todo mode support for ISO date format --]
[-- Type: text/x-patch, Size: 5786 bytes --]

diff --git a/lisp/calendar/todo-mode.el b/lisp/calendar/todo-mode.el
index 093ea0e22b6..5354a65799d 100644
--- a/lisp/calendar/todo-mode.el
+++ b/lisp/calendar/todo-mode.el
@@ -189,20 +189,53 @@ todo-month-abbrev-array
   "Array of abbreviated month names, in order.
 The final element is \"*\", indicating an unspecified month.")

+(defconst todo--date-pattern-groups
+  (pcase calendar-date-style
+          ('american '((monthname . "6") (month . "7") (day . "8") (year . "9")))
+          ('european '((day . "6") (monthname . "7") (month . "8") (year . "9")))
+          ('iso '((year . "6") (monthname . "7") (month . "8") (day . "9"))))
+  "Alist for grouping date components in `todo-date-pattern'.")
+
 (defconst todo-date-pattern
-  (let ((dayname (diary-name-pattern calendar-day-name-array nil t)))
-    (concat "\\(?4:\\(?5:" dayname "\\)\\|"
-	    (calendar-dlet
-                ((dayname)
-		 (monthname (format "\\(?6:%s\\)" (diary-name-pattern
-						   todo-month-name-array
-						   todo-month-abbrev-array)))
-		 (month "\\(?7:[0-9]+\\|\\*\\)")
-		 (day "\\(?8:[0-9]+\\|\\*\\)")
-		 (year "-?\\(?9:[0-9]+\\|\\*\\)"))
-	      (mapconcat #'eval calendar-date-display-form))
-	    "\\)"))
-  "Regular expression matching a todo item date header.")
+  (let* ((dayname (diary-name-pattern calendar-day-name-array nil t))
+         (d (concat "\\(?" (alist-get 'day todo--date-pattern-groups)
+                    ":[0-9]+\\|\\*\\)"))
+         (mn (format (concat "\\(?" (alist-get 'monthname
+                                               todo--date-pattern-groups)
+                             ":%s\\)")
+                     (diary-name-pattern todo-month-name-array
+                                         todo-month-abbrev-array)))
+         (m (concat "\\(?" (alist-get 'month todo--date-pattern-groups)
+                    ":[0-9]+\\|\\*\\)"))
+         (y (concat "\\(?" (alist-get 'year todo--date-pattern-groups)
+                    ":[0-9]+\\|\\*\\)"))
+         (dd "1111111")
+         (mm "2222222")
+         (yy "3333333")
+         (s (concat "\\(?4:\\(?5:" dayname "\\)\\|"
+	            (calendar-dlet
+                        ((dayname)
+		         (monthname mn)
+		         (year yy)
+		         (month mm)
+		         (day dd))
+                      (mapconcat #'eval calendar-date-display-form))
+	            "\\)")))
+    ;; The default value of calendar-iso-date-display-form calls
+    ;; `string-to-number' on the values of `month' and `day', so we
+    ;; gave them placeholder values above and now replace these with
+    ;; the necessary regexps with appropriately numbered groups, because
+    ;; `todo-edit-item--header' uses these groups.
+    (when (string-match dd s nil t)
+      (setq s (string-replace dd d s)))
+    (when (string-match mm s nil t)
+      (setq s (string-replace mm m s)))
+    (when (string-match yy s nil t)
+      (setq s (string-replace yy y s)))
+    s)
+  "Regular expression matching a todo item date header.
+The value of `calendar-date-display-form' determines the form of
+the date header.")

 ;; By itself this matches anything, because of the `?'; however, it's only
 ;; used in the context of `todo-date-pattern' (but Emacs Lisp lacks
@@ -2349,11 +2382,19 @@ todo-edit-item--header
 				     (regexp-quote todo-nondiary-end) "?")
 			     (line-end-position) t)
 	  (let* ((otime (match-string-no-properties 2))
-		 (odayname (match-string-no-properties 5))
-		 (omonthname (match-string-no-properties 6))
-		 (omonth (match-string-no-properties 7))
-		 (oday (match-string-no-properties 8))
-		 (oyear (match-string-no-properties 9))
+ 		 (odayname (match-string-no-properties 5))
+                 (mngroup (string-to-number
+                           (alist-get 'monthname todo--date-pattern-groups)))
+		 (omonthname (match-string-no-properties mngroup))
+                 (mgroup (string-to-number
+                          (alist-get 'month todo--date-pattern-groups)))
+		 (omonth (match-string-no-properties mgroup))
+                 (dgroup (string-to-number
+                          (alist-get 'day todo--date-pattern-groups)))
+		 (oday (match-string-no-properties dgroup))
+                 (ygroup (string-to-number
+                          (alist-get 'year todo--date-pattern-groups)))
+		 (oyear (match-string-no-properties ygroup))
 		 (tmn-array todo-month-name-array)
 		 (mlist (append tmn-array nil))
 		 (tma-array todo-month-abbrev-array)
@@ -2399,11 +2440,23 @@ todo-edit-item--header
 		 ((eq what 'month)
 		  (setf day oday
 			year oyear
-			(if (memq 'month calendar-date-display-form)
+                        ;; With default ISO style, 'month is in a
+                        ;; sublist of c-d-d-f, so we flatten it.
+			(if (memq 'month (flatten-tree
+                                          calendar-date-display-form))
 			    month
 			  monthname)
 			(cond ((not current-prefix-arg)
-			       (todo-read-date 'month))
+			       (let ((nmonth (todo-read-date 'month)))
+                                 ;; If old month is given as a number,
+                                 ;; have to convert new month name to
+                                 ;; the corresponding number.
+                                 (when omonth
+                                   (setq nmonth
+                                         (number-to-string
+                                          (1+ (seq-position tma-array
+                                                            nmonth)))))
+                                 nmonth))
 			      ((or (string= omonth "*") (= mm 13))
 			       (user-error "Cannot increment *"))
 			      (t