Last Comment Bug 276299 - "Mozilla Wallpaper.bmp" should be localizable.
: "Mozilla Wallpaper.bmp" should be localizable.
Status: VERIFIED FIXED
: fixed-seamonkey1.0, fixed1.8, l12y
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
: -- minor (vote)
: ---
Assigned To: Vidar Haarr (not reading bugmail)
:
Mentors:
Depends on: 260437
Blocks:
  Show dependency treegraph
 
Reported: 2004-12-28 18:05 PST by O. Atsushi (Torisugari)
Modified: 2008-07-31 04:22 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 [suite, windows] (maybe already obsolete) (3.16 KB, patch)
2004-12-28 22:18 PST, O. Atsushi (Torisugari)
no flags Details | Diff | Review
patch v2 (3.17 KB, patch)
2005-09-08 08:46 PDT, Vidar Haarr (not reading bugmail)
neil: review+
Details | Diff | Review
version 2.1 (3.67 KB, patch)
2005-10-08 03:42 PDT, Vidar Haarr (not reading bugmail)
vhaarr+bmo: review+
jag-mozilla: superreview+
Details | Diff | Review
patch for checkin, I hope. (4.10 KB, patch)
2005-10-09 00:42 PDT, Vidar Haarr (not reading bugmail)
vhaarr+bmo: review+
vhaarr+bmo: superreview+
kairo: approval‑seamonkey1.0+
Details | Diff | Review
adds localization notes (894 bytes, patch)
2005-10-10 15:47 PDT, Vidar Haarr (not reading bugmail)
kairo: review+
jag-mozilla: superreview+
Details | Diff | Review

Description O. Atsushi (Torisugari) 2004-12-28 18:05:52 PST
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) 2004-12-28 22:18:45 PST
Created attachment 169797 [details] [diff] [review]
patch v1 [suite, windows] (maybe already obsolete)

Is this OS=ALL bug?
Hmm, it seems more investigation needed...
Comment 3 Vidar Haarr (not reading bugmail) 2005-02-03 02:20:48 PST
Torisugari: Does this patch need to be updated to trunk, or is it ready for review ?
Comment 4 O. Atsushi (Torisugari) 2005-03-01 01:19:45 PST
(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) 2005-03-04 00:32:31 PST
I asked because I wanted you to mark it "review?" from for example Dean or Neil :)
Comment 6 neil@parkwaycc.co.uk 2005-03-05 13:58:50 PST
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) 2005-09-08 08:46:47 PDT
Created attachment 195277 [details] [diff] [review]
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 torisugari@gmail.com tested his original patch. :-)
Comment 8 Vidar Haarr (not reading bugmail) 2005-10-06 04:30:25 PDT
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 neil@parkwaycc.co.uk 2005-10-07 06:15:27 PDT
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) 2005-10-08 03:42:57 PDT
Created attachment 198911 [details] [diff] [review]
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) 2005-10-08 04:53:45 PDT
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) 2005-10-09 00:42:56 PDT
Created attachment 198982 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2005-10-09 13:32:23 PDT
Fix checked in.
Comment 14 Christian :Biesinger (don't email me, ping me on IRC) 2005-10-09 16:47:12 PDT
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) 2005-10-09 19:26:22 PDT
I agree about comment 14, so feel free to reopen.

-> verified
Comment 16 jag (Peter Annema) 2005-10-10 10:21:01 PDT
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) 2005-10-10 15:47:06 PDT
Created attachment 199110 [details] [diff] [review]
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 (not working on stability any more) 2005-10-11 04:19:28 PDT
Comment on attachment 199110 [details] [diff] [review]
adds localization notes

Thos L10n note look good to me ;-)
Comment 19 Vidar Haarr (not reading bugmail) 2005-10-11 14:51:43 PDT
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 (not working on stability any more) 2005-10-12 16:48:51 PDT
L10n notes checked in as well, thanks!
Comment 21 neil@parkwaycc.co.uk 2005-12-09 09:50:55 PST
Comment on attachment 198982 [details] [diff] [review]
patch for checkin, I hope.

(including the L10N notes of course)
Comment 22 Robert Kaiser (not working on stability any more) 2005-12-09 15:48:40 PST
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 [:mwargers] (gone per 2016-05-31 :-( ) 2005-12-09 16:57:01 PST
Please advise if you want me to provide a patch for branch checkin.
Comment 24 Vidar Haarr (not reading bugmail) 2005-12-09 16:58:00 PST
Err.. Comment 23 was mine. Yay for public access computers.
Comment 25 neil@parkwaycc.co.uk 2005-12-11 13:02:45 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.