Closed Bug 276299 Opened 20 years ago Closed 19 years ago

"Mozilla Wallpaper.bmp" should be localizable.

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set
minor

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)

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
Severity: enhancement → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Is this OS=ALL bug?
Hmm, it seems more investigation needed...
Keywords: l12y
Depends on: 260437
Torisugari: Does this patch need to be updated to trunk, or is it ready for review ?
(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?
I asked because I wanted you to mark it "review?" from for example Dean or Neil :)
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.
Attached patch patch v2 (obsolete) — Splinter Review
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)
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 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+
Attached patch version 2.1 (obsolete) — Splinter Review
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 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+
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.
Attachment #198911 - Attachment is obsolete: true
Attachment #198982 - Flags: superreview+
Attachment #198982 - Flags: review+
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Should the ".bmp" part really be localizable?

Also, shouldn't there be a localization note about what the %S means?
I agree about comment 14, so feel free to reopen.

-> verified
Status: RESOLVED → VERIFIED
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?
Adds a localization note to wallpaperFile and promptText. Neil had already done
the ".bmp" appending before checking in attachment 198982 [details] [diff] [review].
Attachment #199110 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #199110 - Flags: review?(neil.parkwaycc.co.uk) → review?(kairo)
Comment on attachment 199110 [details] [diff] [review]
adds localization notes

Thos L10n note look good to me ;-)
Attachment #199110 - Flags: review?(kairo) → review+
Attachment #199110 - Flags: superreview?(jag)
Attachment #199110 - Flags: superreview?(jag) → superreview+
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.
L10n notes checked in as well, thanks!
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 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+
Please advise if you want me to provide a patch for branch checkin.
Err.. Comment 23 was mine. Yay for public access computers.
(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
Whiteboard: fixed-seamonkey1.0
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: