207550 - hardcoded ASCII strings in Junk logging / Filter logging code

Status: RESOLVED FIXED

(MailNews Core :: Internationalization, defect)

Description

[Ying-Lin Xia]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; ja-JP; rv:1.4b) Gecko/20030506 Netscape/7.02+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; ja-JP; rv:1.4b) Gecko/20030506 Netscape/7.02+

Please move this hardcoded string into en-US.jar, so l10n can translate it:

http://lxr.mozilla.org/seamonkey/source/mailnews/base/src/nsSpamSettings.cpp#479

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[marina]

nominating

Comment 2

[marina]

actually i wanted to nominate anothet bug for Junk folder

Comment 3

[Ying-Lin Xia]

still, we need this to be fixed as well,
nominating

Comment 4

[Ying-Lin Xia]

Attachment: screenshot JA

this is copied from bugscape bug http://bugscape/show_bug.cgi?id=28361

Comment 5

[marina]

reassigning to Seth

Comment 6

[bobj]

cc'ed rcloseirl

Comment 7

[Samir Gehani]

adt: nsbeta1+/adt2

Seth, can you work with the 18n/l10n folks to get this change in?  Thanks.

Comment 8

[(not reading, please use [e-mail] instead)]

accepting, I'll fix this.

I think I might have an old dup of this.

something to note, we also have some old, hardcoded filter logging code.

the fix for the logging issues will be to nsMsgFilter::LogRuleHit() and
nsSpamSettings::LogJunkHit()

bobj/robinf this will require moving some strings from C++ to .properties.  does
that have a= from i18n at this stage?

Comment 9

[robinf]

There's no help impact for this change.

Comment 10

[Ying-Lin Xia]

when you fix, can you make the strings somthing like:
   # %1=author, %2=subject, %3=date
   messageID="Detected junk message from  %1 - %2 at %3\n";
   #%1=message, %2=folder
   messageID= "Move message id = %1 to %2\n";
this way, other languages can change the replacement symbol order in their needs.

thanks

Comment 11

[bobj]

> bobj/robinf this will require moving some strings from C++ to .properties.
> does that have a= from i18n at this stage?

Yes, a=bobj for UI change.  Thanks!

Comment 12

[(not reading, please use [e-mail] instead)]

Attachment: still needs to be tested, reviewed, etc...

Comment 13

[(not reading, please use [e-mail] instead)]

Attachment: updated patch, log file is utf-8, improve subject / author strings (go ucs2 to utf8)

Comment 14

[(not reading, please use [e-mail] instead)]

in case anyone is watching that's not my final patch.  (it's got some testing
foo in junkLogDetectStr, and I still need to test.)

Comment 15

[Constantine A. Murenin]

What happened with this path? Why is it not in the tree? 
It seems that you still forgot to fix the time notation format to the one used
by OS. See bug #201248.

Comment 16

[Andrew Schultz]

only drivers can set (+) blocking flags.  you can request (?) them.

Comment 17

[chris hofmann]

is this patch ready to request reviews on?  lets get reviews and try for landing
in 1.7b

Comment 18

[Constantine A. Murenin]

*** Bug 174843 has been marked as a duplicate of this bug. ***

Comment 19

[chris hofmann]

still needs some testing.  can anyone help to move the testing and patch along?
 think seth is swamped with other stuff...  thanks     if something shows before
1.7 final renominate

Comment 20

[chris hofmann]

out of time for the pre-view release.  re-nominate if the testing happens

Comment 21

[Pavel Franc - Mozilla.cz]

Requesting blocking TB 2.

Comment 22

[Axel Hecht [:Pike]]

This patch likely doesn't apply anymore. From a l12y point of view, it looks like
the right thing to do, though. We'd like this for a2, due to the l10n impact.

Comment 23

[Simon Montagu :smontagu]

Attachment: Seth's patch merged to current trunk

Comment 24

[Simon Montagu :smontagu]

The merged patch seems to cause problems with filter logging: the actions dropdowns don't appear in the Filter Rules dialog, and the log doesn't get written. Junk mail logging seems fine.

Comment 25

[Axel Hecht [:Pike]]

nit, please chane the LOCALIZATION NOTES to LOCALIZATION NOTE?

Comment 26

[Simon Montagu :smontagu]

