* Re: master 527413f: Go back to not using custom-push-theme when enabling a theme [not found] ` <20201106123612.2172F20B2C@vcs0.savannah.gnu.org> @ 2020-11-06 15:04 ` Stefan Monnier 2020-11-06 15:30 ` Mauro Aranda 0 siblings, 1 reply; 4+ messages in thread From: Stefan Monnier @ 2020-11-06 15:04 UTC (permalink / raw) To: Mauro Aranda; +Cc: emacs-devel > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: master 527413f: Go back to not using custom-push-theme when enabling a theme 2020-11-06 15:04 ` master 527413f: Go back to not using custom-push-theme when enabling a theme Stefan Monnier @ 2020-11-06 15:30 ` Mauro Aranda 2020-11-06 16:47 ` Stefan Monnier 0 siblings, 1 reply; 4+ messages in thread From: Mauro Aranda @ 2020-11-06 15:30 UTC (permalink / raw) To: Stefan Monnier; +Cc: emacs-devel [-- 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 --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: master 527413f: Go back to not using custom-push-theme when enabling a theme 2020-11-06 15:30 ` Mauro Aranda @ 2020-11-06 16:47 ` Stefan Monnier 2020-11-07 13:01 ` Mauro Aranda 0 siblings, 1 reply; 4+ messages in thread From: Stefan Monnier @ 2020-11-06 16:47 UTC (permalink / raw) To: Mauro Aranda; +Cc: emacs-devel > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: master 527413f: Go back to not using custom-push-theme when enabling a theme 2020-11-06 16:47 ` Stefan Monnier @ 2020-11-07 13:01 ` Mauro Aranda 0 siblings, 0 replies; 4+ messages in thread From: Mauro Aranda @ 2020-11-07 13:01 UTC (permalink / raw) To: Stefan Monnier; +Cc: emacs-devel [-- 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 --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-11-07 13:01 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20201106123610.24601.8123@vcs0.savannah.gnu.org>
[not found] ` <20201106123612.2172F20B2C@vcs0.savannah.gnu.org>
2020-11-06 15:04 ` master 527413f: Go back to not using custom-push-theme when enabling a theme Stefan Monnier
2020-11-06 15:30 ` Mauro Aranda
2020-11-06 16:47 ` Stefan Monnier
2020-11-07 13:01 ` Mauro Aranda
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).