Allow localization-friendly reordering of search terms

RESOLVED FIXED in Thunderbird 3.0b3

Status

defect
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: rkent, Assigned: rkent)

Tracking

Trunk
Thunderbird 3.0b3
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

In bug 310359, a patch to add a search term was criticized for causing unnecessary burden on localizers, just to reorder the search terms. As a followup, provide a mechanism to reorder the terms that does not affect localization.
So the way that this works is search attributes are defined in nsMsgSearchCore.idl, filtered for validity in nsMsgSearchValidityTable::GetAvailableAttributes which prepares an output array, then mailWidgets.xml uses those values to fill a menu popup.

I think what I will do is just to define a mapping table in nsMsgSearchCore.idl that is used by GetAvailableAttributes when it outputs the items.
The plan sounds reasonable, what I would really like to see is that we drop the numbers inside the locale files and replace them by strings, i.e.

- 44=Junk Score Origin
+ junkScoreOrigin=Junk Score Origin

Either that or we provide a localisation note for every single numbered string in that file (I'd much prefer the string solution).
So what you are proposing is different than my proposal in comment 1. So let me offer this plan B which is a more concrete version of the request in comment 2.

(I feel like I am reinventing the wheel here with something quite complex. If I am missing an obvious or conventional approach, please show me).

The existing numbering definitions in nsMsgSearchCore.idl like:

  const nsMsgSearchAttribValue ToOrCC = 8;

will be used to set the ordering for display, so I will freely renumber those to set the display order as done originally in https://bugzilla.mozilla.org/attachment.cgi?id=359225  Then I provide a mapping between nsMsgSearchAttrib::ToOrCC (which equals 8) and the string "ToOrCC". search-attributes.properties will be changed to have entries like:

ToOrCC=To or CC

That mapping would be in a C++-only structure in nsMsgSearchCore.idl. mailWidgets.xml will get an XPCOM string instead of an integer from GetAvailableAttributes() The C++ for GetAvailableAttributes will use the mapping to return localization keys like "ToOrCC" in desired display order.

Are you *sure* this is what you want? It will also force localizers to redo all of the "8=To or CC" localizations.

The alternative, plan A from comment 1, is to provide an integer/integer mapping that would look like [ 1, 2, 4, 3, 5 ...] to show attribute 4 before attribute 3.
I really need a response to my "Are you *sure* this is what you want?" from comment 3 to proceed.
Whiteboard: [needs prepatch comments from Standard8, sipaq]
(In reply to comment #3)
> Are you *sure* this is what you want? It will also force localizers to redo 
> all of the "8=To or CC" localizations.

That is something that we will have to accept. Frankly, we can not only think about current localizers, but need to think about future localizers as well.
Whiteboard: [needs prepatch comments from Standard8, sipaq] → [needs prepatch comments from Standard8]
Sorry for not getting back to this earlier. I agree with Simon, forcing redo (although a pain) is probably the best thing. It will make it better for future changes and localisers.

The plan in comment 3 sounds like what we want to do here.
Whiteboard: [needs prepatch comments from Standard8]
Attachment #369975 - Attachment description: Map searcha → Map searchAttributes to localizable strings
Attachment #369975 - Attachment is patch: true
Attachment #369975 - Attachment mime type: application/octet-stream → text/plain
Attachment #369975 - Flags: superreview?(bienvenu)
Attachment #369975 - Flags: review?(bugzilla)
Whiteboard: [needs r standard8, sr bienvenu]
Comment on attachment 369975 [details] [diff] [review]
Map searchAttributes to localizable strings

Looking good, a few nits and one question.


>+            var pref = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch);

nit: as you're touching this line, please re-wrap.

>+            var j=0;
>+            for (var i=0; i<ids.length; i++)

nit: again, as you're touching it, please add a space either side of = and <.

>+# Location not visible
>+Location=Location

What are these used for? Makes me wonder why we have UI strings for them. I'd be tempted to say this should be a localisation note but then they will ask why they are translating strings that aren't used....

>+    /**
>+     * given a search attribute (which is an internal numerical id), return
>+     * a localization-friendly string for use in a .properties file

nit: Capital letter to start the sentence and full stop on the end please.

>+/**
>+ * definitions of search attribute types. The numerical order
>+ * from here will also be used to determine the order that the
>+ * attributes display in the filter editor
>+ */

ditto re sentences.

>diff --git a/suite/mailnews/mailWidgets.xml b/suite/mailnews/mailWidgets.xml

similar comments apply to this file.
(In reply to comment #8)
> >+# Location not visible
> >+Location=Location
> 
> What are these used for? Makes me wonder why we have UI strings for them. I'd
> be tempted to say this should be a localisation note but then they will ask why
> they are translating strings that aren't used....
> 

In the past, the numbers were both the localization strings, as well as internal identitifiers. I simply did a mindless transfer of that to the new format. But trying to engage my mind, perhaps we should try to see if some of these strings can be eliminated. It would be helpful if someone more familiar with the history, and/or usage in the AB search code, could comment on the specific strings that currently are marked "not visible". But in any case, let me delete those strings (and their mapping values) in the next patch.
Posted patch Remove uneeded localizations (obsolete) — Splinter Review
In addition to fixing nits, I removed the localizations strings that comments claimed were not needed.
Attachment #369975 - Attachment is obsolete: true
Attachment #371280 - Flags: superreview?(bienvenu)
Attachment #371280 - Flags: review?(bugzilla)
Attachment #369975 - Flags: superreview?(bienvenu)
Attachment #369975 - Flags: review?(bugzilla)
Attachment #371280 - Attachment is obsolete: true
Attachment #371281 - Flags: superreview?(bienvenu)
Attachment #371281 - Flags: review?(bugzilla)
Attachment #371280 - Flags: superreview?(bienvenu)
Attachment #371280 - Flags: review?(bugzilla)
Posted patch Still didn't get it right (obsolete) — Splinter Review
Definitely not my day. I think I'll quit for now and take advantage of a rare sunny Northwest spring day to do some desperately needed gardening.
Attachment #371281 - Attachment is obsolete: true
Attachment #371282 - Flags: superreview?(bienvenu)
Attachment #371282 - Flags: review?(bugzilla)
Attachment #371281 - Flags: superreview?(bienvenu)
Attachment #371281 - Flags: review?(bugzilla)
Comment on attachment 371282 [details] [diff] [review]
Still didn't get it right

I realize you're just moving code around, but there's no need for the temp var "pref" here:

+            var pref = Components.classes["@mozilla.org/preferences-service;1"]
+                                 .getService(Components.interfaces.nsIPrefBranch);
+            var hdrsArray = null;
+            try
+            {
+              var hdrs = pref.getCharPref("mailnews.customHeaders");


extra credit for using let instead of var for that code as well.


I must confess I didn't know you could do this - it looks odd to me.

+struct
+{
+  nsMsgSearchAttribValue id;
+  const char* property;
+}
+nsMsgSearchAttribMap[] = ...

Doesn't this return the string name that you can as a key to look up the localized string in the .properties file? And not just any properties file, but search-attributes.properties? I think I'd prefer this comment to be a bit more explicit


+     * Given a search attribute (which is an internal numerical id), return
+     * a localization-friendly string for use in a .properties file.
Comment on attachment 371282 [details] [diff] [review]
Still didn't get it right

r=me with David's comments addressed.
Attachment #371282 - Flags: review?(bugzilla) → review+
Whiteboard: [needs r standard8, sr bienvenu] → [needs sr bienvenu]
What do you find odd about:

 struct
 {
   nsMsgSearchAttribValue id
Hmmm, ThunderBrowse ate the rest of my comment. It was:

What do you find odd about

+struct
+{
+  nsMsgSearchAttribValue id;
+  const char* property;
+}
+nsMsgSearchAttribMap[] = ...

? Is it just that this declaration has no naming structure tag after the "struct"? This is just standard K&R C.
Comment on attachment 371282 [details] [diff] [review]
Still didn't get it right

I meant to plus that before, sorry.

the anonymous struct declaration along with the var in the same statement - I figured it was standard K&R; I guess I always used typedef...
Attachment #371282 - Flags: superreview?(bienvenu) → superreview+
Posted patch Fixed bienvenu's nits (obsolete) — Splinter Review
Carrying over r/sr
Attachment #371282 - Attachment is obsolete: true
Attachment #372969 - Flags: superreview+
Attachment #372969 - Flags: review+
Attachment #372969 - Attachment is obsolete: true
Attachment #372971 - Flags: superreview+
Attachment #372971 - Flags: review+
sipaq, the localizations changes that you requested are now complete. Can you please do any postings to the localization newsgroup that you think are appropriate to announce this, as you suggested in bug 483629 comment 11?
Keywords: checkin-needed
Whiteboard: [needs sr bienvenu] → [needs checkin]
Target Milestone: --- → Thunderbird 3.0b3
Checked in: http://hg.mozilla.org/comm-central/rev/96cc9a14c26e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs checkin]
(In reply to comment #20)
> sipaq, the localizations changes that you requested are now complete. Can you
> please do any postings to the localization newsgroup that you think are
> appropriate to announce this, as you suggested in bug 483629 comment 11?

Done. Thanks for your work on this, Kent!
You need to log in before you can comment on or make changes to this bug.