483629 - Add BCC to AllAddresses term in local search

Status: RESOLVED FIXED

(MailNews Core :: Filters, enhancement)

Description

[Kent James (:rkent)]

As part of a series of bugs to add an AllAddresses search term, add the BCC properties that were added to nsIMsgDBHdr in bug 481667 to the AllAddresses search term. This bug will change the string to the long-term value as well.

Still needed:

1) enable AllAddresses in online IMAP search
2) provide method to specify display order of search terms

Comment 1

[Kent James (:rkent)]

Attachment: The fix [Checkin: Comment 14]

A fairly simple patch, since previous patches did most of the preparation.

A lot of my review comments tend to be complaints that I have not upgraded copied code with newer conventions. So I decided to remove the space before the "(" in the AllAddresses search term code. I take it that is what the general plan is, even though it leaves the new code mostly inconsistent with the style of the older code.

The patch for bug 481667 (which is checkin-needed as I write this) must be applied for patch to work.

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 367640 [details] [diff] [review]
The fix
[Checkin: Comment 14]

r=me.

Do we have the bug for doing the l10n string reordering/mapping recorded? We should post to mozilla.dev.l10n when we push this and let them know the string change and the fact that we're following up on it.

Comment 3

[Kent James (:rkent)]

(In reply to comment #2)
> (From update of attachment 367640 [details] [diff] [review])
> r=me.
> 
> Do we have the bug for doing the l10n string reordering/mapping recorded? We
> should post to mozilla.dev.l10n when we push this and let them know the string
> change and the fact that we're following up on it.

This patch has the string change in it, "43=From, To, CC or BCC".  I don't know how the l10n people work, but I sort of assummed that they automatically saw new or changed strings - is that wrong and I need to post string changes somewhere?

The bug for reordering has not been added yet, but I thought the point of that was to remove the l10n impact of the reordering change. Otherwise, I would simply be changing the numbers "43=From ..."

Comment 4

[Mark Banner (:standard8)]

(In reply to comment #3)
> (In reply to comment #2)
> This patch has the string change in it, "43=From, To, CC or BCC".  I don't know
> how the l10n people work, but I sort of assummed that they automatically saw
> new or changed strings - is that wrong and I need to post string changes
> somewhere?

The l10n people see new strings, they aren't guaranteed to see changed strings.

> The bug for reordering has not been added yet, but I thought the point of that
> was to remove the l10n impact of the reordering change. Otherwise, I would
> simply be changing the numbers "43=From ..."

Its both to do resolve the l10n impact and also to provide a string name change so that l10n builds that have missed this the previous change can pick it up.

Comment 5

[Kent James (:rkent)]

(In reply to comment #4)
> 
> The l10n people see new strings, they aren't guaranteed to see changed strings.
> 

This string is new since beta2, so it is not really changed from the perspective of the l10n cycle.

Comment 6

[Mark Banner (:standard8)]

(In reply to comment #5)
> (In reply to comment #4)
> > 
> > The l10n people see new strings, they aren't guaranteed to see changed strings.
> > 
> 
> This string is new since beta2, so it is not really changed from the
> perspective of the l10n cycle.

Yes, but a lot of l10n teams do changes as they go in, not in one big batch. Hence why some of them won't notice it.

Comment 7

[Kent James (:rkent)]

Simon, I would appreciate any suggestions on how or if to notify localizers about this string change. There was discussion of this in bug 310359, so I tried to restrict any changes to between b2 and b3, plus the reordering will be done in a way that hopefully does not affect localization. I would be happy to post something to m.d.l10n - but a) I don't really understand the issues involved, and 2) I looked there and did not see any other posts that seemed to deal with warnings of string changes.

Comment 8

[Simon Paquet [:sipaq]]

Ok, the problem is this:

There is no guarantee that localizers will notice a string change *unless* you change the entity name as well. The reason is that most locales don't watch our pushes in hg (because it's impossible for them to just watch all changes in mail/locales due to hg's limitations). 

Because of that most localizers entirely depend on the compare-locales tool, which basically just looks for (new, deleted, changed) entity names in the en-US locale and compares them with the state of the target locale.

So my question is: Can we change the entity name from "43" to something else (preferably something not as cryptic as a magic number) or is that impossible?

If we can't, we will have no guarantee that all locales will pick this up, even if we post an announcement to md.l10n and to the Thunderbird l10n blog that I maintain.

As for bug 310359... It seems that I didn't watch that bug closely enough. Otherwise I would not have permitted the string change there to occur without further discussion :-(

I hope this clarifies things for you, Kent. Let me know how you plan to proceed.

Comment 9

[Simon Paquet [:sipaq]]

Clearing status whiteboard and keywords for now, unless this is worked out.

Comment 10

[Mark Banner (:standard8)]

(In reply to comment #8)
> So my question is: Can we change the entity name from "43" to something else
> (preferably something not as cryptic as a magic number) or is that impossible?

Yes, but only once we fix bug 484147.

> I hope this clarifies things for you, Kent. Let me know how you plan to
> proceed.

I'm not sure what we want to do, it would be nice to get this patch checked in so we can get some testing on it.

One possible plan:

Land this patch and post to l10n stating that we have changed this string, and we will are intending to fix bug 484147 before the next string freeze.

Hence fixing bug 484147 would redo the string ids, and provide the necessary change notification for l10n.


Otherwise we have to hold-off landing this until we fix that bug.

Comment 11

[Simon Paquet [:sipaq]]

(In reply to comment #10)
> One possible plan:
> Land this patch and post to l10n stating that we have changed this string, 
> and we will are intending to fix bug 484147 before the next string freeze.
> Hence fixing bug 484147 would redo the string ids, and provide the necessary
> change notification for l10n.
> Otherwise we have to hold-off landing this until we fix that bug.

I don't want to hold this bug as hostage to get bug 484147 fixed. 

So I'm fine with landing the fix here now, *if* I get your (Mark) and Kent's assurance that both of you will push hard to get bug 484147 fixed *before* the next string freeze, which will likely be around April 20 (plus or minus a few days).

That being said, your plan sounds fine and we should follow it and notify localizers via m.d.l10n and the TB l10n blog.

Comment 12

[Kent James (:rkent)]

(In reply to comment #11)
> 
> I don't want to hold this bug as hostage to get bug 484147 fixed. 
> 

Then you need to redo the checkin-needed that you removed.

> So I'm fine with landing the fix here now, *if* I get your (Mark) and Kent's
> assurance that both of you will push hard to get bug 484147 fixed *before* the
> next string freeze, which will likely be around April 20 (plus or minus a few
> days).

There is no difficult coding in that bug, only difficult coding philosophies - which tend to be review limited. I can give typically 1 day response to comments and reivews for the next few weeks, though after April 12 I am less predictable (trip to England and Azerbaijan)

> 
> That being said, your plan sounds fine and we should follow it and notify
> localizers via m.d.l10n and the TB l10n blog.

Before or after bug 484147 lands, which could significantly alter the localization?

Comment 13

[Simon Paquet [:sipaq]]

(In reply to comment #12)
>> I don't want to hold this bug as hostage to get bug 484147 fixed. 
> 
> Then you need to redo the checkin-needed that you removed.

Done.
 
>> So I'm fine with landing the fix here now, *if* I get your (Mark) and 
>> Kent's assurance that both of you will push hard to get bug 484147 fixed 
>> *before* the next string freeze, which will likely be around April 20 
>> (plus or minus a few days).
> 
> There is no difficult coding in that bug, only difficult coding 
> philosophies - which tend to be review limited. I can give typically 1 day 
> response to comments and reviews for the next few weeks, though after April 
> 12 I am less predictable (trip to England and Azerbaijan)

Okay.

>> That being said, your plan sounds fine and we should follow it and notify
>> localizers via m.d.l10n and the TB l10n blog.
> 
> Before or after bug 484147 lands, which could significantly alter the
> localization?

We should notify localizers as soon as the patch here lands. Let me know if you need help with drafting a post for localizers.

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 367640 [details] [diff] [review]
The fix
[Checkin: Comment 14]


http://hg.mozilla.org/comm-central/rev/06a2b3323105