:format strings in Custom ending in %h

:format strings in Custom ending in %h

[Luc Teirlinck]

Sorry, my original file had an inconsistency with the defgroup and the
:group.  The bug is real however and stays with the following
corrected file.  Only difference:

:group 'nifty-group gets replaced with:

:group 'nifty

===File ~/customdoc.el======================================
(defgroup nifty nil
  "Single item group"
  :group 'convenience)

(defcustom nifty-var nil
  "*Nifty doc string."
  :group 'nifty
  :type '(choice (const :tag "True"
			:format "%t\n%h"
			:doc
			"True stuff.
Second line of true stuff."
			t)
		 (other :tag "False"
			:format "%t\n%h"
			:doc
			"Falsehoods.
Second line of falsehoods"
			nil)))

============================================================

Re: :format strings in Custom ending in %h

[Per Abrahamsen]

Does this fix the problem?

We should probably always call widget-children-value-delete, as any
widget may have children if they have a long doc string.

-- Per

2003-08-21  Per Abrahamsen  <abraham@dina.kvl.dk>

	* wid-edit.el (item): Delete children (due to doc-string).


*** wid-edit.el.~1.120.~	Thu Aug 21 14:17:37 2003
--- wid-edit.el	Thu Aug 21 14:19:14 2003
***************
*** 1556,1562 ****
    "Constant items for inclusion in other widgets."
    :convert-widget 'widget-value-convert-widget
    :value-create 'widget-item-value-create
!   :value-delete 'ignore
    :value-get 'widget-value-value-get
    :match 'widget-item-match
    :match-inline 'widget-item-match-inline
--- 1556,1562 ----
    "Constant items for inclusion in other widgets."
    :convert-widget 'widget-value-convert-widget
    :value-create 'widget-item-value-create
!   :value-delete 'widget-children-value-delete
    :value-get 'widget-value-value-get
    :match 'widget-item-match
    :match-inline 'widget-item-match-inline

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> Sorry, my original file had an inconsistency with the defgroup and the
> :group.  The bug is real however and stays with the following
> corrected file.  Only difference:
>
> :group 'nifty-group gets replaced with:
>
> :group 'nifty
>
> ===File ~/customdoc.el======================================
> (defgroup nifty nil
>   "Single item group"
>   :group 'convenience)
>
> (defcustom nifty-var nil
>   "*Nifty doc string."
>   :group 'nifty
>   :type '(choice (const :tag "True"
> 			:format "%t\n%h"
> 			:doc
> 			"True stuff.
> Second line of true stuff."
> 			t)
> 		 (other :tag "False"
> 			:format "%t\n%h"
> 			:doc
> 			"Falsehoods.
> Second line of falsehoods"
> 			nil)))
>
> ============================================================

Re: :format strings in Custom ending in %h

[Luc Teirlinck]

Per Abrahamsen wrote:

   Does this fix the problem?

   We should probably always call widget-children-value-delete, as any
   widget may have children if they have a long doc string.

No, the bug persists after that change.  Did you receive the original
message which the message you quoted was a correction to?  It somehow
never arrived at emacs-devel.  I will repost with the correction.

Sincerely,

Luc.

Re: :format strings in Custom ending in %h

[Luc Teirlinck]

In my previous message, I of course meant that I would repost the
corrected version, not the "correction".  The description of the bug
applies with or without your proposed change:

Original message:

Is it a rule that :format strings are not allowed to end in %h?
If so where is this mentioned in either the Elisp manual or the
(widget) documentation?

Such format strings produce trouble as described below.

Do:

emacs-21.3.50 -q --eval "(blink-cursor-mode 0)" &

and load the following file:

===File ~/customdoc.el======================================
(defgroup nifty nil
  "Single item group"
  :group 'convenience)

(defcustom nifty-var nil
  "*Nifty doc string."
  :group 'nifty
  :type '(choice (const :tag "True"
			:format "%t\n%h"
			:doc
			"True stuff.
Second line of true stuff."
			t)
		 (other :tag "False"
			:format "%t\n%h"
			:doc
			"Falsehoods.
Second line of falsehoods"
			nil)))

============================================================

Then do:

M-x customize-group RET nifty

