Closed
Bug 276299
Opened 20 years ago
Closed 19 years ago
"Mozilla Wallpaper.bmp" should be localizable.
Categories
(SeaMonkey :: UI Design, defect)
SeaMonkey
UI Design
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: torisugari, Assigned: vhaarr+bmo)
References
Details
(Keywords: fixed-seamonkey1.0, fixed1.8, l12y)
Attachments
(2 files, 3 obsolete files)
4.10 KB,
patch
|
vhaarr+bmo
:
review+
vhaarr+bmo
:
superreview+
kairo
:
approval-seamonkey1.0+
|
Details | Diff | Splinter Review |
894 bytes,
patch
|
kairo
:
review+
jag+mozilla
:
superreview+
|
Details | Diff | Splinter Review |
According to my desktop property, all file names but "Mozilla Wallpaper" are written in Japanese. IIRC, default wallpapers in Windows95 were also in Japanese. I don't know much about MS GUI guidelines, but it's nice to remove this hard coding.. https://bugzilla.mozilla.org/show_bug.cgi?id=181273#c25
Updated•20 years ago
|
Severity: enhancement → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Reporter | ||
Comment 1•20 years ago
|
||
Is this OS=ALL bug? Hmm, it seems more investigation needed...
Reporter | ||
Comment 2•20 years ago
|
||
OK, accoring to lxr, hardcoded "Wallpaper" is in http://lxr.mozilla.org/mozilla/source/xpfe/components/winhooks/nsWindowsHooks.cpp http://lxr.mozilla.org/mozilla/source/browser/components/shell/src/nsWindowsShellService.cpp http://lxr.mozilla.org/mozilla/source/browser/components/shell/src/nsGNOMEShellService.cpp That is, seamonkey Windows firefox Windows firefox Linux
Assignee | ||
Comment 3•19 years ago
|
||
Torisugari: Does this patch need to be updated to trunk, or is it ready for review ?
Reporter | ||
Comment 4•19 years ago
|
||
(In reply to comment #3) Sorry for the long absence. > Does this patch need to be updated to trunk, > or is it ready for review ? Well, imho, this patch is 99% ready. Only one thing: The locale data enty of the patch is located in mozilla/xpfe/components/winhooks/locale/en-US/nsWindowsHooks.properties However, if we are going to implement this wall paper image functionality on linux : bug 215532 (and mac and so on as well), nsWindowsHooks.properties may not be the proper file. And in that case, I'm not sure which file I should move the entry to, at all. Anybody any idea/info?
Assignee | ||
Comment 5•19 years ago
|
||
I asked because I wanted you to mark it "review?" from for example Dean or Neil :)
Comment 6•19 years ago
|
||
Comment on attachment 169797 [details] [diff] [review] patch v1 [suite, windows] (maybe already obsolete) >+ nsXPIDLString brandName, fileLeaf; You're allowed long variable names, you know... fileLeafName is much more readable. > nsCOMPtr<nsIStringBundleService> bundleService(do_GetService(bundleCID)); >+ NS_ENSURE_TRUE(bundleService, NS_ERROR_FAILURE); You should probably use the 2-arg form of do_GetService so you can return the correct rv on failure.
Assignee | ||
Comment 7•19 years ago
|
||
Patch updated to trunk and fixes issues mentioned in comment #6. Please note that I do not have a Windows environment and can't really test this patch, but it is only attachment 169797 [details] [diff] [review] updated to trunk (chrome://global/locale/brand.properties is now chrome://branding/locale/brand.properties) and two trivial changes, so it should be OK, provided torisugari@gmail.com tested his original patch. :-)
Attachment #169797 -
Attachment is obsolete: true
Attachment #195277 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 8•19 years ago
|
||
Mano: Any thoughts on what to do about the various implementations that exist in toolkit for setting desktop background? The difference in the nsShellService::SetDesktopBackground implementations for the different platforms are crazy (toolkit). For example, for the filename, Windows uses a .properties string, Mac uses the image filename and GNOME uses $brandname_wallpaper.png. the mac impl also supports setting images that are not fully loaded yet as the background because it actually fires off a new SaveURI and implements nsIWebProgressListener, and it fires off a global notification "shell:desktop-background-changed" when it actually sets the background in OnStateChange.
Comment 9•19 years ago
|
||
Comment on attachment 195277 [details] [diff] [review] patch v2 >+ const PRUnichar* stringArray = brandName.get(); My superreview hat tells me that a PRUnichar* is not a string array. You could rename it, or you could tweak the code so that it is actually an array (in which case use NS_ARRAY_LENGTH instead of 1).
Attachment #195277 -
Flags: review?(neil.parkwaycc.co.uk) → review+
Assignee | ||
Comment 10•19 years ago
|
||
Thank you, Neil. Please note, again, that I have no means of testing this patch, so reviewers should take special care and even check for compile errors/warnings.
Assignee: guifeatures → vhaarr+bmo
Attachment #195277 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #198911 -
Flags: superreview?(jag)
Attachment #198911 -
Flags: review+
Comment 11•19 years ago
|
||
Comment on attachment 198911 [details] [diff] [review] version 2.1 Code looks good, just a minor nit to pick. >Index: xpfe/components/winhooks/nsWindowsHooks.cpp >=================================================================== > nsCID bundleCID = NS_STRINGBUNDLESERVICE_CID; >+ nsCOMPtr<nsIStringBundleService> bundleService(do_GetService(bundleCID, &rv)); Just get rid of the nsCID stuff and use the CONTRACTID instead: nsCOMPtr<nsIStringBundleService> bundleService(do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv)); or if you care about the 80 columns style rule: nsCOMPtr<nsIStringBundleService> bundleService = do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv); I don't have a Windows tree I can build right now, let alone test this patch, so if you make that change please have whoever's gonna check this in do a test build first :-) sr=jag
Attachment #198911 -
Flags: superreview?(jag) → superreview+
Assignee | ||
Comment 12•19 years ago
|
||
The nsCID has a brother, so I killed him too. This file doesn't care very much about wrapping on 80, but it may look like it cares sometimes. Personally, I don't care.
Assignee | ||
Updated•19 years ago
|
Attachment #198911 -
Attachment is obsolete: true
Attachment #198982 -
Flags: superreview+
Attachment #198982 -
Flags: review+
Comment 13•19 years ago
|
||
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 14•19 years ago
|
||
Should the ".bmp" part really be localizable? Also, shouldn't there be a localization note about what the %S means?
Reporter | ||
Comment 15•19 years ago
|
||
I agree about comment 14, so feel free to reopen. -> verified
Status: RESOLVED → VERIFIED
Comment 16•19 years ago
|
||
Good call, biesi. The .bmp part doesn't need to be localizable and probably shouldn't be. Vidar, would you mind addressing that, and adding a quick note to nsWindowsHooks.properties to explain what the %S there stands for?
Assignee | ||
Comment 17•19 years ago
|
||
Adds a localization note to wallpaperFile and promptText. Neil had already done the ".bmp" appending before checking in attachment 198982 [details] [diff] [review].
Assignee | ||
Updated•19 years ago
|
Attachment #199110 -
Flags: review?(neil.parkwaycc.co.uk)
Updated•19 years ago
|
Attachment #199110 -
Flags: review?(neil.parkwaycc.co.uk) → review?(kairo)
Comment 18•19 years ago
|
||
Comment on attachment 199110 [details] [diff] [review] adds localization notes Thos L10n note look good to me ;-)
Attachment #199110 -
Flags: review?(kairo) → review+
Assignee | ||
Updated•19 years ago
|
Attachment #199110 -
Flags: superreview?(jag)
Updated•19 years ago
|
Attachment #199110 -
Flags: superreview?(jag) → superreview+
Assignee | ||
Comment 19•19 years ago
|
||
Comment on attachment 199110 [details] [diff] [review] adds localization notes Thanks for the reviews! If someone could help me check this in, that would be great.
Comment 20•19 years ago
|
||
L10n notes checked in as well, thanks!
Comment 21•19 years ago
|
||
Comment on attachment 198982 [details] [diff] [review] patch for checkin, I hope. (including the L10N notes of course)
Attachment #198982 -
Flags: approval-seamonkey1.0?
Comment 22•19 years ago
|
||
Comment on attachment 198982 [details] [diff] [review] patch for checkin, I hope. a=me given you check it in with the same modifications as done on trunk
Attachment #198982 -
Flags: approval-seamonkey1.0? → approval-seamonkey1.0+
Comment 23•19 years ago
|
||
Please advise if you want me to provide a patch for branch checkin.
Assignee | ||
Comment 24•19 years ago
|
||
Err.. Comment 23 was mine. Yay for public access computers.
Comment 25•19 years ago
|
||
(In reply to comment #23) >Please advise if you want me to provide a patch for branch checkin. This case was particularly easy as there were no other changes on trunk.
Keywords: fixed1.8
Whiteboard: fixed-seamonkey1.0
Updated•18 years ago
|
Keywords: fixed-seamonkey1.0
Whiteboard: fixed-seamonkey1.0
You need to log in
before you can comment on or make changes to this bug.
Description
•