76512 - Need dialog for failed theme switch due to version incompatibility

Status: VERIFIED FIXED

(Core Graveyard :: Skinability, defect, P3)

Description

[Todd Pringle]

Hyatt has fixed Bug 53670 which implements versioning in the chrome registry. 
As a result, users will be unable (correctly) to switch to incompatible earlier
themes designed for 6.0/6.01.  We need to inform users via a dialog of this
fact.  This dialog should be encountered when a user selects an incompatible
theme via the View>Apply Theme menu or via the Themes Preference panel. 
Suggested dialog:

-------------------------------------------------------------------------------
You have selected a theme which was designed for an earlier version of Netscape
and is incompatible with your current Netscape version.  Please check the
Netscape Theme Park [is a link possible in this dialog, or would it be modal?]
for an updated version of the [insert name here if possible] theme.  You can
uninstall this theme by clicking "Uninstall."

                  [OK]     [Cancel]     [Uninstall]
--------------------------------------------------------------------------------

Suggestions welcome.  The uninstall functionality is in the product now and it
would be very handy to expose it to users at this point, but it is not critical.
 They can always uninstall from the Theme Pref panel.

Comment 1

[Todd Pringle]

Nominating nsbeta1.

Comment 2

[Viswanath Ramachandran]

why would we let them install an incompatible theme in the first place? should
this check not be done before downloading a theme? there needs to be a way for a
theme to specify its version before it is downloaded ideally. 

Comment 3

[Todd Pringle]

Vishy, the theme park will be sniffing for browser version and prevent 6.x users
from downloading 6.0 themes, but the existing 6.0 users who downloaded themes
from the theme park will need this feature.

Comment 4

[timeless]

I would prefer for the theme list to show the incompatible theme as disabled 
and disable the apply theme button when it's selected.

Comment 5

[Todd Pringle]

Timeless, I agree - I think that is a better solution, but the implementation we
have now is as far as we're going to get for beta, based on discussions with
hyatt.  He can probably tell you more, but I think he's onboard with disabling
being the longer term solution.  Can you file a separate bug for that?  Thanks.

Comment 6

[Viswanath Ramachandran]

Joe/Anatoliy - would you be interested in taking this bug? 

Comment 7

[Anatoliy Averbukh]

I will look at this.

Comment 8

[german]

I agree with timeless suggestions. Disabling incompatible themes is the best way
to prevent users from selecting something that does not work properly. 2 questions:
- if these older themes are shown as disabled how can they be selected to get
uninstalled? Possible solution: In prefs don't disable these themes in the list
but give them a diffrent description in the area below and only disable the
apply button
- what do we do with those users/profiles that have currently an old custom
theme selected and upgradeto mojo? Proposed solution: run the check not only on
theme switching but also the first time a new version of the product is used

I also whoever is taking this bug please use the commonly used dialog overlays
for proper button placements for the respective platforms.

Comment 9

[german]

I agree with timeless suggestions. Disabling incompatible themes is the best way
to prevent users from selecting something that does not work properly. 2 questions:
- if these older themes are shown as disabled how can they be selected to get
uninstalled? Possible solution: In prefs don't disable these themes in the list
but give them a diffrent description in the area below and only disable the
apply button
- what do we do with those users/profiles that have currently an old custom
theme selected and upgradeto mojo? Proposed solution: run the check not only on
theme switching but also the first time a new version of the product is used

I also whoever is taking this bug please use the commonly used dialog overlays
for proper button placements for the respective platforms.

Comment 10

[Todd Pringle]

German, I agree that what you have proposed is the better way to go - it's what
I wanted originally, this was the low cost solution hyatt implemented (and I
agreed to) because of time/resource constraints.  If someone else besides hyatt
can implement as you describe, I'm all for it.  If not, we need a dialog for the
next release at a minimum.

FYI, hyatt's code looks for what theme is selected in the profile and if it is
an old, incompatible 6.0 theme, we open in the default theme (Modern).

Comment 11

[Viswanath Ramachandran]

retargeting to mozilla0.9.2. 

Comment 12

[Todd Pringle]

Vishy, I don't think we can push this off post-beta.  If we do, beta users will
believe theme switching is broken and the press will definitely pick up on this
as well.  They may assume it is because these are 6.0 themes, but I really think
we need the inelegant explanatory dialog solution implemented for beta.

Comment 13

[Viswanath Ramachandran]

retargeting to mozilla0.9.1. reassigning to [e-mail]

Comment 14

[Anatoliy Averbukh]

I am currently working on this bug.

Comment 15

[Anatoliy Averbukh]

Attachment: Please review

Comment 16

[Blake Ross]

Get rid of the tabs and whitespace issues, please.

Comment 17

[Anatoliy Averbukh]

Attachment: tabs and whitespace issues are resolved. Please review

Comment 18

[timeless]

