224777 - Options-Themes: add an apply button and make doubleclick apply the selected theme

Status: RESOLVED DUPLICATE of bug 170006

(Firefox :: Settings UI, enhancement)

Description

[Steffen Wilberg]

Is there a reason we don't have an apply button in Options-Themes?
There are even localization strings, but no code for this.

I implemented it and it works fine (tested with Orbit Blue and Bluemonkey). Left
to the "uninstall %theme_name%" button is an "apply %theme_name%" button.
Doubleclicking a theme now applies it. This is very convenient because you can
click through all your themes and see how they look like.

Maybe selecting a theme and clicking OK shouldn't change the theme as it does
now? That would be one line to comment out/remove if we wanted it.

Comment 1

[Steffen Wilberg]

Attachment: patch

In pref-themes.js, the same is done to the apply button as to the uninstall
button: It gets the theme name attached, and it's disabled when no proper theme
is selected (like Modern).
Calling the applyTheme() function on doubleclick seems to be the right thing to
do, since that is the function that gets called upon clicking OK. Well, it
works as it should and there's no error.

<button id="selectSkin" label="&selectSkin.label;" oncommand="applyTheme()"/>
<button id="uninstallSkin" label="&uninstallSkin.label;"
oncommand="uninstallSkin();"/>
Theme? Skin? What do you prefer!?

Comment 2

[Mike Connor [:mconnor]]

We really need live theme-switching to work reliably and completely if we want 
this to work properly.  Or, modify this to save the current session and restart 
the browser.  Exposing Apply Theme as a button just makes the issue more acute.

Comment 3

[Steffen Wilberg]

Sure, there are some problems with live-switching, e.g. the icons in the
bookmarks/history sidebar aren't changed until you close and reopen the sidebar.
But this happens regardless if you click OK, click my apply button or
double-click the theme.
I think we should fix these problems (automatically close the sidebar?) instead
of hiding the functionality. And my patch makes it easier because you can switch
back and forth in a splitsecond!
I know the "use this theme" checkbox in the themes install dialog doesn't work.
Maybe we should hide it. Do you know other problems with live-switching?

Comment 4

[Steffen Wilberg]

Some issues are theme-dependent: The icons in the bookmarks toolbar don't change
if you switch from the default theme to Orbit Blue or Phly Warm. It works if you
switch from default to Bluemonkey or Pinball 0.7.5. It also works if you switch
from Bluemonkey or Pinball to Orbit Blue or Phly Warm.
On-doubleclick-switching is really fun! :-)

Comment 5

[Alfred Kayser]

Allthough 'live' theme switching seems to be dependent with themes, the
issue is still somewhere in the mozilla code base itself. 
The issue seems to be mostly related with correctly reloading and applying
images, stylesheets and bindings from the new themes. 
I have noticied the following:
* Images are not reloaded if they use the same filename, but a simple workaround
is to open a new window
* Styles for example of the currently opened submenu are not updated, until a
new window is opened.
* My themes (Nautilus, Walnut, LittleFirebird, MicroFirebird) uses their own
'scrollbar' bindings, which is not applied until a restart of Firebird is done.

Having live switching enabled, including this 'double-click' feature, will force
the mozilla community to really address these painfull issues.

Comment 6

[Steffen Wilberg]

Comment on attachment 134832 [details] [diff] [review]
patch

So I guess it's time to seek review. Pierre, can you have a look at this,
please?

Comment 7

[Pierre Chanial]

The problem with this patch is that it is inconsistent with how the preference
dialog work for the other panel: the changes are only validated if the user
press OK and cancelled if the user press cancel.
Though, that may be more friendly to be able to change the themes without
closing/opening the dialog box. The theme change should be reverted if the user
change a theme then press cancel.
-> ben, the pref guru.

Comment 8

[Steffen Wilberg]

Comment on attachment 134832 [details] [diff] [review]
patch

Ben, we need some help here, see comment 7.

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 134832 [details] [diff] [review]
patch

removing review request, the EM obsoletes this.

Comment 10

[Steffen Wilberg]

> ... the EM obsoletes this.
Indeed (once it works). Removing dependency and marking as duplicate of 170006.

*** This bug has been marked as a duplicate of 170006 ***

Comment 11

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs,
filter on "beltznerLovesGoats" to get rid of this mass change