* bug#63811: 29.0.91; todo-mode item editing bugs
@ 2023-05-31 9:12 Stephen Berman
2023-05-31 12:31 ` Eli Zaretskii
0 siblings, 1 reply; 3+ messages in thread
From: Stephen Berman @ 2023-05-31 9:12 UTC (permalink / raw)
To: 63811
[-- Attachment #1: Type: text/plain, Size: 4352 bytes --]
I have encountered several bugs in todo-mode when creating and editing
todo items. I have fixes for them and since I'm the todo-mode
maintainer I intend to push them to the Savannah repo when I get a bug
number and clearance for which branch to push to. The bugs are
long-standing, at least since Emacs 28 and probably earlier. The fixes
touch no code outside of todo-mode.el and seem low-risk; in my testing
they work as expected. I've attached a diff against emacs-29 below (the
todo-mode.el files in current emacs-29 and master are identical).
One of the bugs is that, when editing an item's date-time header string,
which uses replace-match, the match data can get clobbered, e.g. by
using dabbrev-expand while editing. (In bug#42976 I had fixed the same
bug just for editing a done item comment, but at the time didn't notice
that it can also happen when editing the item header string.)
The other bugs all have to do with the fact that you can switch from the
buffer in which an item is entered or edited (mainly the minibuffer, but
for editing also a special indirect buffer of the todo-mode buffer) to
the todo-mode buffer and there, move point to another item (with `n' or
`p') or to another todo category in the todo file (with `f' or `b').
This possibility is useful in order, for example, to copy text from
another item to use in the item you are creating or editing (you could,
of course, do that before invoking the item insertion or editing
command, then there's no problem). But when you move point in this way
and then switch back to the editing buffer without moving point back to
where it was when you invoked the item insertion or editing command,
this can cause problems ranging from relatively harmless though
annoying, such as misplacement of the inserted or edited item (you can
always relocate the item later in todo-mode, when you notice the
misplacement), to serious, such as unintentionally replacing data and
even corruption of the todo-mode file format.
The latter can happen in three ways:
(1) Type `ee' on an item in todo-mode to edit it in the minibuffer and
before typing RET to complete the edit, switch back to the todo-mode
buffer, type `f' to display the next category in the todo file, then
switch back to the minibuffer and complete the edit. The edited item
does not replace the item on which `ee' was invoked but instead is
entered into the category moved to as the first item, replacing the item
that was there before.
(2) In a todo category that has done items, type `ih' ("insert here") to
insert a new item into the todo buffer on the line at point, and before
completing the item (by typing RET in the minibuffer), switch to the
todo-mode buffer, display the done items (`v'), move point (e.g. with
`n') to one of the done items, then switch back to the minibuffer and
complete with RET. The item will be inserted in the done items section
but it doesn't have the form of a done item, so you can't repair the
mistake using todo-mode commands, and also the calculated category
counts of todo and done items now conflict with the resulting
distribution of the items in the category.
(3) The todo-mode buffer is read-only most of the time and is supposed
to be changed only by using todo-mode commands, which temporarily make
it writeable. However, the item editing commands `ee' (for item text)
and `ec' (for done item comments) currently already make the todo-mode
buffer writeable when the minibuffer is being used for editing, and if
you then switch to the todo-mode buffer, you can corrupt it by inserting
newlines or yanking arbitrary text (but self-inserting keys remain
disabled in todo-mode).
In GNU Emacs 29.0.91 (build 1, x86_64-pc-linux-gnu, GTK+ Version
3.24.37, cairo version 1.17.6) of 2023-05-27 built on strobelfssd
Repository revision: 5e7c826bfa5cb7459f5b162b498af1c57c4578e6
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12101008
System Description: Linux From Scratch r11.3-100-systemd
Configured using:
'configure -C --with-xwidgets 'CFLAGS=-Og -g3''
Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBSYSTEMD LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG RSVG
SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS WEBP X11 XDBE XIM
XINPUT2 XPM XWIDGETS GTK3 ZLIB
[-- Attachment #2: commit message --]
[-- Type: text/plain, Size: 955 bytes --]
2023-05-31 Stephen Berman <stephen.berman@gmx.net>
Fix several todo-mode.el item editing bugs (bug#)
* lisp/calendar/todo-mode.el (todo-insert-item--basic): With
insertion type 'here', ensure item is inserted on the todo-mode
line where the command was invoked.
(todo-edit-item--cat, todo-edit-item--pos): New variables.
(todo-edit-item--text): Restrict the scope of nil-valued
buffer-read-only to the functions that change buffer text. If
user moved point while editing a single-line todo item or a done
item comment, or while inserting a done item comment, restore
point, and for comments, make sure the done items section is
displayed. For multiline items, set the new variables so
todo-edit-quit can use them.
(todo-edit-quit): Use the values of the new variables to restore
point in the todo-mode buffer if it had been moved while editing.
(todo-edit-item--header): Avoid clobbering match data when editing
a todo item header.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: todo-mode.el patch --]
[-- Type: text/x-patch, Size: 14935 bytes --]
diff --git a/lisp/calendar/todo-mode.el b/lisp/calendar/todo-mode.el
index 671210f3ee8..35cac5d7310 100644
--- a/lisp/calendar/todo-mode.el
+++ b/lisp/calendar/todo-mode.el
@@ -1985,7 +1985,13 @@ todo-insert-item--basic
(setq done-only t)
(todo-toggle-view-done-only))
(if here
- (todo-insert-with-overlays new-item)
+ (progn
+ ;; Ensure item is inserted where command was invoked.
+ (unless (= (point) opoint)
+ (todo-category-number ocat)
+ (todo-category-select)
+ (goto-char opoint))
+ (todo-insert-with-overlays new-item))
(todo-set-item-priority new-item cat t))
(setq item-added t))
;; If user cancels before setting priority, restore
@@ -2119,6 +2125,9 @@ todo-edit-item
((or marked (todo-item-string))
(todo-edit-item--next-key 'todo arg)))))
+(defvar todo-edit-item--cat nil)
+(defvar todo-edit-item--pos nil)
+
(defun todo-edit-item--text (&optional arg)
"Function providing the text editing facilities of `todo-edit-item'."
(let ((full-item (todo-item-string)))
@@ -2127,6 +2136,7 @@ todo-edit-item--text
;; 1+ signals an error, so just make this a noop.
(when full-item
(let* ((opoint (point))
+ (ocat (todo-current-category))
(start (todo-item-start))
(end (save-excursion (todo-item-end)))
(item-beg (progn
@@ -2151,8 +2161,7 @@ todo-edit-item--text
(concat " \\[" (regexp-quote todo-comment-string)
": \\([^]]+\\)\\]")
end t)))
- (prompt (if comment "Edit comment: " "Enter a comment: "))
- (buffer-read-only nil))
+ (prompt (if comment "Edit comment: " "Enter a comment: ")))
;; When there are marked items, user can invoke todo-edit-item
;; even if point is not on an item, but text editing only
;; applies to the item at point.
@@ -2170,22 +2179,43 @@ todo-edit-item--text
end t)
(if comment-delete
(when (todo-y-or-n-p "Delete comment? ")
- (delete-region (match-beginning 0) (match-end 0)))
- (replace-match (save-match-data
- (read-string prompt
- (cons (match-string 1) 1)))
- nil nil nil 1))
+ (let ((buffer-read-only nil))
+ (delete-region (match-beginning 0) (match-end 0))))
+ (let ((buffer-read-only nil))
+ (replace-match (save-match-data
+ (prog1 (let ((buffer-read-only t))
+ (read-string
+ prompt
+ (cons (match-string 1) 1)))
+ ;; If user moved point while editing
+ ;; a comment, restore it and ensure
+ ;; done items section is displayed.
+ (unless (= (point) opoint)
+ (todo-category-number ocat)
+ (let ((todo-show-with-done t))
+ (todo-category-select)
+ (goto-char opoint)))))
+ nil nil nil 1)))
(if comment-delete
(user-error "There is no comment to delete")
- (insert " [" todo-comment-string ": "
- (prog1 (read-string prompt)
- ;; If user moved point during editing,
- ;; make sure it moves back.
- (goto-char opoint)
- (todo-item-end))
- "]")))))
+ (let ((buffer-read-only nil))
+ (insert " [" todo-comment-string ": "
+ (prog1 (let ((buffer-read-only t))
+ (read-string prompt))
+ ;; If user moved point while inserting a
+ ;; comment, restore it and ensure done items
+ ;; section is displayed.
+ (unless (= (point) opoint)
+ (todo-category-number ocat)
+ (let ((todo-show-with-done t))
+ (todo-category-select)
+ (goto-char opoint)))
+ (todo-item-end))
+ "]"))))))
(multiline
(let ((buf todo-edit-buffer))
+ (setq todo-edit-item--cat ocat)
+ (setq todo-edit-item--pos opoint)
(set-window-buffer (selected-window)
(set-buffer (make-indirect-buffer
(buffer-name) buf)))
@@ -2208,10 +2238,14 @@ todo-edit-item--text
;; Ensure lines following hard newlines are indented.
(setq new (replace-regexp-in-string "\\(\n\\)[^[:blank:]]"
"\n\t" new nil nil 1))
- ;; If user moved point during editing, make sure it moves back.
- (goto-char opoint)
- (todo-remove-item)
- (todo-insert-with-overlays new)
+ ;; If user moved point while editing item, restore it.
+ (unless (= (point) opoint)
+ (todo-category-number ocat)
+ (todo-category-select)
+ (goto-char opoint))
+ (let ((buffer-read-only nil))
+ (todo-remove-item)
+ (todo-insert-with-overlays new))
(move-to-column item-beg)))))))))
(defun todo-edit-quit ()
@@ -2243,6 +2277,9 @@ todo-edit-quit
(kill-buffer)
(unless (eq (current-buffer) buf)
(set-window-buffer (selected-window) (set-buffer buf)))
+ (todo-category-number todo-edit-item--cat)
+ (todo-category-select)
+ (goto-char todo-edit-item--pos)
(if transient-mark-mode (deactivate-mark)))
;; We got here via `F e'.
(when (todo-check-format)
@@ -2315,117 +2352,118 @@ todo-edit-item--header
;; If there are marked items, use only the first to set
;; header changes, and apply these to all marked items.
(when first
- (cond
- ((eq what 'date)
- (setq ndate (todo-read-date)))
- ((eq what 'calendar)
- (setq ndate (save-match-data (todo-set-date-from-calendar))))
- ((eq what 'today)
- (setq ndate (calendar-date-string (calendar-current-date) t t)))
- ((eq what 'dayname)
- (setq ndate (todo-read-dayname)))
- ((eq what 'time)
- (setq ntime (save-match-data (todo-read-time)))
- (when (> (length ntime) 0)
- (setq ntime (concat " " ntime))))
- ;; When date string consists only of a day name,
- ;; passing other date components is a noop.
- ((and odayname (memq what '(year month day))))
- ((eq what 'year)
- (setq day oday
- monthname omonthname
- month omonth
- year (cond ((not current-prefix-arg)
- (todo-read-date 'year))
- ((string= oyear "*")
- (user-error "Cannot increment *"))
- (t
- (number-to-string (+ yy inc))))))
- ((eq what 'month)
- (setf day oday
- year oyear
- (if (memq 'month calendar-date-display-form)
- month
- monthname)
- (cond ((not current-prefix-arg)
- (todo-read-date 'month))
- ((or (string= omonth "*") (= mm 13))
- (user-error "Cannot increment *"))
- (t
- (let* ((mmo mm)
- ;; Change by 12 or more months?
- (bigincp (>= (abs inc) 12))
- ;; Month number is in range 1..12.
- (mminc (+ mm (% inc 12)))
- (mm (% (+ mminc 12) 12))
- ;; 12n mod 12 = 0, so 0 is December.
- (mm (if (= mm 0) 12 mm))
- ;; Does change in month cross year?
- (mmcmp (cond ((< inc 0) (> mm mmo))
- ((> inc 0) (< mm mmo))))
- (yyadjust (if bigincp
- (+ (abs (/ inc 12))
- (if mmcmp 1 0))
- 1)))
- ;; Adjust year if necessary.
- (setq yy (cond ((and (< inc 0)
- (or mmcmp bigincp))
- (- yy yyadjust))
- ((and (> inc 0)
- (or mmcmp bigincp))
- (+ yy yyadjust))
- (t yy)))
- (setq year (number-to-string yy))
- ;; Return the changed numerical month as
- ;; a string or the corresponding month name.
- (if omonth
- (number-to-string mm)
- (aref tma-array (1- mm)))))))
- ;; Since the number corresponding to the arbitrary
- ;; month name "*" is out of the range of
- ;; calendar-last-day-of-month, set it to 1
- ;; (corresponding to January) to allow 31 days.
- (let ((mm (if (= mm 13) 1 mm)))
- (if (> (string-to-number day)
- (calendar-last-day-of-month mm yy))
- (user-error "%s %s does not have %s days"
- (aref tmn-array (1- mm))
- (if (= mm 2) yy "") day))))
- ((eq what 'day)
- (setq year oyear
- month omonth
- monthname omonthname
- day (cond
- ((not current-prefix-arg)
- (todo-read-date 'day mm yy))
- ((string= oday "*")
- (user-error "Cannot increment *"))
- ((or (string= omonth "*") (string= omonthname "*"))
- (setq dd (+ dd inc))
- (if (> dd 31)
- (user-error
- "A month cannot have more than 31 days")
- (number-to-string dd)))
- ;; Increment or decrement day by INC,
- ;; adjusting month and year if necessary
- ;; (if year is "*" assume current year to
- ;; calculate adjustment).
- (t
- (let* ((yy (or yy (calendar-extract-year
- (calendar-current-date))))
- (date (calendar-gregorian-from-absolute
- (+ (calendar-absolute-from-gregorian
- (list mm dd yy))
- inc)))
- (adjmm (nth 0 date)))
- ;; Set year and month(name) to adjusted values.
- (unless (string= year "*")
- (setq year (number-to-string (nth 2 date))))
- (if month
- (setq month (number-to-string adjmm))
- (setq monthname (aref tma-array (1- adjmm))))
- ;; Return changed numerical day as a string.
- (number-to-string (nth 1 date)))))))))
+ (save-match-data
+ (cond
+ ((eq what 'date)
+ (setq ndate (todo-read-date)))
+ ((eq what 'calendar)
+ (setq ndate (todo-set-date-from-calendar)))
+ ((eq what 'today)
+ (setq ndate (calendar-date-string (calendar-current-date) t t)))
+ ((eq what 'dayname)
+ (setq ndate (todo-read-dayname)))
+ ((eq what 'time)
+ (setq ntime (todo-read-time))
+ (when (> (length ntime) 0)
+ (setq ntime (concat " " ntime))))
+ ;; When date string consists only of a day name,
+ ;; passing other date components is a noop.
+ ((and odayname (memq what '(year month day))))
+ ((eq what 'year)
+ (setq day oday
+ monthname omonthname
+ month omonth
+ year (cond ((not current-prefix-arg)
+ (todo-read-date 'year))
+ ((string= oyear "*")
+ (user-error "Cannot increment *"))
+ (t
+ (number-to-string (+ yy inc))))))
+ ((eq what 'month)
+ (setf day oday
+ year oyear
+ (if (memq 'month calendar-date-display-form)
+ month
+ monthname)
+ (cond ((not current-prefix-arg)
+ (todo-read-date 'month))
+ ((or (string= omonth "*") (= mm 13))
+ (user-error "Cannot increment *"))
+ (t
+ (let* ((mmo mm)
+ ;; Change by 12 or more months?
+ (bigincp (>= (abs inc) 12))
+ ;; Month number is in range 1..12.
+ (mminc (+ mm (% inc 12)))
+ (mm (% (+ mminc 12) 12))
+ ;; 12n mod 12 = 0, so 0 is December.
+ (mm (if (= mm 0) 12 mm))
+ ;; Does change in month cross year?
+ (mmcmp (cond ((< inc 0) (> mm mmo))
+ ((> inc 0) (< mm mmo))))
+ (yyadjust (if bigincp
+ (+ (abs (/ inc 12))
+ (if mmcmp 1 0))
+ 1)))
+ ;; Adjust year if necessary.
+ (setq yy (cond ((and (< inc 0)
+ (or mmcmp bigincp))
+ (- yy yyadjust))
+ ((and (> inc 0)
+ (or mmcmp bigincp))
+ (+ yy yyadjust))
+ (t yy)))
+ (setq year (number-to-string yy))
+ ;; Return the changed numerical month as
+ ;; a string or the corresponding month name.
+ (if omonth
+ (number-to-string mm)
+ (aref tma-array (1- mm)))))))
+ ;; Since the number corresponding to the arbitrary
+ ;; month name "*" is out of the range of
+ ;; calendar-last-day-of-month, set it to 1
+ ;; (corresponding to January) to allow 31 days.
+ (let ((mm (if (= mm 13) 1 mm)))
+ (if (> (string-to-number day)
+ (calendar-last-day-of-month mm yy))
+ (user-error "%s %s does not have %s days"
+ (aref tmn-array (1- mm))
+ (if (= mm 2) yy "") day))))
+ ((eq what 'day)
+ (setq year oyear
+ month omonth
+ monthname omonthname
+ day (cond
+ ((not current-prefix-arg)
+ (todo-read-date 'day mm yy))
+ ((string= oday "*")
+ (user-error "Cannot increment *"))
+ ((or (string= omonth "*") (string= omonthname "*"))
+ (setq dd (+ dd inc))
+ (if (> dd 31)
+ (user-error
+ "A month cannot have more than 31 days")
+ (number-to-string dd)))
+ ;; Increment or decrement day by INC,
+ ;; adjusting month and year if necessary
+ ;; (if year is "*" assume current year to
+ ;; calculate adjustment).
+ (t
+ (let* ((yy (or yy (calendar-extract-year
+ (calendar-current-date))))
+ (date (calendar-gregorian-from-absolute
+ (+ (calendar-absolute-from-gregorian
+ (list mm dd yy))
+ inc)))
+ (adjmm (nth 0 date)))
+ ;; Set year and month(name) to adjusted values.
+ (unless (string= year "*")
+ (setq year (number-to-string (nth 2 date))))
+ (if month
+ (setq month (number-to-string adjmm))
+ (setq monthname (aref tma-array (1- adjmm))))
+ ;; Return changed numerical day as a string.
+ (number-to-string (nth 1 date))))))))))
(unless odayname
;; If year, month or day date string components were
;; changed, rebuild the date string.
^ permalink raw reply related [flat|nested] 3+ messages in thread
* bug#63811: 29.0.91; todo-mode item editing bugs
2023-05-31 9:12 bug#63811: 29.0.91; todo-mode item editing bugs Stephen Berman
@ 2023-05-31 12:31 ` Eli Zaretskii
2023-05-31 14:25 ` Stephen Berman
0 siblings, 1 reply; 3+ messages in thread
From: Eli Zaretskii @ 2023-05-31 12:31 UTC (permalink / raw)
To: Stephen Berman; +Cc: 63811
> From: Stephen Berman <stephen.berman@gmx.net>
> Date: Wed, 31 May 2023 11:12:32 +0200
>
> I have encountered several bugs in todo-mode when creating and editing
> todo items. I have fixes for them and since I'm the todo-mode
> maintainer I intend to push them to the Savannah repo when I get a bug
> number and clearance for which branch to push to. The bugs are
> long-standing, at least since Emacs 28 and probably earlier. The fixes
> touch no code outside of todo-mode.el and seem low-risk; in my testing
> they work as expected. I've attached a diff against emacs-29 below (the
> todo-mode.el files in current emacs-29 and master are identical).
Assuming all the tests still pass after these changes, please go ahead
and install on emacs-29, and thanks.
^ permalink raw reply [flat|nested] 3+ messages in thread
* bug#63811: 29.0.91; todo-mode item editing bugs
2023-05-31 12:31 ` Eli Zaretskii
@ 2023-05-31 14:25 ` Stephen Berman
0 siblings, 0 replies; 3+ messages in thread
From: Stephen Berman @ 2023-05-31 14:25 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 63811-done
On Wed, 31 May 2023 15:31:07 +0300 Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Stephen Berman <stephen.berman@gmx.net>
>> Date: Wed, 31 May 2023 11:12:32 +0200
>>
>> I have encountered several bugs in todo-mode when creating and editing
>> todo items. I have fixes for them and since I'm the todo-mode
>> maintainer I intend to push them to the Savannah repo when I get a bug
>> number and clearance for which branch to push to. The bugs are
>> long-standing, at least since Emacs 28 and probably earlier. The fixes
>> touch no code outside of todo-mode.el and seem low-risk; in my testing
>> they work as expected. I've attached a diff against emacs-29 below (the
>> todo-mode.el files in current emacs-29 and master are identical).
>
> Assuming all the tests still pass after these changes, please go ahead
> and install on emacs-29, and thanks.
All todo-mode tests passed `make check'. I did get some unexpected
failures in a few other tests and a hang in epg-tests, but the same
thing happened when I ran `make check' on my master branch, which does
not have the todo-mode changes, so these are not responsible for the
test problems, and I went ahead and pushed the changes to emacs-29 and
am closing the bug report. Thanks.
Steve Berman
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-05-31 14:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-31 9:12 bug#63811: 29.0.91; todo-mode item editing bugs Stephen Berman
2023-05-31 12:31 ` Eli Zaretskii
2023-05-31 14:25 ` Stephen Berman
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).