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.

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

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.

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.

In addition to fixing nits, I removed the localizations strings that comments claimed were not needed.

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.

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.

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

Carrying over r/sr

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?

Checked in: http://hg.mozilla.org/comm-central/rev/96cc9a14c26e

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