213994 - Hide Incompatible Themes

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[mozilla-bugs]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030726 Mozilla Firebird/0.6.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030726 Mozilla Firebird/0.6.1

Hello.

First of all, I'm using a fresh ~/.phoenix profile directory.  I've downloaded
tonight's nightly and extracted it.  I customized it in options and then chose
to install a few new themes from the default web site.  However, two problems
followed.

(1) When 'Use this theme' checkbox is checked the browser *did not* apply the theme.
(2) When I manually selected a new theme in 'Options' and clicked OK the
dialogue box refused to vanish and the theme was not applied.  However, when I
clicked on the default theme and clicked OK the dialogue box disappeared and the
theme remained on default.

Also, the texturizer theme site popped up a particular dialogue box repeatedly
which said '421 There are too many connections from your internet address' even
though I only went to it once from my IP address.

I'd be grateful if firebird or the themes could be updated to work with one another.

With regards.
Dhruba Bandopadhyay.

Reproducible: Always

Steps to Reproduce:
1. Open tonight's firebird nightly with new profile directory.
2. Install a new theme.
3. Try and apply it.

Actual Results:  
The dialogue box refused to disappear and theme was not applied.  Any new themes
could not be applied at all.

Expected Results:  
It should :-

(1) Apply the theme immediately when the checkbox 'Use this theme' is checked
when installing a new theme.
(2) Apply the theme when asked to do so in 'Options'.

Comment 1

[Andrew Benton]

I got this 421 to many internet connections error on the texturiser themes page
earlier this evening when I was using the Firebird 6.1 release candidate but
Iwas able to install themes successfully in that build

Comment 2

[alanjstr]

As posted on MozillaZine, this is due to the skinVersion change.  

Therefore this bug is probably invalid.  

But it does raise another issue:  how to prevent the installation of
incompatible themes in the first place.  It certainly sounds like the installer
says it was succesful.  

Comment 3

[David P James]

See if you can install and use the Pinball 0.6.7 theme from
http://members.rogers.com/dpjames/firebird/pinball.html

If you can, then this bug is invalid since I updated Pinball to use version
skinVersion 1.5

Comment 4

[David P James]

*** Bug 214005 has been marked as a duplicate of this bug. ***

Comment 5

[mozilla-bugs]

The pinball theme applies fine but no other ones do.  However, user should be
alerted of incorrect theme versions or unsuccessful installations somehow as
pointed out.  Also, how long before all other texturizer themes get updated? 
Many thanks.

Comment 6

[Mike Connor [:mconnor]]

Bug is definitely INVALID, older themes are not going to be supported due to
changes in CSS support for -moz attributes.  Its just been a long while since
the skinVersion changed afaik.

Comment 7

[David P James]

Mike:

Alan and Dhruba are correct though that Firebird reported as successful the
installation of incompatible/deprecated themes, and that is definitely a bug.
The fact that the reporter originally misdiagnosed the bug doesn't mean it isn't
one (I'm eating my own words here I realize...). Anyway, changing the summary
from "Cannot apply newly installed themes" to "Firebird "successfully" installs
incompatible themes".

Comment 8

[David P James]

Confirming and taking QA contact.

Comment 9

[James Ross]

I believe it's dying on line 136 of pref-themes.js
(http://lxr.mozilla.org/mozilla/source/browser/components/prefwindow/content/pref-themes.js#136)

Comment 10

[Mike Connor [:mconnor]]

taking

Comment 11

[Mike Connor [:mconnor]]

Attachment: patch

since the description info about this being an incompatible theme isn't
visible, we're running into this now, patch restores the popup functionality

Comment 12

[Mike Connor [:mconnor]]

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

need r= and checkin :)

Comment 13

[Mike Connor [:mconnor]]

*** Bug 214929 has been marked as a duplicate of this bug. ***

Comment 14

[noririty]

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

We can't close dialogbox after pressing "OK".

Comment 15

[Mike Connor [:mconnor]]

Right, we need to pick another theme since the one we picked wasn't valid.

We can bypass this, but I don't think its necessarily the right way of doing this.

Comment 16

[Mike Connor [:mconnor]]

patch checked in

Comment 17

[Mike Connor [:mconnor]]

Verified using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b)
Gecko/20030817 Mozilla Firebird/0.6.1+

