GNU bug report logs - #63290
30.0.50; Customize UI shows extra fields for (choice (const ...) (alist ...))

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 63290 in the body.
You can then email your comments to 63290 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; Customize UI shows extra fields for (choice (const ...)
 (alist ...))
Date: Fri, 05 May 2023 02:02:53 -0400

This test case shows the issue:

(defcustom test-custom nil "" :type
  '(choice (alist
	    :key-type (string :tag "key")
	    :value-type (string :tag "value"))
           (const :tag "auto" nil)))
(customize-variable 'test-custom)

The UI first shows:

Hide Test Custom: Choice: Value Menu Alist:
INS
    State : STANDARD.

Then if I choose "Value Menu", and option 1 to choose the "auto" const
value, I get:

Hide Test Custom: Choice: Value Menu auto
    State : EDITED, shown value does not take effect until you set or save it.

which is fine.  Then if I choose "Value Menu" again and choose 0 for the
Alist, I get:

Hide Test Custom: Choice: Value Menu Alist:
INS DEL key: 
            value: 
INS
    State : EDITED, shown value does not take effect until you set or save it.

I wasn't expecting:

INS DEL key: 
            value: 

If I then save the customization, test-custom is ("" . "").  I think it
should instead be nil.

I noticed this on excorporate-configuration, which has:

(defcustom test-custom nil "" :type
  '(choice (const :tag "auto" nil)
           (alist
	    :key-type (string :tag "key")
	    :value-type (string :tag "value"))))

but where the alist is a large nested structure.  If the user
customizes, test-custom, selects the alist, and saves, the structure has
degenerate ("" . ""), or (nil . nil) entries in it.  To avoid this, the
user would have to hit "DEL" on the empty key/value entries, which is
not ideal.

It seems like after a const is shown, Customize considers the variable
"edited".  I don't know why it is adding those extra INS/DEL key/value
UI boxes though.

Thomas

Message #8 received at 63290 <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
Cc: 63290 <at> debbugs.gnu.org
Subject: Re: bug#63290: 30.0.50; Customize UI shows extra fields for (choice
 (const ...) (alist ...))
Date: Sun, 16 Jul 2023 10:15:26 -0300

Thomas Fitzsimmons <fitzsim <at> fitzsim.org> writes:

> This test case shows the issue:
>
> (defcustom test-custom nil "" :type
>   '(choice (alist
>         :key-type (string :tag "key")
>         :value-type (string :tag "value"))
>            (const :tag "auto" nil)))
> (customize-variable 'test-custom)
>
> The UI first shows:
>
> Hide Test Custom: Choice: Value Menu Alist:
> INS
>     State : STANDARD.
>
> Then if I choose "Value Menu", and option 1 to choose the "auto" const
> value, I get:
>
> Hide Test Custom: Choice: Value Menu auto
>     State : EDITED, shown value does not take effect until you set or 
save it.
>
> which is fine.  Then if I choose "Value Menu" again and choose 0 for the
> Alist, I get:
>
> Hide Test Custom: Choice: Value Menu Alist:
> INS DEL key:
>             value:
> INS
>     State : EDITED, shown value does not take effect until you set or 
save it.
>
> I wasn't expecting:
>
> INS DEL key:
>             value:
>
> If I then save the customization, test-custom is ("" . "").  I think it
> should instead be nil.
>

Thanks for the bug report.

>
> It seems like after a const is shown, Customize considers the variable
> "edited".  I don't know why it is adding those extra INS/DEL key/value
> UI boxes though.

The code currently ignores the value if it's present but nil. That's
not good, obviously.  But it might be tricky to fix it because other
widgets depend on the code ignoring it...

I think it might be good to have a different property specify a default
value (defaulting to :value upon creation, if not provided), and let
:value be treated as of today, like the current value holder.

Message #11 received at 63290 <at> debbugs.gnu.org (full text, mbox):

From: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 63290 <at> debbugs.gnu.org
Subject: Re: bug#63290: 30.0.50; Customize UI shows extra fields for (choice
 (const ...) (alist ...))
Date: Sun, 16 Jul 2023 22:37:21 -0400

Mauro Aranda <maurooaranda <at> gmail.com> writes:

> Thomas Fitzsimmons <fitzsim <at> fitzsim.org> writes: 

[...] 