if (NS_FAILED(rv)) return rv;
rv = nsComponentManager::CreateInstance(...)
if (NS_FAILED(rv))
  return NS_OK;

Can we be somewhat consistent about returning the reason for failures, and 
perhaps not returning NS_OK for NS_FAILED() ?

+oldTheme=You have selected a theme which was designed for an earlier version 
of Netscape\nand is incompatible with your current Netscape version. Please 
check the Netscape Theme Park\nfor an updated version of the %theme_name% 
theme. You can uninstall this theme by clicking\n%button% button.

Netscape ?? No. %foo% ? please don't.  please use %s.
This corresponds to a function instead of .replace() and is the correct way to 
support localization.

Comment 19

[Anatoliy Averbukh]

Attachment: new patch

Comment 20

[Anatoliy Averbukh]

Updated error checks and failure processing is made consistently,
also messages are fully localizable now.
Please review.

Comment 21

[David Hallowell]

I still don't think the average Mozilla/Beonex/(insert any other non Netscape
distro) really wants to be informed that their theme will not work in their
current Netscape version :)

I still Netscape mentioned in the most recent patch. I'm sure there's an easy
way to get the product name so it makes it easier for distributors like Netscape
and Beonex to easily brand their browser without doing a global find/replace on
the word mozilla.

Comment 22

[Paul Chen]

Timeless and David Hallowell are right. You need to use brand.properties and get
the string 'brandShortName' which contains the correct name of the app.

It's bad form to hard code "Netscape" into strings that are being checked into
the mozilla tree. ;-)

Comment 23

[timeless]

fwiw, i have a patch for another bug that tries to use brand.properties, but i 
don't know if it works. you can look at it and test if you like... it's for 
imap something

Comment 24

[Anatoliy Averbukh]

Attachment: added brand from brand.properties file. Please review

Comment 25

[timeless]

+      nsCOMPtr<nsIRDFResource> entry = do_QueryInterface(packageSkinEntry);
+         nsCOMPtr<nsIRDFResource> 
packageResource(do_QueryInterface(packageNode))

please be consistent. I prefer the latter.

 {
+  if (themeName.getAttribute("name")=="")
+        return;
^looks like a tab, i think you should be using 2 space indents to match the if.

+    message = message.replace(/%theme_name%/, 
themeName.getAttribute("displayName"));
+    message = message.replace(/%brand%/g, 
gBrandBundle.getString("brandShortName"));

No. you should be using %s and intl subst. 
try following:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/uriloader/exthandler/mac/ns
OSHelperAppService.cpp&rev=1.15&mark=84-96,101-110#71

Comment 26

[Anatoliy Averbukh]

Timeless, 
as localization for brandShortName: it is localizable already. So there is no 
need to use %s there

(you may check code
 profileManager.js, line 64 or
 nsHelperAppDlg.js, line 165 ).



  

Comment 27

[timeless]

It was my understanding that .replace() should not be used for string bundles.

Comment 28

[Anatoliy Averbukh]

Yes, .replace method itself cannot provide localization for a string.
But, if you look at file pref-themes.xul (for instance), there are lines:

<stringbundle id="bundle_brand"
                src="chrome://global/locale/brand.properties"/>

And just that URI for src (chrome://global/locale/brand.properties) provides
the localization.

Also (I checked that again), there are no TABs in my last attachment (id=33768).
Just spaces.
  

Comment 29

[timeless]

BAH!@ it's too hard to explain.

whenever a string could be moved by a localizer, translator, distributor, end 
user, alien, developer ... you should use %s.  Because it's the _standard_ for 
substitution and it has well defined behavior.

Yes the brand thing is localized, but your file that has %foobarheadblah% might 
be localized or translated or commercialized or privatized and it _should_ use 
%s so that people understand what is happening and how to change it.

Comment 30

[Paul Chen]

Jeez, timeless, get that ten foot pole out of your ass, ok? Personally, %brand%
works just as well as %s. In fact, I think it's much clearer.

r=pchen

Comment 31

[timeless]

bug 67372. Alecf and mkaply indicate that it's a standard. If you want to 
reopen that bug or argue it, be my guest.

Comment 32

[Blake Ross]

Hyatt needs to sr the chrome registry part of this.

Yes, it seems picky, but timeless is correct: nsIStringBundle's 
FormatStringFromName is the standardized way of doing this.  XUL 
<stringbundle/>s provide a simplified helper function (formatStringFromName) 
that takes two parameters: the first is the string key, and the second is an 
array of values to be used in place of the %S's (or %1S, %2S, etc, iirc).

Thanks for removing the tabs, but the style in this patch is still not 
correct.  If you didn't write this code and had to modify it later, how happy 
would you be?  Come on.  Spacing should follow the modeline at the top of the 
file. 

Comment 33

[Brendan Eich [:brendan]]

+  nsCAutoString resourceStr( "urn:mozilla:skin" );
+  resourceStr += ":";
+  resourceStr.AppendWithConversion(aProviderName);

Shouldn't aProviderName, which is in wstring or const PRUnichar*, be converted
to UTF-8 before being appended, rather than being chopped down to char?

Blake's right about the indentation.  At one point, the levels are 2, 2, 2, 3,
6, 4 (or 5)!  Why not use Fibonacci numbers?  Seriously, stick with 2.  One last
nit:

+               if (packageName.IsEmpty())
+                   // the package is not represented for current version, so
ignore it
+                    *isCompatible =true;
+

Space on both sides of = please.

/be
/be

Comment 34

[David Hyatt]

Ok, comments on the chrome registry patch.

(1) Fix the indentation to be either 2-space or 4-space, but pick a style and 
stick to it.  Don't mix it up like that.
(2) The IDL method is capitalized and specified with an OUT even though it has 
a single return value.  Please change it to:

boolean checkThemeVersion(in wstring aProviderName, out boolean isCompatible);

(3) I have a standing rule that you can't add a theme function without adding 
the corresponding locale function, since they're exactly the same.  You need to 
also define a checkLocaleVersion, and then make the two functions 
checkThemeVersion and checkLocaleVersion call into a checkProviderVersion that 
is parameterized on "skin" vs. "locale".
(4) Function params start with "a", so you need to change isCompatible to 
aIsCompatible.
(5) Don't capitalize local variables.  Please change ThemePackageVersion to 
themePackageVersion.
(6) You can't use "true" and "false".  You need to use PR_TRUE and PR_FALSE.  
Fix all of the places where you fill in the return result.

Comment 35

[David Hyatt]

Sorry, I forgot to remove the out param.

boolean checkThemeVersion(in wstring aProviderName);

is what I want it changed to.

Comment 36

[Anatoliy Averbukh]

Attachment: new attachment

Comment 37

[Anatoliy Averbukh]

 I added new function for locale (checkLocaleVersion and changed inteface 
functions accordingly)
 Fixed indentation and style things (also changed "true" and "false" to the 
constants).
Please SR.

Comment 38

[Brendan Eich [:brendan]]

Still chopping aProviderName down from PRUnichars to chars -- why?  Should it
not be UTF-8 converted?  If provider names are required to be ISO-Latin-1, say
so in a big flashing comment.

Random if style ('if(...){', 'if (...) {') and scrunched try{ (no space before
the brace) still evident.

Indentation off-by-one error here:

+          }
+               
+           // if just one theme package is NOT compatible, the theme will be
disabled
+           if (!(*aCompatible))
+             return NS_OK;
+
+         }
+      }