(In reply to comment #24)
> The merged patch seems to cause problems with filter logging: the actions
> dropdowns don't appear in the Filter Rules dialog

This wasn't caused by the patch: it's bug 341010

Comment 27

[Axel Hecht [:Pike]]

Seth, could you try to remember how the filterActionX stuff ended up to be used?
I.e., do we need to support grammar for those, too?
The patch just lost me a few times when trying to find that out.

Comment 28

[Simon Montagu :smontagu]

Attachment: slightly enhanced version

This is a more intelligent merge allowing for new filter actions that have been added in the meantime. It also fixes the bug with the log not being written, and adds localization of the dates.

I'll hold on review requests until I can test it after bug 341010 is fixed.

Comment 29

[Simon Montagu :smontagu]

Comment on attachment 225322 [details] [diff] [review]
slightly enhanced version

It turns out that a patch was checked in some time ago for bug 341010, but it wasn't marked as fixed; I am no longer seeing the problem that I was seeing before.

Comment 30

[David :Bienvenu]

Comment on attachment 225322 [details] [diff] [review]
slightly enhanced version

Thx very much for doing this!

r=bienvenu, with some nits:

+      mDateFormatter = do_CreateInstance(NS_DATETIMEFORMAT_CONTRACTID);
+      if (!mDateFormatter) {
+        return NS_ERROR_FAILURE;


won't do_CreateInstance return an rv as an out value when it fails? I'd prefer to return that rv instead of NS_ERROR_FAILURE.

the other thing is that I'd prefer the braces style to be this:

if (a)
{
  ...
}

which should be pretty much the prevailing style in this file.

Comment 31

[Simon Montagu :smontagu]

*** Bug 191183 has been marked as a duplicate of this bug. ***

Comment 32

[Scott MacGregor]

Comment on attachment 225322 [details] [diff] [review]
slightly enhanced version

should these strings be in filter.properties instead of messenger.properties?

We could use a space after the comma here:
+    NS_ENSURE_SUCCESS(rv,rv);

rest of it looks good modulo the suggestions David already made.

Comment 33

[David :Bienvenu]

I think Scott's right about using filter.properties. With those changes, I think we're good. If you can attach a new patch, I can slap an r/sr on it. Thx!

Comment 34

[Simon Montagu :smontagu]

Attachment: Patch addressing comments

Comment 35

[David :Bienvenu]

Comment on attachment 233446 [details] [diff] [review]
Patch addressing comments

looks good, thx a lot, Simon. One nit, which you can address or not, before you checkin, up to you:

+      if (actionType == nsMsgFilterAction::MoveToFolder)
+        rv = bundle->FormatStringFromName(
+          NS_LITERAL_STRING("logMoveStr").get(),
+          logMoveFormatStrings, 2,
+          getter_Copies(logMoveStr));
+      else // CopyToFolder
+        rv = bundle->FormatStringFromName(
+          NS_LITERAL_STRING("logCopyStr").get(),
+          logMoveFormatStrings, 2,
+          getter_Copies(logMoveStr));

this can be something like:

+        rv = bundle->FormatStringFromName(
+          actionType == nsMsgFilterAction::MoveToFolder ? NS_LITERAL_STRING("logMoveStr").get() : NS_LITERAL_STRING("logCopyStr").get(),
+          logMoveFormatStrings, 2,
+          getter_Copies(logMoveStr));

Comment 36

[Simon Montagu :smontagu]

Checked in with the change from comment 35

Comment 37

[Alex Ihrig [:Thunderbird_Mail_DE]]

Because of thunderbirds latest improvements:

filterAction7 should be "starred"

filterAction8 should be "tagged"

Comment 38

[Simon Montagu :smontagu]

Am I right that the change from "labeled" to "tagged" should be made for both Seamonkey mail and Thunderbird, but the change from "flagged" to "starred" should only be made for Thunderbird?

Comment 39

[Simon Montagu :smontagu]

Attachment: Update the strings

Comment 40

[David :Bienvenu]

Re TB vs SM for starred, I think that's up to the SM owners like Neil or Karsten

Comment 41

[Simon Montagu :smontagu]

Checked in the correction.

> Re TB vs SM for starred, I think that's up to the SM owners like Neil or
> Karsten

The filter descriptions are now consistent with the menus in both versions so if they change they can all change together.

Comment 42

[Scott MacGregor]

not a stop ship bug but if Simon is interested in moving this to the branch we'd probably approve it.

Comment 43

[Simon Montagu :smontagu]

Does l10n freeze not apply to thunderbird-2?

Comment 44

[Scott MacGregor]

No, we haven't had our l10n freeze yet. We haven't even had our first beta yet :)

Comment 45

[Simon Montagu :smontagu]

Attachment: Patch for 1.8 branch