Re: master 527413f: Go back to not using custom-push-theme when enabling a theme

Re: master 527413f: Go back to not using custom-push-theme when enabling a theme

[Stefan Monnier]

>     Go back to not using custom-push-theme when enabling a theme
>     
>     * lisp/custom.el (enable-theme): Relying on custom-push-theme to
>     handle theme settings and prior user settings was a mistake.  The
>     theme settings haven't changed between loading the theme and enabling
>     it, so we don't need all of what custom-push-theme does.  However, we
>     still need to save a user setting outside of Customize, in order to be
>     able to get back to it, so do that in enable-theme itself.

Hi, could you explain what's the motivation behind this change?
Is it an optimization (i.e. with hopefully no visible effect), or does
it fix an actual problem?  If it fixes an actual problem, could you try
and add a corresponding regression test for it?


        Stefan

Re: master 527413f: Go back to not using custom-push-theme when enabling a theme

[Mauro Aranda]

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Hi, could you explain what's the motivation behind this change?
> Is it an optimization (i.e. with hopefully no visible effect), or does
> it fix an actual problem?  If it fixes an actual problem, could you try
> and add a corresponding regression test for it?

I made a dumb mistake while fixing Bug#34027.  When enabling a theme we
need to change the theme-value or theme-face property of the symbols
the theme customizes, and also save a "changed" value, in case there is
any.  Since custom-push-theme did that, I thought it was fine to call
custom-push-theme here and in disable-theme (I haven't had the time yet
to test if something bad happens in disable-theme).

As a result, theme settings, saved under the theme-settings property of
THEME, got duplicated (you can see the code that adds each setting, at
the end of custom-push-theme).  So, this change fixes that bug I
introduced.  Since the test for Bug#34027, custom--test-theme-variables,
still pass after the change, I didn't consider adding a test for this.
But sure, it should be easy to add one.

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

Re: master 527413f: Go back to not using custom-push-theme when enabling a theme

[Stefan Monnier]

> I made a dumb mistake while fixing Bug#34027.  When enabling a theme we
> need to change the theme-value or theme-face property of the symbols
> the theme customizes, and also save a "changed" value, in case there is
> any.  Since custom-push-theme did that, I thought it was fine to call
> custom-push-theme here and in disable-theme (I haven't had the time yet
> to test if something bad happens in disable-theme).
>
> As a result, theme settings, saved under the theme-settings property of
> THEME, got duplicated (you can see the code that adds each setting, at
> the end of custom-push-theme).  So, this change fixes that bug I
> introduced.

I see, thanks.

> Since the test for Bug#34027, custom--test-theme-variables, still pass
> after the change, I didn't consider adding a test for this.  But sure,
> it should be easy to add one.

That would be very appreciated.


        Stefan

Re: master 527413f: Go back to not using custom-push-theme when enabling a theme

[Mauro Aranda]

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

>> Since the test for Bug#34027, custom--test-theme-variables, still pass
>> after the change, I didn't consider adding a test for this.  But sure,
>> it should be easy to add one.

> That would be very appreciated.

No problem; done in commit 423b6b62296df0558cf16f286dd268e0b49b3bce.

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