Closed
Bug 76512
Opened 23 years ago
Closed 23 years ago
Need dialog for failed theme switch due to version incompatibility
Categories
(Core Graveyard :: Skinability, defect, P3)
Core Graveyard
Skinability
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.1
People
(Reporter: tpringle, Assigned: anatoliya)
References
Details
Attachments
(5 files)
11.72 KB,
patch
|
Details | Diff | Splinter Review | |
12.34 KB,
patch
|
Details | Diff | Splinter Review | |
12.33 KB,
patch
|
Details | Diff | Splinter Review | |
13.80 KB,
patch
|
Details | Diff | Splinter Review | |
14.71 KB,
patch
|
Details | Diff | Splinter Review |
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 2•23 years ago
|
||
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.
Reporter | ||
Comment 3•23 years ago
|
||
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.
I would prefer for the theme list to show the incompatible theme as disabled and disable the apply theme button when it's selected.
Reporter | ||
Comment 5•23 years ago
|
||
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•23 years ago
|
||
Joe/Anatoliy - would you be interested in taking this bug?
Assignee | ||
Comment 7•23 years ago
|
||
I will look at this.
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.
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.
Target Milestone: mozilla0.9.2 → M1
Reporter | ||
Comment 10•23 years ago
|
||
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).
Reporter | ||
Comment 12•23 years ago
|
||
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•23 years ago
|
||
retargeting to mozilla0.9.1. reassigning to anatoliya@aol.com
Assignee: ben → anatoliya
Target Milestone: mozilla0.9.2 → mozilla0.9.1
Assignee | ||
Comment 15•23 years ago
|
||
Comment 16•23 years ago
|
||
Get rid of the tabs and whitespace issues, please.
Assignee | ||
Comment 17•23 years ago
|
||
Comment 18•23 years ago
|
||
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.
Assignee | ||
Comment 19•23 years ago
|
||
Assignee | ||
Comment 20•23 years ago
|
||
Updated error checks and failure processing is made consistently, also messages are fully localizable now. Please review.
Comment 21•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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
Assignee | ||
Comment 24•23 years ago
|
||
Comment 25•23 years ago
|
||
+ 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
Assignee | ||
Comment 26•23 years ago
|
||
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•23 years ago
|
||
It was my understanding that .replace() should not be used for string bundles.
Comment 28•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
bug 67372. Alecf and mkaply indicate that it's a standard. If you want to reopen that bug or argue it, be my guest.
Depends on: 67372
Comment 32•23 years ago
|
||
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•23 years ago
|
||
+ 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•23 years ago
|
||
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•23 years ago
|
||
Sorry, I forgot to remove the out param. boolean checkThemeVersion(in wstring aProviderName); is what I want it changed to.
Assignee | ||
Comment 36•23 years ago
|
||
Assignee | ||
Comment 37•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
sr=hyatt
Assignee | ||
Comment 40•23 years ago
|
||
checked-in
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 41•23 years ago
|
||
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•23 years ago
|
||
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!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 43•23 years ago
|
||
*** Bug 81083 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 44•23 years ago
|
||
Forgot to check in file nsChromeRegistry.h. Will do it today.
Comment 45•23 years ago
|
||
nit: nsIChromeRegistry.idl, following the interCaps rule: CheckThemeVersion should be checkThemeVersion
Comment 46•23 years ago
|
||
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•23 years ago
|
||
Seth, it had already fixed (look at my attachment with id=34128) (You probably looked at old one).
Comment 48•23 years ago
|
||
whoops, sorry. I was looking at the wrong attachment. I'll put down the crack pipe now.
Comment 49•23 years ago
|
||
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.
Assignee | ||
Comment 50•23 years ago
|
||
All stuff was checked in.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 52•22 years ago
|
||
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...
Updated•16 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•