Closed Bug 172004 Opened 22 years ago Closed 22 years ago

Provide override for individual strings in string bundles

Categories

(Core :: Internationalization, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.2beta

People

(Reporter: alecf, Assigned: alecf)

References

Details

(Keywords: intl, topembed+, Whiteboard: fix in hand, waiting on approval)

Attachments

(2 files, 4 obsolete files)

Embeddors need a way to override specific string values in various property
files across the product. the string bundle module should provide a mechanism to
override the existing values and use the overrides instead.

In addition, the mechanism probably needs to make sure to support enumeration of
string properties that don't already exist in the string bundle.

This allows the embeddor to distribute the GRE with minimal customizations to
the actual product, and instead just distribute this override file.
code issue? QA to yokoyama@netscape.com for now.
Keywords: intl
QA Contact: ruixu → yokoyama
Adding topembed+
Keywords: topembed+
Blocks: 160165
OS: Windows 2000 → All
Hardware: PC → All
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.2beta
ok, I've made some really good progress here. Basically what I did was provide a
mechanism for anyone to override a given {stringbundle, key} combination, and
then provided an implementation which overrides it from custom-strings.txt

I'm about 90% done with the implementation, and the only question up in the air
is if each string bundle will maintain a pointer to this string-override object,
or if they will all defer to the string bundle service first. I've also
implemented a mechanism to enumerate override keys for a given bundle, so it
should not be hard to allow the override object to add new keys to an existing
bundle.

I'll have a patch ready hopefully by late afternoon.
ok, here's my first cut at this feature. 

This will look for custom-strings.txt in your chrome directory, and use that to
override any strings.

custom-strings.txt should be in the form:
chromeurl:key=value

such as:

chrome://navigator/locale/navigator.properties:font.language.group=en-GB
chrome://communicator/locale/openLocation.properties:existingNavigatorWindow=Existing
browser window

The two things that this patch that are still an open issue are:
1) right now there is only one override implementation, and we probably want to
be more generic for bug 
2) the enumeration system isn't quite working - I've implemented the
per-chrome-url string enumeration in the override object, but haven't yet
hooked it up to nsStringBundle::GetSimpleEnumeration() - all this basically
means is that you can't yet enumerate the union of all keys in both the
override and the string bundle in question.