>> It seems like after a const is shown, Customize considers the 
>> variable "edited".  I don't know why it is adding those extra 
>> INS/DEL key/value UI boxes though. 
> 
> The code currently ignores the value if it's present but 
> nil. That's not good, obviously.  But it might be tricky to fix 
> it because other widgets depend on the code ignoring it... 
> 
> I think it might be good to have a different property specify a 
> default value (defaulting to :value upon creation, if not 
> provided), and let :value be treated as of today, like the 
> current value holder. 

OK, thanks for taking a look; let me know if you want me to try a 
patch.  Sounds like a good way of maintaining backward 
compatibility with respect to other widgets.

Or you can test it against excorporate-configuration in GNU ELPA; 
just search for the FIXME that mentions this bug report.

Thanks,
Thomas

Message #14 received at 63290 <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>, 63290 <at> debbugs.gnu.org
Subject: Re: bug#63290: 30.0.50; Customize UI shows extra fields for (choice
 (const ...) (alist ...))
Date: Wed, 9 Aug 2023 09:19:37 -0300

[Message part 1 (text/plain, inline)]

tags 63290 patch
quit


I ended up adding a custom :default-get function for the list widget, to
make it respect a nil value as the :value.  This should be backward
compatible with other widgets, and should fix these "ghost" elements
insertions.

I also added a test for cus-edit-tests.

[0001-Respect-the-value-property-in-a-list-widget-Bug-6329.patch (text/x-patch, attachment)]

Message #19 received at 63290 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 63290 <at> debbugs.gnu.org, fitzsim <at> fitzsim.org
Subject: Re: bug#63290: 30.0.50;
 Customize UI shows extra fields for (choice (const ...) (alist ...))
Date: Wed, 09 Aug 2023 15:53:39 +0300

> Date: Wed, 9 Aug 2023 09:19:37 -0300
> From: Mauro Aranda <maurooaranda <at> gmail.com>
> 
> I ended up adding a custom :default-get function for the list widget, to
> make it respect a nil value as the :value.  This should be backward
> compatible with other widgets, and should fix these "ghost" elements
> insertions.
> 
> I also added a test for cus-edit-tests.

Thanks.

Message #22 received at 63290 <at> debbugs.gnu.org (full text, mbox):

From: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 63290 <at> debbugs.gnu.org
Subject: Re: bug#63290: 30.0.50; Customize UI shows extra fields for (choice
 (const ...) (alist ...))
Date: Wed, 09 Aug 2023 11:51:00 -0400

Hi Mauro,

Mauro Aranda <maurooaranda <at> gmail.com> writes:

> tags 63290 patch quit 
>  
> I ended up adding a custom :default-get function for the list 
> widget, to make it respect a nil value as the :value.  This 
> should be backward compatible with other widgets, and should fix 
> these "ghost" elements insertions. 
> 
> I also added a test for cus-edit-tests. 

Can you try this patch with:

M-x package-install RET excorporate RET

Then:

M-x customize-variable RET excorporate-configuration RET

then select "Value Menu" and 3, which is "EWS URL OAuth 2.0 
settings (no autodiscovery)".  With your wis-edit.el patch applied 
I still get empty values for:

   INS DEL Argument name: 
           Argument value:

and:

   INS DEL OAuth 2.0 setting name: 
           OAuth 2.0 setting value:

and when I apply the setting the value contains: 

 (... (... (#1# . #1#)) 
  (#1# . #1#))

Maybe this is a more complicated case than the test case I 
provided (which does now work for me with your patch)?

Thanks,
Thomas

Message #25 received at 63290 <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
Cc: 63290 <at> debbugs.gnu.org
Subject: Re: bug#63290: 30.0.50; Customize UI shows extra fields for (choice
 (const ...) (alist ...))
Date: Wed, 9 Aug 2023 12:56:50 -0300

Thomas Fitzsimmons <fitzsim <at> fitzsim.org> writes:

> Hi Mauro,
>
> Mauro Aranda <maurooaranda <at> gmail.com> writes:
>
>> tags 63290 patch quit  I ended up adding a custom :default-get
>> function for the list widget, to make it respect a nil value as the
>> :value.  This should be backward compatible with other widgets, and
>> should fix these "ghost" elements insertions. I also added a test
>> for cus-edit-tests.
>
> Can you try this patch with:
>
> M-x package-install RET excorporate RET
>
> Then:
>
> M-x customize-variable RET excorporate-configuration RET
>
> then select "Value Menu" and 3, which is "EWS URL OAuth 2.0 settings
> (no autodiscovery)".  With your wis-edit.el patch applied I still get
> empty values for:
>
>    INS DEL Argument name:             Argument value:
>
> and:
>
>    INS DEL OAuth 2.0 setting name:             OAuth 2.0 setting
>   value:
>
> and when I apply the setting the value contains:   (... (... (#1#
> . #1#))    (#1# . #1#))
>
> Maybe this is a more complicated case than the test case I provided
> (which does now work for me with your patch)?
>
> Thanks,
> Thomas

Oh, OK.  I'll take a look at it later today or tomorrow.

Thanks.

Message #28 received at 63290 <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
Cc: 63290 <at> debbugs.gnu.org
Subject: Re: bug#63290: 30.0.50; Customize UI shows extra fields for (choice
 (const ...) (alist ...))
Date: Wed, 9 Aug 2023 15:03:24 -0300

Thomas Fitzsimmons <fitzsim <at> fitzsim.org> writes:

> Hi Mauro,
>
> Mauro Aranda <maurooaranda <at> gmail.com> writes:
>
>> tags 63290 patch quit  I ended up adding a custom :default-get
>> function for the list widget, to make it respect a nil value as the
>> :value.  This should be backward compatible with other widgets, and
>> should fix these "ghost" elements insertions. I also added a test
>> for cus-edit-tests.
>
> Can you try this patch with:
>
> M-x package-install RET excorporate RET
>
> Then:
>
> M-x customize-variable RET excorporate-configuration RET
>
> then select "Value Menu" and 3, which is "EWS URL OAuth 2.0 settings
> (no autodiscovery)".  With your wis-edit.el patch applied I still get
> empty values for:
>
>    INS DEL Argument name:             Argument value:
>
> and:
>
>    INS DEL OAuth 2.0 setting name:             OAuth 2.0 setting
>   value:
>
> and when I apply the setting the value contains:   (... (... (#1#
> . #1#))    (#1# . #1#))
>
> Maybe this is a more complicated case than the test case I provided
> (which does now work for me with your patch)?
>
> Thanks,
> Thomas

I took a look and this seems to have something to do with how the alist
widget (and almost surely the plist widget) get created when they have
:options.  I'm not sure yet, and I don't totally understand what's
happening as of now, but I wanted to report back.

I'll try to investigate and see how this can be fixed, but it's going to
take me some time.

Message #31 received at 63290 <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
Cc: 63290 <at> debbugs.gnu.org
Subject: Re: bug#63290: 30.0.50; Customize UI shows extra fields for (choice
 (const ...) (alist ...))
Date: Thu, 10 Aug 2023 19:58:24 -0300

[Message part 1 (text/plain, inline)]

Thomas Fitzsimmons <fitzsim <at> fitzsim.org> writes:

> Hi Mauro,
>
> Mauro Aranda <maurooaranda <at> gmail.com> writes:
>
>> I ended up adding a custom :default-get
>> function for the list widget, to make it respect a nil value as the
>> :value.  This should be backward compatible with other widgets, and
>> should fix these "ghost" elements insertions. I also added a test
>> for cus-edit-tests.
>
> Can you try this patch with:
>
> M-x package-install RET excorporate RET
>
> Then:
>
> M-x customize-variable RET excorporate-configuration RET
>
> then select "Value Menu" and 3, which is "EWS URL OAuth 2.0 settings
> (no autodiscovery)".  With your wis-edit.el patch applied I still get
> empty values for:
>
>    INS DEL Argument name:             Argument value:
>
> and:
>
>    INS DEL OAuth 2.0 setting name:             OAuth 2.0 setting
>   value:
>
> and when I apply the setting the value contains:   (... (... (#1#
> . #1#))    (#1# . #1#))
>
> Maybe this is a more complicated case than the test case I provided
> (which does now work for me with your patch)?

I think this ammended patch fixes it.  Since we want
widget-list-default-get to respect a nil :value property, the alist
widget needs to be modified so that its default value is nil.

[0001-Respect-the-value-property-in-a-list-widget-Bug-6329.patch (text/x-patch, attachment)]

Message #34 received at 63290 <at> debbugs.gnu.org (full text, mbox):

From: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 63290 <at> debbugs.gnu.org
Subject: Re: bug#63290: 30.0.50; Customize UI shows extra fields for (choice
 (const ...) (alist ...))
Date: Fri, 11 Aug 2023 09:29:24 -0400

Mauro Aranda <maurooaranda <at> gmail.com> writes:

> Thomas Fitzsimmons <fitzsim <at> fitzsim.org> writes: 
> 
>> Hi Mauro, 
>> 
>> Mauro Aranda <maurooaranda <at> gmail.com> writes: 
>> 
>>> I ended up adding a custom :default-get function for the list 
>>> widget, to make it respect a nil value as the :value.  This 
>>> should be backward compatible with other widgets, and should 
>>> fix these "ghost" elements insertions. I also added a test for 
>>> cus-edit-tests. 
>> 
>> Can you try this patch with: 
>> 
>> M-x package-install RET excorporate RET 
>> 
>> Then: 
>> 
>> M-x customize-variable RET excorporate-configuration RET 
>> 
>> then select "Value Menu" and 3, which is "EWS URL OAuth 2.0 
>> settings (no autodiscovery)".  With your wis-edit.el patch 
>> applied I still get empty values for: 
>>     INS DEL Argument name:             Argument value:  
>> and: 
>>     INS DEL OAuth 2.0 setting name:             OAuth 2.0 
>>setting    value:  
>> and when I apply the setting the value contains:   
>> (... (... (#1# . #1#))    (#1# . #1#)) 
>> 
>> Maybe this is a more complicated case than the test case I 
>> provided (which does now work for me with your patch)? 
> 
> I think this ammended patch fixes it.  Since we want 
> widget-list-default-get to respect a nil :value property, the 
> alist widget needs to be modified so that its default value is 
> nil. 

With the updated patch, when I select "EWS URL OAuth 2.0 settings 
(no autodiscovery)", all the widgets are disabled.  The blank 
values are no longer added though.  However, if I then set the 
value, without configuring anything, excorporate-configuration 
stays nil.  So I don't think the patch is correct yet.

Thomas

Message #37 received at 63290 <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
Cc: 63290 <at> debbugs.gnu.org
Subject: Re: bug#63290: 30.0.50; Customize UI shows extra fields for (choice
 (const ...) (alist ...))
Date: Tue, 15 Aug 2023 19:46:35 -0300

[Message part 1 (text/plain, inline)]

Hi Thomas,

Thomas Fitzsimmons <fitzsim <at> fitzsim.org> writes:

> Mauro Aranda <maurooaranda <at> gmail.com> writes:
>>> Can you try this patch with: M-x package-install RET excorporate
>>> RET Then: M-x customize-variable RET excorporate-configuration RET
>>> then select "Value Menu" and 3, which is "EWS URL OAuth 2.0
>>> settings (no autodiscovery)".  With your wis-edit.el patch applied
>>> I still get empty values for:     INS DEL Argument
>>> name:             Argument value:  and:     INS DEL OAuth 2.0
>>> setting name:             OAuth 2.0 setting value:  and when I
>>> apply the setting the value contains:   (... (... (#1# . #1#))
>>> (#1# . #1#)) Maybe this is a more complicated case than the test
>>> case I provided (which does now work for me with your patch)?
>> I think this ammended patch fixes it.  Since we want
>> widget-list-default-get to respect a nil :value property, the alist
>> widget needs to be modified so that its default value is nil.
>
> With the updated patch, when I select "EWS URL OAuth 2.0 settings (no
> autodiscovery)", all the widgets are disabled.  The blank values are
> no longer added though.  However, if I then set the value, without
> configuring anything, excorporate-configuration stays nil. So I don't
> think the patch is correct yet.

So, in case the :value is missing for the alist widget, we want to
compute the default-value with the :options, and without including the
editable-list.

Hopefully the attached patch is 100% correct now.  I did try it with
excorporate-configuration and I think it works OK, but please give it
yourself a try.  And thank you for your patience.

[0001-Specialize-default-get-for-alist-widgets-Bug-63290.patch (text/x-patch, attachment)]

Message #40 received at 63290 <at> debbugs.gnu.org (full text, mbox):

From: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 63290 <at> debbugs.gnu.org
Subject: Re: bug#63290: 30.0.50; Customize UI shows extra fields for (choice
 (const ...) (alist ...))
Date: Wed, 16 Aug 2023 11:16:32 -0400

Hi Mauro,

Mauro Aranda <maurooaranda <at> gmail.com> writes:

> Thomas Fitzsimmons <fitzsim <at> fitzsim.org> writes: 
> 
>> Mauro Aranda <maurooaranda <at> gmail.com> writes: 
>>>> Can you try this patch with: M-x package-install RET 
>>>> excorporate RET Then: M-x customize-variable RET 
>>>> excorporate-configuration RET then select "Value Menu" and 3, 
>>>> which is "EWS URL OAuth 2.0 settings (no autodiscovery)".  
>>>> With your wis-edit.el patch applied I still get empty values 
>>>> for:     INS DEL Argument name:             Argument value:  
>>>> and:     INS DEL OAuth 2.0 setting name:             OAuth 
>>>> 2.0 setting value:  and when I apply the setting the value 
>>>> contains:   (... (... (#1# . #1#)) (#1# . #1#)) Maybe this is 
>>>> a more complicated case than the test case I provided (which 
>>>> does now work for me with your patch)? 
>>> I think this ammended patch fixes it.  Since we want 
>>> widget-list-default-get to respect a nil :value property, the 
>>> alist widget needs to be modified so that its default value is 
>>> nil. 
>> 
>> With the updated patch, when I select "EWS URL OAuth 2.0 
>> settings (no autodiscovery)", all the widgets are disabled.  
>> The blank values are no longer added though.  However, if I 
>> then set the value, without configuring anything, 
>> excorporate-configuration stays nil. So I don't think the patch 
>> is correct yet. 
> 
> So, in case the :value is missing for the alist widget, we want 
> to compute the default-value with the :options, and without 
> including the editable-list. 
> 
> Hopefully the attached patch is 100% correct now.  I did try it 
> with excorporate-configuration and I think it works OK, but 
> please give it yourself a try.  And thank you for your patience. 

Yes, after applying this patch, I retried the test case and 
excorporate-configuration and both behave as I was originally 
expecting them to; no stray structures upon setting the value.

Thank you for all your work on this and the comprehensive test 
cases.  Feel free to close this bug report once the patch is 
pushed.

Thanks,
Thomas

Message #45 received at 63290-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 63290-done <at> debbugs.gnu.org, fitzsim <at> fitzsim.org
Subject: Re: bug#63290: 30.0.50;
 Customize UI shows extra fields for (choice (const ...) (alist ...))
Date: Sat, 19 Aug 2023 11:34:24 +0300

> Cc: 63290 <at> debbugs.gnu.org
> Date: Tue, 15 Aug 2023 19:46:35 -0300
> From: Mauro Aranda <maurooaranda <at> gmail.com>
> 
> So, in case the :value is missing for the alist widget, we want to
> compute the default-value with the :options, and without including the
> editable-list.
> 
> Hopefully the attached patch is 100% correct now.  I did try it with
> excorporate-configuration and I think it works OK, but please give it
> yourself a try.  And thank you for your patience.

Thanks, I've now installed this on the master branch, and I'm closing
the bug.

Message #48 received at 63290 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattias.engdegard <at> gmail.com>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 63290 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
Subject: bug#63290: 30.0.50; Customize UI shows extra fields for (choice
 (const ...) (alist ...)) 
Date: Mon, 21 Aug 2023 14:23:38 +0200

Mauro, the test has an unused variable binding, `const-opt`. Would you please confirm that it is an oversight and can be removed, or was your intention that it be used in the test?

Message #51 received at 63290 <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Mattias Engdegård <mattias.engdegard <at> gmail.com>
Cc: 63290 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
Subject: Re: bug#63290: 30.0.50; Customize UI shows extra fields for (choice
 (const ...) (alist ...))
Date: Mon, 21 Aug 2023 11:43:32 -0300

Mattias Engdegård <mattias.engdegard <at> gmail.com> writes:

> Mauro, the test has an unused variable binding, `const-opt`. Would you
> please confirm that it is an oversight and can be removed, or was your
> intention that it be used in the test?

Hi Mattias,

It was a copy-pasta from the previous let.  It should be removed.

Thanks for catching it!

Message #54 received at 63290 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattias.engdegard <at> gmail.com>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 63290 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
Subject: Re: bug#63290: 30.0.50; Customize UI shows extra fields for (choice
 (const ...) (alist ...))
Date: Mon, 21 Aug 2023 17:24:47 +0200

21 aug. 2023 kl. 16.43 skrev Mauro Aranda <maurooaranda <at> gmail.com>:

> It was a copy-pasta from the previous let.  It should be removed.

Now removed.

> Thanks for catching it!

Oh, thank the compiler. I'm just the messenger.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.