Click on "More" next to "Falsehoods."

Then click on "Value Menu" and choose "True".

Now we have:

True stuff. More
Falsehoods. Hide Rest
Second line of falsehoods
 
We should not have those last two lines any more and if we add a space
to both :format strings, i.e :format "%t\n%h ", and repeat the
exercise, they indeed disappear and everything works wonderfully.

Sincerely,

Luc.

Re: :format strings in Custom ending in %h

[Per Abrahamsen]

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> Is it a rule that :format strings are not allowed to end in %h?

All occurences of %h in :format string in the Emacs source is at the
end, so that is definetely not a rule.

The patch I posted solved the problem for me.

Re: :format strings in Custom ending in %h

[Luc Teirlinck]

Per Abrahamsen wrote:

   The patch I posted solved the problem for me.

This is strange.  This does not seem to be the kind of thing that
could depend on operating system, fonts or whatever.  I used emacs -q
and latest CVS.

Sincerely,

Luc.

Re: :format strings in Custom ending in %h

[Per Abrahamsen]

Does this patch help?

2003-08-24  Per Abrahamsen  <abraham@dina.kvl.dk>

	* wid-edit.el (widget-default-delete): Always delete child widgets.

*** wid-edit.el.~1.120.~	Thu Aug 21 14:17:37 2003
--- wid-edit.el	Sun Aug 24 17:37:04 2003
***************
*** 1465,1470 ****
--- 1465,1471 ----
  	(inhibit-modification-hooks t)
  	(inhibit-read-only t))
      (widget-apply widget :value-delete)
+     (widget-children-value-delete widget)
      (when inactive-overlay
        (delete-overlay inactive-overlay))
      (when button-overlay

Re: :format strings in Custom ending in %h

[Luc Teirlinck]

Per Abrahamsen wrote:

   Does this patch help?

   2003-08-24  Per Abrahamsen  <abraham@dina.kvl.dk>

	   * wid-edit.el (widget-default-delete): Always delete child widgets.

   *** wid-edit.el.~1.120.~	Thu Aug 21 14:17:37 2003
   --- wid-edit.el	Sun Aug 24 17:37:04 2003
   ***************
   *** 1465,1470 ****
   --- 1465,1471 ----
	   (inhibit-modification-hooks t)
	   (inhibit-read-only t))
	 (widget-apply widget :value-delete)
   +     (widget-children-value-delete widget)
	 (when inactive-overlay
	   (delete-overlay inactive-overlay))
	 (when button-overlay

Yes.

I guess that after applying this patch `widget-children-value-delete'
will become a pretty redundant value for :value-delete, essentially
equivalent with `ignore'.

Sincerely,

Luc.

Re: :format strings in Custom ending in %h

[Per Abrahamsen]

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> I guess that after applying this patch `widget-children-value-delete'
> will become a pretty redundant value for :value-delete, essentially
> equivalent with `ignore'.

Yes, if we apply this patch we probably also should change all these
values, mark widget-children-value-delete as obsolete (and inline the
code in widget-default-delete), and document this property of :buttons
and :children somewhere.

But I don't know if there is some code that depend on :buttons and
:children *not* being deleted recursively.  Maybe applying the patch
will be the easiest way to find out.

Oh, and we probably should find other places where :buttons and
:children are deleted (now) reduntantly, and adjust those.

Re: :format strings in Custom ending in %h

[Luc Teirlinck]

I am about ready to submit a file for inclusion in Emacs.  It contains
a complex defcustom which does not function properly without the
following patch.  I have applied this patch to my own Emacs CVS and
have used it ever since the end of August without problems.  The patch
is needed to fix an annoying problem in Custom which we discussed at
the end of August.  I had the impression that you were going to commit
something along these lines:

2003-08-24  Per Abrahamsen  <abraham@dina.kvl.dk>

        * wid-edit.el (widget-default-delete): Always delete child
          widgets.

*** wid-edit.el.~1.120.~        Thu Aug 21 14:17:37 2003
--- wid-edit.el Sun Aug 24 17:37:04 2003
***************
*** 1465,1470 ****
--- 1465,1471 ----
        (inhibit-modification-hooks t)
        (inhibit-read-only t))
      (widget-apply widget :value-delete)
