"Mozilla Wallpaper.bmp" should be localizable.

VERIFIED FIXED

Status

SeaMonkey
UI Design
--
minor
VERIFIED FIXED
13 years ago
9 years ago

People

(Reporter: O. Atsushi (Torisugari), Assigned: Vidar Haarr (not reading bugmail))

Tracking

({fixed-seamonkey1.0, fixed1.8, l12y})

Trunk
fixed-seamonkey1.0, fixed1.8, l12y

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

13 years ago
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
(Reporter)

Comment 1

13 years ago
Created attachment 169797 [details] [diff] [review]
patch v1 [suite, windows] (maybe already obsolete)

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

Comment 2

13 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

Updated

13 years ago
Keywords: l12y
(Reporter)

Updated

13 years ago
Depends on: 260437
(Assignee)

Comment 3

12 years ago
Torisugari: Does this patch need to be updated to trunk, or is it ready for review ?
(Reporter)

Comment 4

12 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

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

Comment 6

12 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

12 years ago
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. :-)
Attachment #169797 - Attachment is obsolete: true
Attachment #195277 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 8

12 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

12 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

12 years ago
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.
Assignee: guifeatures → vhaarr+bmo
Attachment #195277 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #198911 - Flags: superreview?(jag)
Attachment #198911 - Flags: review+

Comment 11

12 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

12 years ago
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.
(Assignee)

Updated

12 years ago
Attachment #198911 - Attachment is obsolete: true
Attachment #198982 - Flags: superreview+
Attachment #198982 - Flags: review+

Comment 13

12 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Should the ".bmp" part really be localizable?

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

Comment 15

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

-> verified
Status: RESOLVED → VERIFIED

Comment 16

12 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

12 years ago
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].
(Assignee)

Updated

12 years ago
Attachment #199110 - Flags: review?(neil.parkwaycc.co.uk)

Updated

12 years ago
Attachment #199110 - Flags: review?(neil.parkwaycc.co.uk) → review?(kairo)

Comment 18

12 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

12 years ago
Attachment #199110 - Flags: superreview?(jag)

Updated

12 years ago
Attachment #199110 - Flags: superreview?(jag) → superreview+
(Assignee)

Comment 19

12 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

12 years ago
L10n notes checked in as well, thanks!

Comment 21

12 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

12 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+
Please advise if you want me to provide a patch for branch checkin.
(Assignee)

Comment 24

12 years ago
Err.. Comment 23 was mine. Yay for public access computers.

Comment 25

12 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
Keywords: fixed-seamonkey1.0
Whiteboard: fixed-seamonkey1.0

Updated

9 years ago
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.