/be

Comment 39

[David Hyatt]

sr=hyatt

Comment 40

[Anatoliy Averbukh]

checked-in

Comment 41

[Blake Ross]

Was the last attached past tested and built?  If so, how? Three platforms are 
busted because of this and the rest are sure to follow.  Where's the prototype 
for CheckProviderVersion?

Comment 42

[Dan Rosen]

anatoliya: A poetic remix from http://www.mozilla.org/hacking/rules.html:

  After checking in, you watch Tinderbox until your check-ins clear.
  You do not log out or experiment with drugs.
  You do not become unavailable.
  XP horkage will not be tolerated.

So anyway, you've been backed out -- See yokoyama (sheriff) and #mozilla for
answers. Na Zdorovya!

Comment 43

[cls]

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

Comment 44

[Anatoliy Averbukh]

Forgot to check in file nsChromeRegistry.h.
Will do it today.

Comment 45

[(not reading, please use [e-mail] instead)]

nit:  nsIChromeRegistry.idl, following the interCaps rule:

CheckThemeVersion should be checkThemeVersion

Comment 46

[(not reading, please use [e-mail] instead)]

fixing that would require you to fix pref-themes.js and navigator.js

(I'm only dropping in here because I'm looking for a cause for #81274)

Comment 47

[Anatoliy Averbukh]

Seth, it had already fixed  (look at my attachment with id=34128)
(You probably looked at old one).

Comment 48

[(not reading, please use [e-mail] instead)]

whoops, sorry.  I was looking at the wrong attachment.

I'll put down the crack pipe now.

Comment 49

[Dan Rosen]

anatoliya: We (in #mozilla) noticed that in your checkin to nsChromeRegistry.h,
your method declaration is |NS_IMETHODIMP| -- that is incorrect, and is probably
about to start burning on Windows soon. Please change that to |NS_IMETHOD| ASAP.
In general, please make sure that all files changed are in your patches, so that
all of them can receive appropriate review. Thanks.

Comment 50

[Anatoliy Averbukh]

All stuff was checked in.

Comment 51

[Patty Mac]

Verified.

Comment 52

[Andreas Kunz]

This checkin seems to have caused bug 147028 ("Please check the %brand% Theme
Park") leads to Theme Parks that do not exist.
Sorry, if I'm wrong...