+     (widget-children-value-delete widget)
      (when inactive-overlay
        (delete-overlay inactive-overlay))
      (when button-overlay

Old follow-up message (from Per Abrahamsen):

    Luc Teirlinck <teirllm@dms.auburn.edu> writes:

    > I guess that after applying this patch
    > `widget-children-value-delete'
    > will become a pretty redundant value for :value-delete, essentially
    > equivalent with `ignore'.

    Yes, if we apply this patch we probably also should change all these
    values, mark widget-children-value-delete as obsolete (and inline the
    code in widget-default-delete), and document this property of :buttons
    and :children somewhere.

    But I don't know if there is some code that depend on :buttons and
    :children *not* being deleted recursively.  Maybe applying the patch
    will be the easiest way to find out.

    Oh, and we probably should find other places where :buttons and
    :children are deleted (now) reduntantly, and adjust those.

Did you change your mind about the next to last paragraph?

>From diffing with the current CVS, I noticed that the actual complete
patch I have been using since August (and hence has been tested for a
while) is the diff below.  I believe your patch above (included in the
one below) makes the second (earlier) change redundant however.  I
just forgot to remove it after applying your patch above.

d ~/
diff -c /home/teirllm/emacscvsdir/emacs/lisp/wid-edit.el  ;; (current CVS)
/home/teirllm/wid-edit.el ;; personal, used since end August.
*** /home/teirllm/emacscvsdir/emacs/lisp/wid-edit.el	Mon Oct 20
11:00:20 2003
--- /home/teirllm/wid-edit.el	Tue Sep  2 07:38:18 2003
***************
*** 1465,1470 ****
--- 1465,1471 ----
    (inhibit-modification-hooks t)
    (inhibit-read-only t))
      (widget-apply widget :value-delete)