I'll try to hook up the enumeration stuff today/tomorrow.
oops, lets try that again. here's a patch that builds
Attachment #102062 - Attachment is obsolete: true
above, 1), should have read "for bug 160165"
Ok, here's my first candidate. I'm going to do some testing this afternoon and
hopefully get it finished & checked in today or tomorrow. Wish me luck.
Attachment #102064 - Attachment is obsolete: true
Attached patch updated patch (obsolete) — Splinter Review
here's an updated patch, where I've worked out a few issues. The biggest issue
is that .properties files actually use ':' as a delimiter (and since we're
trying to adhere to the Java standard, I won't change that)

so instead, I'm escaping the : in the URL with %3A, and using "#" as a
delimiter to indicate the string key. This means that the entries in
custom-strings.txt need to look like:

chrome%3A//navigator/locale/navigator.properties#nv_done=Done loading the page.


I've tested to make sure this works, and it does! feel free to try the patch.
it will also spit out an WARNING on startup (debug builds) if it finds
custom-strings.txt
Attachment #102190 - Attachment is obsolete: true
There are a few other things going on in this patch. For the sake over reviewers:
- moved nsStringBundleService definition out to nsStringBundleService.h, to make
the code easier to understand
- switched the extensible bundle stuff from nsISupportsArray to
nsCOMArray<nsIStringBundle>
- removed ASYNC_LOADING code which had bit-rotted and made the existing code
hard to read
- fixed some spacing issues where someone was using (gasp!) 1-space indenting
I applied Alec's patch to my latest trunk build and did a quick test to make
sure that it works in a GRE based env - and it does!

Here's what i did:

1. Created a custom-strings.txt file (with the contents below) in my embedding
client's "chrome" dir(change status msgs. to uppercase):

chrome%3A//necko/locale/necko.properties#ResolvingHost=RESOLVING HOST %1$S...
chrome%3A//necko/locale/necko.properties#ConnectedTo=CONNECTED TO %1$S...
chrome%3A//necko/locale/necko.properties#SendingTo=SENDING REQUEST TO %1$S...
chrome%3A//necko/locale/necko.properties#ReceivingFrom=TRANSFERRING DATA FROM
%1$S...
chrome%3A//necko/locale/necko.properties#ConnectingTo=CONNECTING TO %1$S...

2. Ran MfcEmbed and noticed that the status msgs. now appear in upper case in
MfcEmbed's status bar.

Hi Ashish : Can you or someone else in the QA team verify this...just to make
sure we have another set of eyes take a look...thanks
Comment on attachment 102209 [details] [diff] [review]
updated patch

Lots of work here...A few comments:
1. since you are taking out Async loading code, why not
taking out CreateAsyncBundle() 
2. How many times would the bundleoverride created if the stringBundleService
is QI-ed multiple times? Will each QI cause a re-check of the ".txt"?
3. What's the performance overhead of this feature?
What's the impact to start up time?

THX
Ok, this fixes a bug in the enumeration code. It should work now.
Attachment #102209 - Attachment is obsolete: true
Whiteboard: fix in hand, waiting on reviews
adding my reviewers so I can answer their questions.

tao:
1. I will do that - and in fact I've now done it in my tree. new patch coming up.
2. QI has nothing to do with the creation of the override object - it is
connected to the string bundle service creation, which only happens once. Thus,
the override object is only created once.
3. Performance impact is negligable in the case where the override does not
exist. during the application's lifetime, the impact is a simple null check
every time a string is created. Startup impact is minimal, and is just a check
for custom-strings.txt, which is a single stat() call to one file.
here's the updated version that reflects tao's comments.
Comment on attachment 102359 [details] [diff] [review]
provide override mechanism, addresses tao's comments

r=tao. thx!
Attachment #102359 - Flags: review+
Comment on attachment 102359 [details] [diff] [review]
provide override mechanism, addresses tao's comments

sr=sfraser
Attachment #102359 - Flags: superreview+
approval for 1.2 checkin has been requested.
Whiteboard: fix in hand, waiting on reviews → fix in hand, waiting on approval
Comment on attachment 102359 [details] [diff] [review]
provide override mechanism, addresses tao's comments

a=asa for checkin to 1.2beta (on behalf of drivers)
Attachment #102359 - Flags: approval+
Fix has been checked in.

Thanks, Alec
yep! Tomorrows build should have this.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
I updated and built my trunk but am unable to verify this fix i.e. the strings
are not being overridden.

1. I created a file named "custom-strings.txt" with the following contents in
the dist\bin\chrome dir:

chrome%3A//necko/locale/necko.properties#3=RESOLVING HOST %1$S...
chrome%3A//necko/locale/necko.properties#4=CONNECTED TO %1$S...
chrome%3A//necko/locale/necko.properties#5=SENDING REQUEST TO %1$S...
chrome%3A//necko/locale/necko.properties#6=TRANSFERRING DATA FROM %1$S...
chrome%3A//necko/locale/necko.properties#7=CONNECTING TO %1$S...

2. When i run MfcEmbed or Mozilla from dist\bin i do not see the status msgs. in
all upper case (it showed status msgs. in upper case when i applied the patch a
few days ago and tested)

However, it seems to be finding the custom-strings.txt file as indicated by the
following console msg:

WARNING: Using custom-strings.txt to override string bundles., file d:/mozilla_s
rc/mozilla/intl/strres/src/nsStringBundleTextOverride.cpp, line 107

Any ideas?
An update...

I built the MfcEmbed base embedding distribution and a GRE distribution and
tested the string override mechanism - it works fine in both the GRE based and
in the regular embedding scenarios.

I'm not sure why Mozilla did not pick up the string overrides. Alec and i
suspect it may be due to the fact that Mozilla FE might be using different
string ids than what's in the sample custom-strings.txt test file.

Anyway, it seems to be working fine now.
Upon further investigation....there's a rational explanation for all of this
madness.

Please note that when i tested (in Comment #21 above) i used the following
custom-strings.txt:

chrome%3A//necko/locale/necko.properties#3=RESOLVING HOST %1$S...
chrome%3A//necko/locale/necko.properties#4=CONNECTED TO %1$S...
chrome%3A//necko/locale/necko.properties#5=SENDING REQUEST TO %1$S...
chrome%3A//necko/locale/necko.properties#6=TRANSFERRING DATA FROM %1$S...
chrome%3A//necko/locale/necko.properties#7=CONNECTING TO %1$S...

Notice the usage of string id's 3,4,5 etc. instead of ResolvingHost,ConnectedTo
etc. That's the reason it did not work from dist\bin.

When i tested in the embedding scenarios, i had created custom-strings.txt with
the following contents - which worked:

chrome%3A//necko/locale/necko.properties#ResolvingHost=RESOLVING HOST %1$S...
chrome%3A//necko/locale/necko.properties#ConnectedTo=CONNECTED TO %1$S...
chrome%3A//necko/locale/necko.properties#SendingTo=SENDING REQUEST TO %1$S...
chrome%3A//necko/locale/necko.properties#ReceivingFrom=TRANSFERRING DATA FROM
%1$S...
chrome%3A//necko/locale/necko.properties#ConnectingTo=CONNECTING TO %1$S...

So, basically we do not have any issues here - everything works just fine!

Except, i'm not sure why we have string id's 3,4,5 etc. and ResolvingHost,
ConnectedTo etc. referring to the same strings in
http://lxr.mozilla.org/seamonkey/source/netwerk/resources/locale/en-US/necko.properties
- guess that's a different issue.
os2 doesn't seem to like this: ..\..\..\dist\include\xpcom\nsCOMArray.h(212:15)
: error EDC3090: Syntax error - expected "<" and found "&".

Actually, its probably right - you need nsCOMArray<T> for both the param and the
return type, don't you? I don't know why other compilers accept this, but it may
have something to do with the instantiation thing os2 breaks on - see bug 107291
I checked that fix in to hopefully fix the os2 bustage, r=sicking
You need to log in before you can comment on or make changes to this bug.