276299 - "Mozilla Wallpaper.bmp" should be localizable.

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[O. Atsushi (Torisugari)]

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

Comment 1

[O. Atsushi (Torisugari)]

Attachment: patch v1 [suite, windows] (maybe already obsolete)

Is this OS=ALL bug?
Hmm, it seems more investigation needed...

Comment 2

[O. Atsushi (Torisugari)]

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

Comment 3

[Vidar Haarr (not reading bugmail)]

Torisugari: Does this patch need to be updated to trunk, or is it ready for review ?

Comment 4

[O. Atsushi (Torisugari)]

(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?

Comment 5

[Vidar Haarr (not reading bugmail)]

I asked because I wanted you to mark it "review?" from for example Dean or Neil :)

Comment 6

[[e-mail]]

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.

Comment 7

[Vidar Haarr (not reading bugmail)]

Attachment: patch v2

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 [e-mail] tested his original patch. :-)

Comment 8

[Vidar Haarr (not reading bugmail)]

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

[[e-mail]]

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).

Comment 10

[Vidar Haarr (not reading bugmail)]

Attachment: version 2.1

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.

Comment 11

[jag (Peter Annema)]

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

Comment 12

[Vidar Haarr (not reading bugmail)]

Attachment: patch for checkin, I hope.

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.

Comment 13

[[e-mail]]

Fix checked in.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

Should the ".bmp" part really be localizable?

Also, shouldn't there be a localization note about what the %S means?

Comment 15

[O. Atsushi (Torisugari)]

I agree about comment 14, so feel free to reopen.

-> verified

Comment 16

[jag (Peter Annema)]

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?

Comment 17

[Vidar Haarr (not reading bugmail)]

Attachment: adds localization notes

Adds a localization note to wallpaperFile and promptText. Neil had already done
the ".bmp" appending before checking in attachment 198982 [details] [diff] [review].

Comment 18

[Robert Kaiser]

Comment on attachment 199110 [details] [diff] [review]
adds localization notes

Thos L10n note look good to me ;-)

Comment 19

[Vidar Haarr (not reading bugmail)]

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

[Robert Kaiser]

L10n notes checked in as well, thanks!

Comment 21

[[e-mail]]

Comment on attachment 198982 [details] [diff] [review]
patch for checkin, I hope.

(including the L10N notes of course)

Comment 22

[Robert Kaiser]

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

Comment 23

[Martijn Wargers (dead)]

Please advise if you want me to provide a patch for branch checkin.

Comment 24

[Vidar Haarr (not reading bugmail)]

Err.. Comment 23 was mine. Yay for public access computers.

Comment 25

[[e-mail]]

(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.