bug#45623: 28.0.50; widget-backward sometimes skips one widget

bug#45623: 28.0.50; widget-backward sometimes skips one widget

[Mauro Aranda]

Starting from emacs -Q:

1. Eval the following code:
(require 'wid-edit)

(defun test-widget-backward ()
  (interactive)
  (switch-to-buffer "*Widget Test*")
  (kill-all-local-variables)
  (let ((inhibit-read-only t))
    (erase-buffer))
  (remove-overlays)
  (widget-insert "Testing widget movement commands.\n\n")
  (dolist (el '("First" "Second" "Third"))
    (widget-create 'push-button el))
  (widget-insert "\n")
  (goto-char (point-min))
  (use-local-map widget-keymap)
  (widget-setup))

2. C-u 3 C-i to move to the start of the button labeled Third:

3. C-M-i to run the widget-backward command.

Point moves to the start of the button labeled First, not to the
start of the button labeled Second.  I expected point to move to the
start of the button labeled Second.


If instead I use buttons from button.el, moving point with
backward-button (the analogous to widget-backward) behaves as I
expected.


The very first line of widget-move is:
(or (bobp) (> arg 0) (backward-char))

That backward-char moves point from the third button to the second
button before starting to look for the previous widget, so that's the
reason point ends up at the start of the first button.

I don't understand why that call to backward-char is there.  Does anyone
know of a situation where it is useful?


In GNU Emacs 28.0.50 (build 8, x86_64-pc-linux-gnu, GTK+ Version 3.22.30, cairo version 1.15.10)
 of 2021-01-03 built on tbb-desktop
Repository revision: 825b4ec338e82869dc656c7041ab2483b6c22479
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12008000
System Description: Ubuntu 18.04.5 LTS

Configured features:
XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND DBUS GSETTINGS GLIB NOTIFY
INOTIFY LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM MODULES THREADS PDUMPER

Important settings:
  value of $LC_MONETARY: es_AR.UTF-8
  value of $LC_NUMERIC: es_AR.UTF-8
  value of $LC_TIME: es_AR.UTF-8
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Fundamental

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
rfc822 mml easymenu mml-sec epa derived epg epg-config gnus-util rmail
rmail-loaddefs auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json map text-property-search time-date
subr-x seq byte-opt gv bytecomp byte-compile cconv mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils wid-edit
cl-loaddefs cl-lib iso-transl tooltip eldoc electric uniquify ediff-hook
vc-hooks lisp-float-type mwheel term/x-win x-win term/common-win x-dnd
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode elisp-mode lisp-mode prog-mode register page
tab-bar menu-bar rfn-eshadow isearch timer select scroll-bar mouse
jit-lock font-lock syntax facemenu font-core term/tty-colors frame
minibuffer cl-generic cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite charscript charprop case-table epa-hook jka-cmpr-hook help
simple abbrev obarray cl-preloaded nadvice button loaddefs faces
cus-face macroexp files window text-properties overlay sha1 md5 base64
format env code-pages mule custom widget hashtable-print-readable
backquote threads dbusbind inotify dynamic-setting system-font-setting
font-render-setting cairo move-toolbar gtk x-toolkit x multi-tty
make-network-process emacs)

Memory information:
((conses 16 54440 9138)
 (symbols 48 7084 1)
 (strings 32 19227 1450)
 (string-bytes 1 632153)
 (vectors 16 12046)
 (vector-slots 8 168478 8434)
 (floats 8 23 47)
 (intervals 56 240 0)
 (buffers 984 12))

bug#45623: 28.0.50; widget-backward sometimes skips one widget

[Lars Ingebrigtsen]

Mauro Aranda <maurooaranda@gmail.com> writes:

> I don't understand why that call to backward-char is there.  Does anyone
> know of a situation where it is useful?

If point is just after a button, it makes C-M-i not going to that
button, but the previous one?  But that doesn't really sound like
something you'd want, either?

So, no, I don't see any situation where it would useful -- looks like a
misfeature to me.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#45623: 28.0.50; widget-backward sometimes skips one widget

[Mauro Aranda]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Mauro Aranda <maurooaranda@gmail.com> writes:
>
>> I don't understand why that call to backward-char is there.  Does anyone
>> know of a situation where it is useful?
>
> If point is just after a button, it makes C-M-i not going to that
> button, but the previous one?  But that doesn't really sound like
> something you'd want, either?
>
> So, no, I don't see any situation where it would useful -- looks like a
> misfeature to me.

Agreed.  So, any objections to installing this patch and see if
something breaks?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Patch --]
[-- Type: text/x-patch, Size: 2153 bytes --]

From 86c211725ab39ddfc569a16fefd0db6724651b43 Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Sun, 3 Jan 2021 11:27:02 -0300
Subject: [PATCH] Don't skip widgets when moving backward

* lisp/wid-edit.el (widget-move): Remove code that caused
widget-backward to skip a previous adjacent widget when moving
backward from the start of a widget.  (Bug#45623)

* test/lisp/wid-edit-tests.el (widget-test-widget-backward): New test.
---
 lisp/wid-edit.el            |  1 -
 test/lisp/wid-edit-tests.el | 22 ++++++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
index f920130226..8b10d71dcb 100644
--- a/lisp/wid-edit.el
+++ b/lisp/wid-edit.el
@@ -1204,7 +1204,6 @@ widget-move
 ARG may be negative to move backward.
 When the second optional argument is non-nil,
 nothing is shown in the echo area."
-  (or (bobp) (> arg 0) (backward-char))
   (let ((wrapped 0)
 	(number arg)
 	(old (widget-tabable-at)))
diff --git a/test/lisp/wid-edit-tests.el b/test/lisp/wid-edit-tests.el
index 35235c6566..f2e8c321f5 100644
--- a/test/lisp/wid-edit-tests.el
+++ b/test/lisp/wid-edit-tests.el
@@ -301,4 +301,26 @@ widget-test-option-can-handle-inlinable-choice
       (should child)
       (should (equal (widget-value widget) '((1 "One")))))))
 
+(ert-deftest widget-test-widget-backward ()
+  "Test that `widget-backward' works OK."
+  (with-temp-buffer
+    (widget-insert "Testing.\n\n")
+    (dolist (el '("First" "Second" "Third"))
+      (widget-create 'push-button el))
+    (widget-insert "\n")
+    (goto-char (point-min))
+    (use-local-map widget-keymap)
+    (widget-setup)
+    ;; Check that moving from the widget's start works.
+    (widget-forward 3)
+    (should (string= "Third" (widget-value (widget-at))))
+    (widget-backward 1)
+    (should (string= "Second" (widget-value (widget-at))))
+    ;; Check that moving from inside the widget works.
+    (goto-char (point-min))
+    (widget-forward 3)
+    (forward-char)
+    (widget-backward 1)
+    (should (string= "Second" (widget-value (widget-at))))))
+
 ;;; wid-edit-tests.el ends here
-- 
2.29.2

bug#45623: 28.0.50; widget-backward sometimes skips one widget

[Lars Ingebrigtsen]

Mauro Aranda <maurooaranda@gmail.com> writes:

> Agreed.  So, any objections to installing this patch and see if
> something breaks?

Go ahead.

We've had some of the same issues with TAB/BACKTAB with button.el
buttons (and eww links) -- the edge cases where two buttons touch each
other have been under-scrutinised, although I hope I got all of them the
last time.  *crosses fingers*

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#45623: 28.0.50; widget-backward sometimes skips one widget

[Mauro Aranda]

tags 45623 fixed
close 45623
quit


Lars Ingebrigtsen <larsi@gnus.org> writes:

> Mauro Aranda <maurooaranda@gmail.com> writes:
>
>> Agreed.  So, any objections to installing this patch and see if
>> something breaks?
>
> Go ahead.

Thanks; done.