Comment 18

[Ben Goodger (use ben at mozilla dot org for email)]

BZZZZT

Wrong fix, and you busted the options dialog. Showing an alert after the fact
(and after the user may have switched away from themes and to somewhere else) is
not good enough UI to indicate that their theme choice is invalid. Either the
theme should not show up in the list or it should not be selectable or something. 

Whatever the case, this patch broke the options dialog because it attempted to
access nodes in the dialog that are not present if the user switches panels away
from Options. You cannot access the pref pane document from Options callback
functions. Please, please, please, be more careful. 

Comment 19

[Mike Connor [:mconnor]]

Attachment: patch to hide incompatible themes

Ben, I wasn't even thinking of applyTheme being a callback, I should have known
better.  As it was I was just fixing broken code by providing correct
variables, hence I wasn't thinking it was broken before.

This patch simplifies the dialog, however to avoid this being a problem with
UI, the theme install needs a check for validity.  Hopefully once the theme
versions settle down, the theme site's browser detection work will allow
effective limiting of choices, lessening the chance of installing an
incompatible theme.

We also need to fix bug 214465 and bug 191992 for this fix to be ideal.

Comment 20

[Mike Connor [:mconnor]]

Attachment: ugly hack to show default theme

also removes the extra var oldTheme = null which isn't needed.

Comment 21

[Mike Connor [:mconnor]]

Attachment: another hack while I'm at it

this one is to hide the modern theme as well

Comment 22

[Mike Connor [:mconnor]]

Comment on attachment 130741 [details] [diff] [review]
another hack while I'm at it

Ben, I know this has a couple of ugly hacks going, we can strip those out once
we figure out what the Marvels of RDF are causing.  (bug 191992 and bug 214465
to be precise)

Comment 23

[Mike Connor [:mconnor]]

*** Bug 215707 has been marked as a duplicate of this bug. ***

Comment 24

[Mike Connor [:mconnor]]

On further investigation, I don't know if this is what broke prefs after all,
since backing this out fixed nothing as far as I can tell.

In any case, I think the last patch I submitted is a better solution, and fixes
a couple other bugs in the process (via crappy hacks, but it works)

Comment 25

[Mike Connor [:mconnor]]

Attachment: fix stupid perf bug

bleh

Comment 26

[Mike Connor [:mconnor]]

Comment on attachment 131150 [details] [diff] [review]
fix stupid perf bug

fixes stupid perf bug from repeating removeInvalidThemes() call for each theme
(genious move there)

now that bug 214465 is fixed, I'd like to see this in for 0.7 if possible,
should be low-risk, I've put plenty of testing into it by now.

Comment 27

[Mike Connor [:mconnor]]

-> 0.7, in the hopes that we can take this fix instead of fixing just 215707

Comment 28

[Mike Connor [:mconnor]]

Attachment: patch above minus modern hack

blake wanted the modern entry left in the list (bug 191992 covers this)

Comment 29

[Mike Connor [:mconnor]]

patch landed on trunk, waiting on ok for landing on branch for 0.7

Comment 30

[Mike Connor [:mconnor]]

Fixed on trunk, still need to get this on the branch for 1.5/0.7

Bryner, okay with taking this for 0.7?  Its in the current nightly, will not
cause problems, and fixed a regression caused by the improper backout of the
previous patch for this bug.

Already have a blanket a=asa for checkins to the branch that are /browser or
/toolkit specific, just need a module peer/owner to okay it as well.

Comment 31

[Blake Ross]

I think this can go into 0.7.

Comment 32

[Mike Connor [:mconnor]]

Fixed in 0.7.

Comment 33

[Simon Paquet [:sipaq]]

verified fixed 2003-11-09