bug#12533: 24.2.50; incorrect behavior & formatting in Customize

bug#12533: 24.2.50; incorrect behavior & formatting in Customize

[Drew Adams]

emacs -Q
 
(defcustom foo ()
  "..."
  :type '(repeat (cons
                  string
                  (choice
                   (const flimpo)
                   (const nil)
                   (other t))))
  :group 'edit)
 
M-x customize-option foo
 
Click button INS.
 
(Optional: Enter something in field `String'.)
 
Choose `Other' in `Value Menu'.
 
An INS button pops up inappropriately under `Choice'.  If you click it
then it shows that it expects another `repeat' cell, at the wrong level.
 
If you click the outer (i.e., the correct) INS button, it behaves
normally.  But if you then choose `Other' in its `Value Menu' you get
the same problem again.
 
This broken behavior happens as long as you choose `Other' in `Value
Menu'.  If you choose one of the other possibilities then Customize
behaves correctly.
 
To me this is clearly a bug.  But it is so all the way back through
Emacs 20.  So I wonder - am I right?  I cannot believe that this
behavior is correct.  What am I missing?
 
In GNU Emacs 24.2.50.1 (i386-mingw-nt5.1.2600)
 of 2012-09-17 on MARVIN
Bzr revision: 110062 cyd@gnu.org-20120917054104-r93rtwkrtva73ewe
Windowing system distributor `Microsoft Corp.', version 5.1.2600
Configured using:
 `configure --with-gcc (4.7) --no-opt --enable-checking --cflags
 -ID:/devel/emacs/libs/libXpm-3.5.8/include
 -ID:/devel/emacs/libs/libXpm-3.5.8/src
 -ID:/devel/emacs/libs/libpng-dev_1.4.3-1/include
 -ID:/devel/emacs/libs/zlib-dev_1.2.5-2/include
 -ID:/devel/emacs/libs/giflib-4.1.4-1/include
 -ID:/devel/emacs/libs/jpeg-6b-4/include
 -ID:/devel/emacs/libs/tiff-3.8.2-1/include
 -ID:/devel/emacs/libs/gnutls-3.0.9/include
 -ID:/devel/emacs/libs/libiconv-1.13.1-1-dev/include
 -ID:/devel/emacs/libs/libxml2-2.7.8/include/libxml2'
 

bug#12533: 24.2.50; incorrect behavior & formatting in Customize

[Mauro Aranda]

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

tags 12533 patch
quit


"Drew Adams" <drew.adams@oracle.com> writes:

> emacs -Q
>
> (defcustom foo ()
>   "..."
>   :type '(repeat (cons
>                   string
>                   (choice
>                    (const flimpo)
>                    (const nil)
>                    (other t))))
>   :group 'edit)
>
> M-x customize-option foo
>
> Click button INS.
>
> (Optional: Enter something in field `String'.)
>
> Choose `Other' in `Value Menu'.
>
> An INS button pops up inappropriately under `Choice'.  If you click it
> then it shows that it expects another `repeat' cell, at the wrong level.

I can reproduce this.

> This broken behavior happens as long as you choose `Other' in `Value
> Menu'.  If you choose one of the other possibilities then Customize
> behaves correctly.
>
> To me this is clearly a bug.  But it is so all the way back through
> Emacs 20.  So I wonder - am I right?  I cannot believe that this
> behavior is correct.  What am I missing?
>

I think you are right.  And it happens with the `Other' option, which
creates a widget of type `other', because that widget was defined with a
format string that contains the %n escape at the end of the string.

That escape add spaces (for indentation), after inserting a newline.
But it's not useful when at the end of the format string, because that
indents unconditionally the contents of the buffer that follow the
widget.  The %n escape is better used in the middle of a format string,
to cause indentation of the contents that belong to the same widget.

Therefore, I think it is a mistake to have that %n in the format string
of the `other' widget, and I propose the attached patch.

Best regards,
Mauro.

[-- Attachment #2: Type: text/html, Size: 2163 bytes --]

bug#12533: 24.2.50; incorrect behavior & formatting in Customize

[Mauro Aranda]

[-- Attachment #1.1: Type: text/plain, Size: 24 bytes --]

Oops, here's the patch.

[-- Attachment #1.2: Type: text/html, Size: 53 bytes --]

[-- Attachment #2: 0001-Don-t-indent-unrelated-widgets-following-widget-of-t.patch --]
[-- Type: text/x-patch, Size: 876 bytes --]

From 789110c496067c75451ab3e5ea4d24de0c3b8a08 Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Wed, 25 Sep 2019 18:12:55 -0300
Subject: [PATCH] Don't indent unrelated widgets following widget of type
 'other

* lisp/wid-edit.el (widget 'other): Use \n instead of the %n escape in the
:format property of this widget.  If %n is used at the end of the
format string, unrelated widgets get indented.  (Bug#12533)
---
 lisp/wid-edit.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
index 52b7532..916d41a 100644
--- a/lisp/wid-edit.el
+++ b/lisp/wid-edit.el
@@ -3063,7 +3063,7 @@ 'other
 If the user selects this alternative, that specifies DEFAULT
 as the value."
   :tag "Other"
-  :format "%t%n"
+  :format "%t\n"
   :value 'other)
 
 (defvar widget-string-prompt-value-history nil
-- 
2.7.4

bug#12533: 24.2.50; incorrect behavior & formatting in Customize

[Eli Zaretskii]

> From: Mauro Aranda <maurooaranda@gmail.com>
> Date: Wed, 25 Sep 2019 18:27:16 -0300
> 
> Oops, here's the patch.

Thanks, but can you also add some tests for this?

bug#12533: 24.2.50; incorrect behavior & formatting in Customize

[Mauro Aranda]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Mauro Aranda <maurooaranda@gmail.com>
>> Date: Wed, 25 Sep 2019 18:27:16 -0300
>>
>> Oops, here's the patch.
>
> Thanks, but can you also add some tests for this?

Sure.  Will do.

[-- Attachment #2: Type: text/html, Size: 402 bytes --]

bug#12533: 24.2.50; incorrect behavior & formatting in Customize

[Mauro Aranda]

[-- Attachment #1.1: Type: text/plain, Size: 308 bytes --]

I've added three tests to the patch:
1) One that should fail, because of the use of %n at the end.
2) One that should pass, because \n is used instead of %n.
3) Another one that should pass, because %n is used correctly,
at the middle of the format string.

Let me know what you think.

Best regards,
Mauro.

[-- Attachment #1.2: Type: text/html, Size: 447 bytes --]

[-- Attachment #2: 0001-Don-t-indent-unrelated-widgets-following-widget-of-t.patch --]
[-- Type: text/x-patch, Size: 4852 bytes --]

From 7033339fe25f490d451d8e599677be48622b9fd3 Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Wed, 25 Sep 2019 18:12:55 -0300
Subject: [PATCH] Don't indent unrelated widgets following widget of type
 'other

* lisp/wid-edit.el (widget 'other): Use \n instead of the %n escape in the
:format property of this widget.  If %n is used at the end of the
format string, unrelated widgets get indented.  (Bug#12533)

* test/wid-edit-tests.el (widget-test-indentation-after-%n)
(widget-test-indentation-after-newline)
(widget-test-newline-and-indent-same-widget): New tests.
---
 lisp/wid-edit.el            |  2 +-
 test/lisp/wid-edit-tests.el | 77 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
index 52b7532..916d41a 100644
--- a/lisp/wid-edit.el
+++ b/lisp/wid-edit.el
@@ -3063,7 +3063,7 @@ 'other
 If the user selects this alternative, that specifies DEFAULT
 as the value."
   :tag "Other"
-  :format "%t%n"
+  :format "%t\n"
   :value 'other)
 
 (defvar widget-string-prompt-value-history nil
diff --git a/test/lisp/wid-edit-tests.el b/test/lisp/wid-edit-tests.el
index a4350e71..679a29a 100644
--- a/test/lisp/wid-edit-tests.el
+++ b/test/lisp/wid-edit-tests.el
@@ -36,4 +36,81 @@
     (insert-button "overlay button")
     (should-not (widget-at (1- (point))))))
 
+;; The following three tests compare the effect of using either %n or \n at the
+;; end of a format string, as well as using %n at the end or in the middle of
+;; the format string.  (Bug#12533)
+
+(ert-deftest widget-test-indentation-after-%n ()
+  "Fail when %n is used at the end of a format string."
+  :expected-result :failed
+  (with-temp-buffer
+    (let (wid indented)
+      (widget-insert "Testing indentation.\n")
+      ;; If we use %n at the end of the format string of the widget `other', we
+      ;; screw up indentation of the following widgets.
+      (setq wid (widget-create
+                 '(repeat :indent 4
+                   (cons
+                    string (choice (other :tag "Other" :format "%t%n" c))))))
+      (goto-char (widget-get wid :value-pos))
+      ;; Since we indent the `repeat' widget, we skip the space characters
+      ;; inserted.
+      (skip-chars-forward " ")
+      (setq indented (current-column)) ; Save the column to which we indented.
+      (should (eq indented (or (widget-get wid :indent) 0)))
+      ;; Insert an entry.  This simulates a click or RET at the INS button.
+      (widget-apply (widget-at) :action)
+      (goto-char (widget-get wid :value-pos))
+      (skip-chars-forward " ")
+      ;; This fails, because the button is not at the right column.
+      (should (eq (current-column) indented)))))
+
+(ert-deftest widget-test-indentation-after-newline ()
+  "Pass when the newline is used at the end of a format string."
+  (with-temp-buffer
+    (let (wid indented)
+      (widget-insert "Testing indentation.\n")
+      (setq wid (widget-create
+                 '(repeat :indent 4
+                   (cons
+                    string
+                    (choice (other :tag "Other" :format "%t\n" c))))))
+      (goto-char (widget-get wid :value-pos))
+      (skip-chars-forward " ")
+      (setq indented (current-column))
+      (should (eq (current-column) (or (widget-get wid :indent) 0)))
+      (widget-apply (widget-at) :action)
+      (goto-char (widget-get wid :value-pos))
+      (skip-chars-forward " ")
+      ;; Because we used \n in the format string, this pass.
+      (should (eq (current-column) indented)))))
+
+(ert-deftest widget-test-newline-and-indent-same-widget ()
+  "It's OK to use the %n escape sequence in the middle of the format string."
+  (with-temp-buffer
+    (let (wid indented)
+      (widget-insert "Testing indentation.\n")
+      (setq wid (widget-create
+                 '(repeat :indent 4
+                          :format "%{%t%}:%n%v%i\n"
+                          (cons
+                           string
+                           (choice (other :tag "Other" :format "%t\n" c))))))
+      (goto-char (widget-get wid :value-pos))
+      (skip-chars-forward " ")
+      (setq indented (current-column))
+      (should (eq indented (or (widget-get wid :indent) 0)))
+      (widget-apply (widget-at) :action)
+      (goto-char (widget-get wid :value-pos))
+      (skip-chars-forward " ")
+      (should (eq (current-column) indented))
+
+      ;; Also, the children are indented correctly.
+      (let ((grandchild
+             ;; This gets the `string' widget.
+             (car (widget-get (car (widget-get wid :children)) :children))))
+        (goto-char (widget-get grandchild :from))
+        (should (eq (current-column)
+                    (widget-get grandchild :indent)))))))
+
 ;;; wid-edit-tests.el ends here
-- 
2.7.4

bug#12533: 24.2.50; incorrect behavior & formatting in Customize

[Lars Ingebrigtsen]

Mauro Aranda <maurooaranda@gmail.com> writes:

> I've added three tests to the patch:
> 1) One that should fail, because of the use of %n at the end.
> 2) One that should pass, because \n is used instead of %n.
> 3) Another one that should pass, because %n is used correctly,
> at the middle of the format string.
>
> Let me know what you think.

Thanks, looks great; applied to the trunk.

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

bug#12533: 24.2.50; incorrect behavior & formatting in Customize

[Mauro Aranda]

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

Thanks!

[-- Attachment #2: Type: text/html, Size: 33 bytes --]