+     (widget-children-value-delete widget)
      (when inactive-overlay
        (delete-overlay inactive-overlay))
      (when button-overlay
***************
*** 1556,1562 ****
    "Constant items for inclusion in other widgets."
    :convert-widget 'widget-value-convert-widget
    :value-create 'widget-item-value-create
!   :value-delete 'ignore
    :value-get 'widget-value-value-get
    :match 'widget-item-match
    :match-inline 'widget-item-match-inline
--- 1557,1563 ----
    "Constant items for inclusion in other widgets."
    :convert-widget 'widget-value-convert-widget
    :value-create 'widget-item-value-create
!   :value-delete 'widget-children-value-delete
    :value-get 'widget-value-value-get
    :match 'widget-item-match
    :match-inline 'widget-item-match-inline

Diff finished at Mon Oct 20 11:05:58

Re: :format strings in Custom ending in %h

[Luc Teirlinck]

>From my previous message:

   >From diffing with the current CVS, I noticed that the actual complete
   patch I have been using since August (and hence has been tested for a
   while) is the diff below.  I believe your patch above (included in the
   one below) makes the second (earlier) change redundant however.

I checked that your 2003-08-24 one-line patch is indeed sufficient and
that the extra change I accidentally left in is indeed unnecessary (as
was completely obvious from the code anyway).

Sincerely,

Luc.

Re: :format strings in Custom ending in %h

[Richard Stallman]

    I checked that your 2003-08-24 one-line patch is indeed sufficient and
    that the extra change I accidentally left in is indeed unnecessary (as
    was completely obvious from the code anyway).

That clears up one uncertainty, which is good.  We still have the
other issues that Per raised back in August:

    Yes, if we apply this patch we probably also should change all these
    values, mark widget-children-value-delete as obsolete (and inline the
    code in widget-default-delete), and document this property of :buttons
    and :children somewhere.

    But I don't know if there is some code that depend on :buttons and
    :children *not* being deleted recursively.  Maybe applying the patch
    will be the easiest way to find out.

    Oh, and we probably should find other places where :buttons and
    :children are deleted (now) reduntantly, and adjust those.

(I don't entirely understand the first paragraph.)  Can someone else
figure out exactly all of what we should do here?

Re: :format strings in Custom ending in %h

[Per Abrahamsen]

Richard Stallman <rms@gnu.org> writes:

> (I don't entirely understand the first paragraph.)  Can someone else
> figure out exactly all of what we should do here?

Only the one line patch is necessary.  The rest is about removing
redundant but harmless code, and documenting the new behavior.

Re: :format strings in Custom ending in %h

[Richard Stallman]

    Only the one line patch is necessary.  The rest is about removing
    redundant but harmless code, and documenting the new behavior.

we can live without removing the redundant code, but documenting the
change is essential.  can someone write the doc?

Re: :format strings in Custom ending in %h

[Per Abrahamsen]

Richard Stallman <rms@gnu.org> writes:

>     Only the one line patch is necessary.  The rest is about removing
>     redundant but harmless code, and documenting the new behavior.
>
> we can live without removing the redundant code, but documenting the
> change is essential.  can someone write the doc?

Here is a doc change:

*** widget.texi.~1.22.~	2003-10-24 15:35:50.000000000 +0200
--- widget.texi	2003-10-24 17:59:49.000000000 +0200
***************
*** 1586,1609 ****
  Function to delete a widget.  The function takes one argument, a widget,
  and should remove all traces of the widget from the buffer.
  
  @vindex value-create@r{ keyword}
  @item :value-create
  Function to expand the @samp{%v} escape in the format string.  It will
  be called with the widget as its argument and should insert a
  representation of the widget's value in the buffer.
  
  @vindex value-delete@r{ keyword}
  @item :value-delete
  Should remove the representation of the widget's value from the buffer.
  It will be called with the widget as its argument.  It doesn't have to
  remove the text, but it should release markers and delete nested widgets
! if such have been used.
! 
! The following predefined function can be used here:
! 
! @defun widget-children-value-delete widget
! Delete all @code{:children} and @code{:buttons} in @var{widget}.
! @end defun
  
  @vindex value-get@r{ keyword}
  @item :value-get
--- 1586,1616 ----
  Function to delete a widget.  The function takes one argument, a widget,
  and should remove all traces of the widget from the buffer.
  
+ The default value is:
+ 
+ @defun widget-default-delete widget
+ Remove @var{widget} from the buffer.
+ Delete all @code{:children} and @code{:buttons} in @var{widget}.
+ @end defun
+ 
+ In most cases you should not change this value, but instead use
+ @code{:value-delete} to make any additional cleanup.
+ 
  @vindex value-create@r{ keyword}
  @item :value-create
  Function to expand the @samp{%v} escape in the format string.  It will
  be called with the widget as its argument and should insert a
  representation of the widget's value in the buffer.
  
+ Nested widgets should be listed in @code{:children} or @code{:buttons}
+ to make sure they are automatically deleted.
+ 
  @vindex value-delete@r{ keyword}
  @item :value-delete
  Should remove the representation of the widget's value from the buffer.
  It will be called with the widget as its argument.  It doesn't have to
  remove the text, but it should release markers and delete nested widgets
! if these are not listed in @code{:children} or @code{:buttons}.
  
  @vindex value-get@r{ keyword}
  @item :value-get

Re: :format strings in Custom ending in %h

[Richard Stallman]

thanks.  luc, can you install all of this now?

Re: :format strings in Custom ending in %h

[Per Abrahamsen]

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> Did you change your mind about the next to last paragraph?

No, I'm just waiting for a go-ahead.

RMS: Should I apply the patch?

And should I try to make the follow-up changes to obsolete
widget-children-value-delete and document the new behavior?

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> I am about ready to submit a file for inclusion in Emacs.  It contains
> a complex defcustom which does not function properly without the
> following patch.  I have applied this patch to my own Emacs CVS and
> have used it ever since the end of August without problems.  The patch
> is needed to fix an annoying problem in Custom which we discussed at
> the end of August.  I had the impression that you were going to commit
> something along these lines:
>
> 2003-08-24  Per Abrahamsen  <abraham@dina.kvl.dk>
>
>         * wid-edit.el (widget-default-delete): Always delete child
>           widgets.
>
> *** wid-edit.el.~1.120.~        Thu Aug 21 14:17:37 2003
> --- wid-edit.el Sun Aug 24 17:37:04 2003
> ***************
> *** 1465,1470 ****
> --- 1465,1471 ----
>         (inhibit-modification-hooks t)
>         (inhibit-read-only t))
>       (widget-apply widget :value-delete)
> +     (widget-children-value-delete widget)
>       (when inactive-overlay
>         (delete-overlay inactive-overlay))
>       (when button-overlay
>
> Old follow-up message (from Per Abrahamsen):
>
>     Luc Teirlinck <teirllm@dms.auburn.edu> writes:
>
>     > I guess that after applying this patch
>     > `widget-children-value-delete'
>     > will become a pretty redundant value for :value-delete, essentially
>     > equivalent with `ignore'.
>
>     Yes, if we apply this patch we probably also should change all these
>     values, mark widget-children-value-delete as obsolete (and inline the
>     code in widget-default-delete), and document this property of :buttons
>     and :children somewhere.
>
>     But I don't know if there is some code that depend on :buttons and
>     :children *not* being deleted recursively.  Maybe applying the patch
>     will be the easiest way to find out.
>
>     Oh, and we probably should find other places where :buttons and
>     :children are deleted (now) reduntantly, and adjust those.
>
> Did you change your mind about the next to last paragraph?
>
>>From diffing with the current CVS, I noticed that the actual complete
> patch I have been using since August (and hence has been tested for a
> while) is the diff below.  I believe your patch above (included in the
> one below) makes the second (earlier) change redundant however.  I
> just forgot to remove it after applying your patch above.
>
> d ~/
> diff -c /home/teirllm/emacscvsdir/emacs/lisp/wid-edit.el  ;; (current CVS)
> /home/teirllm/wid-edit.el ;; personal, used since end August.
> *** /home/teirllm/emacscvsdir/emacs/lisp/wid-edit.el	Mon Oct 20
> 11:00:20 2003
> --- /home/teirllm/wid-edit.el	Tue Sep  2 07:38:18 2003
> ***************
> *** 1465,1470 ****
> --- 1465,1471 ----
>     (inhibit-modification-hooks t)
>     (inhibit-read-only t))
>       (widget-apply widget :value-delete)
> +     (widget-children-value-delete widget)
>       (when inactive-overlay
>         (delete-overlay inactive-overlay))
>       (when button-overlay
> ***************
> *** 1556,1562 ****
>     "Constant items for inclusion in other widgets."
>     :convert-widget 'widget-value-convert-widget
>     :value-create 'widget-item-value-create
> !   :value-delete 'ignore
>     :value-get 'widget-value-value-get
>     :match 'widget-item-match
>     :match-inline 'widget-item-match-inline
> --- 1557,1563 ----
>     "Constant items for inclusion in other widgets."
>     :convert-widget 'widget-value-convert-widget
>     :value-create 'widget-item-value-create
> !   :value-delete 'widget-children-value-delete
>     :value-get 'widget-value-value-get
>     :match 'widget-item-match
>     :match-inline 'widget-item-match-inline
>
> Diff finished at Mon Oct 20 11:05:58

Re: :format strings in Custom ending in %h

[Luc Teirlinck]

Per Abrahamsen wrote:

   The patch I posted solved the problem for me.

Not only does that not work empirically for me, I believe that the
following excerpts from wid-edit.el show that your patch can not
possibly have any effect.  Did you really follow my recipe literally
and did you really not use any code that is different from the current
uncustomized CVS?

The problem seems to be:

(define-widget 'other 'sexp
(define-widget 'sexp 'editable-field
(define-widget 'editable-field 'default

Why not:

(define-widget 'other 'item

with suitable further adjustments, which would, combined with your
patch, solve the problem (or so I believe) ?

(define-widget 'other 'sexp
  "Matches any value, but doesn't let the user edit the value.

So why pattern if after an editable-field?

The details:

Your patch in:

(define-widget 'item 'default

putting in:

 :value-delete 'widget-children-value-delete

can, I believe, not possibly have any effect for 'other.  I followed
the sequence of define-widget's starting at 'other up to 'default:

(define-widget 'other 'sexp
(define-widget 'sexp 'editable-field
(define-widget 'editable-field 'default
  "An editable text field."
  :convert-widget 'widget-value-convert-widget
  :keymap widget-field-keymap
  :format "%v"
  :help-echo "M-TAB: complete field; RET: enter value"
  :value ""
  :prompt-internal 'widget-field-prompt-internal
  :prompt-history 'widget-field-history
  :prompt-value 'widget-field-prompt-value
  :action 'widget-field-action
  :validate 'widget-field-validate
  :valid-regexp ""
  :error "Field's value doesn't match allowed forms"
  :value-create 'widget-field-value-create
  :value-delete 'widget-field-value-delete
  :value-get 'widget-field-value-get
  :match 'widget-field-match)

The last define-widget contains:

  :value-delete 'widget-field-value-delete

The two previous define-widgets do not override this.
Where does 'item fit into all of this?

Your patch works for 'const because:

(define-widget 'const 'item

But my example involves 'other.

I checked with edebug that :value-delete for the widget in question is
indeed 'widget-field-value-delete, as expected from the above code
excerpts.

Sincerely,

Luc.

Re: :format strings in Custom ending in %h

[Luc Teirlinck]

Of course,

(define-widget 'other 'item

or:

(define-widget 'other 'const

with suitable adaptations, would solve the problem for 'other, but
_not_ for others that use `widget-field-value-delete', such as 'int.

So it would not make the original problem go away, but it still would
make sense for the reason I gave in my follow-up message: there should
be no editable field for 'other, since there is no real choice.

Sincerely,

Luc.

Re: :format strings in Custom ending in %h

[Luc Teirlinck]

I should add that there is a further problem with:

(define-widget 'other 'sexp
(define-widget 'sexp 'editable-field
(define-widget 'editable-field 'default

Indeed, %v in :format for 'other will, very misleadingly, present the
user with an editable field, giving the illusion of a choice, which is
not really there.

(This is a follow-up to a previous message in which I pointed out
problems with the above sequence of define-widgets.)

Sincerely,

Luc.

Re: :format strings in Custom ending in %h

[Luc Teirlinck]

>From my previous message:

   Indeed, %v in :format for 'other will, very misleadingly, present the
   user with an editable field, giving the illusion of a choice, which is
   not really there.

I misunderstood this.  The edited value does indeed seem to get used,
whether it fits the intended meaning of "other" or not.  So if the
:format contains a %v, the user can choose "other" to customize the
option as if it were a 'sexp.

I am not immediately sure whether or not that means that

(define-widget 'other 'sexp

is appropriate after all.

Sincerely,

Luc.

Re: :format strings in Custom ending in %h

[Luc Teirlinck]

>From my previous message:

   I am not immediately sure whether or not that means that

   (define-widget 'other 'sexp

   is appropriate after all.

It probably is.  I probably got drawn into a side-track here.  The
real problem is with `widget-field-value-delete', not with the above
"define-widget".  One probably should not use %v in a :format for
"other" to begin with.

Sincerely,

Luc.

:format strings in Custom ending in %h

[Luc Teirlinck]

Is it a rule that :format strings are not allowed to end in %h?
If so where is this mentioned in either the Elisp manual or the
(widget) documentation?

Such format strings produce trouble as described below.

Do:

emacs-21.3.50 -q --eval "(blink-cursor-mode 0)" &

and load the following file:

===File ~/customdoc.el======================================
(defgroup nifty nil
  "Single item group"
  :group 'convenience)

(defcustom nifty-var nil
  "*Nifty doc string."
  :group 'nifty-group
  :type '(choice (const :tag "True"
			:format "%t\n%h"
			:doc
			"True stuff.
Second line of true stuff."
			t)
		 (other :tag "False"
			:format "%t\n%h"
			:doc
			"Falsehoods.
Second line of falsehoods"
			nil)))

============================================================

Then do:

M-x customize-group RET nifty

Click on "More" next to "Falsehoods."

Then click on "Value Menu" and choose "True".

Now we have:

True stuff. More
Falsehoods. Hide Rest
Second line of falsehoods
 
We should not have those last two lines any more and if we add a space
to both :format strings, i.e :format "%t\n%h ", and repeat the
exercise, they indeed disappear and everything works wonderfully.

Sincerely,

Luc.