Last Comment Bug 786648 - Stop stealing strings for log viewer
: Stop stealing strings for log viewer
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: Trunk
: x86 All
: -- trivial (vote)
: Thunderbird 21.0
Assigned To: adu
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-29 06:53 PDT by Mike Conley (:mconley)
Modified: 2013-01-17 16:01 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
First Patch (2.83 KB, patch)
2013-01-10 07:07 PST, adu
no flags Details | Diff | Splinter Review
Second Patch (2.48 KB, patch)
2013-01-11 05:51 PST, adu
mconley: review-
Details | Diff | Splinter Review
Third Patch (2.32 KB, patch)
2013-01-12 03:34 PST, adu
mconley: review-
Details | Diff | Splinter Review
Patch File (3.29 KB, patch)
2013-01-15 03:49 PST, adu
mconley: review-
Details | Diff | Splinter Review
Patch file (3.06 KB, patch)
2013-01-17 04:28 PST, adu
mconley: review+
Details | Diff | Splinter Review

Description Mike Conley (:mconley) 2012-08-29 06:53:26 PDT
We currently steal the "Today", "Yesterday", "Last Week", "Two Weeks Ago" strings from other components of Thunderbird.

We should decouple these, and give the log viewer its own strings.
Comment 1 adu 2013-01-09 05:23:46 PST
Can i pick this Bug?
Comment 2 Patrick Cloke [:clokep] 2013-01-09 05:27:38 PST
(In reply to adu from comment #1)
> Can i pick this Bug?

Sure, I've assigned it to you. For reference, adu had contact Florian, Mike and I via email about this bug. I gave some background information and suggested he come on IRC to talk to us further.
Comment 3 adu 2013-01-10 07:07:37 PST
Created attachment 700404 [details] [diff] [review]
First Patch

I have attached the patch. Please check.
Comment 4 Mike Conley (:mconley) 2013-01-10 07:09:27 PST
Comment on attachment 700404 [details] [diff] [review]
First Patch

Adding to my review queue.
Comment 5 adu 2013-01-11 05:51:17 PST
Created attachment 701040 [details] [diff] [review]
Second Patch

Please check my second patch.
Comment 6 Mike Conley (:mconley) 2013-01-11 10:35:23 PST
Comment on attachment 701040 [details] [diff] [review]
Second Patch

Hey adu,

Thanks for the patch! Next time, make sure to set review? on me, so the patch gets added to my queue.

-Mike
Comment 7 Mike Conley (:mconley) 2013-01-11 11:23:19 PST
Comment on attachment 701040 [details] [diff] [review]
Second Patch

Review of attachment 701040 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks adu - but what about log.lastWeek and log.twoWeeksAgo ?

::: mail/locales/en-US/chrome/messenger/chat.properties
@@ +89,5 @@
>  # %1$S is the date, %2$S is the date.
>  dateTime=%1$S %2$S
> +
> +# LOCALIZATION NOTE (log.today, log.yesterday, log.lastWeek, log.twoWeeksAgo):
> +#This is used for logging. 

I don't think this comment adds much - I think you can remove it.
Comment 8 adu 2013-01-12 03:34:59 PST
Created attachment 701448 [details] [diff] [review]
Third Patch

Please review my Third Patch.
Comment 9 Mike Conley (:mconley) 2013-01-14 09:00:12 PST
Comment on attachment 701448 [details] [diff] [review]
Third Patch

Review of attachment 701448 [details] [diff] [review]:
-----------------------------------------------------------------

Hey adu,

You wrote in your email:

> I removed the comment and  the strings "log.lastWeek" and "log.twoWeeksAgo"
> because those strings are not used in the chat-messenger. Hope i am on the
> right path.

"lastWeek" and "twoWeeksAgo" are indeed used in chat-messenger-overlay.js.

This line: http://mxr.mozilla.org/comm-central/source/mail/components/im/content/chat-messenger-overlay.js#1157

retrieves the appropriate string based on the group names defined here:

http://mxr.mozilla.org/comm-central/source/mail/components/im/content/chat-messenger-overlay.js#1088

So those strings are now missing. You should put them back, and make sure to retrieve the strings with "log." prefixing them.

Let me know in this bug if you have any more questions.

Thanks,

-Mike
Comment 10 adu 2013-01-15 03:49:59 PST
Created attachment 702217 [details] [diff] [review]
Patch File

hope this will fix the bug .Please review my patch.
Comment 11 Mike Conley (:mconley) 2013-01-15 06:42:27 PST
Comment on attachment 702217 [details] [diff] [review]
Patch File

Review of attachment 702217 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there!

::: mail/components/im/content/chat-messenger-overlay.js
@@ +1085,5 @@
>  
>      // The keys used in the 'groups' object should match string ids in
>      // messenger.properties, except 'other' that has a special handling.
>      let groups = {
> +      log.lastWeek: [],

This is not valid Javascript - you cannot have properties with periods in their names.

Among other reasons, it's because it's ambiguous. Suppose I had the following:

let groups = {
  log.lastWeek: [],
  log: {
    lastWeek: "Hello!"
  }
};

What does groups.log.lastWeek refer to then?

TL;DR:  These lines should stay the way they are.

@@ +1122,4 @@
>          continue;
>        }
>        else if (timeFromToday <= kWeekInMsecs)
> +        group = groups.log.lastWeek;

Please revert the changes to line 1125 and 1127.

@@ +1131,5 @@
>      }
>  
>      if (today)
>        this._rowMap.push(today);
>      if (yesterday)

The change you need to make is below, right here:

http://mxr.mozilla.org/comm-central/source/mail/components/im/content/chat-messenger-overlay.js#1157

You need to change:

groupName = msgBundle.getString(groupId);

to

groupName = msgBundle.getString("log." + groupId);
Comment 12 Florian Quèze [:florian] [:flo] 2013-01-15 07:02:51 PST
I think it would make sense to rename the msgBundle variable to chatBundle.
Comment 13 adu 2013-01-17 04:28:31 PST
Created attachment 703270 [details] [diff] [review]
Patch file

I replaced the variable msgBundle with chatBundle, as florian asked for.
changed line +1157 
>groupName = msgBundle.getString(groupId);
to 
 groupName = chatBundle.getString("log."+groupId);
Comment 14 Patrick Cloke [:clokep] 2013-01-17 04:56:57 PST
Comment on attachment 703270 [details] [diff] [review]
Patch file

It would be better for mconley to review this.
Comment 15 Mike Conley (:mconley) 2013-01-17 12:11:43 PST
Comment on attachment 703270 [details] [diff] [review]
Patch file

Review of attachment 703270 [details] [diff] [review]:
-----------------------------------------------------------------

From inspection, this looks good - just one nit. I'll test the patch tonight.

::: mail/components/im/content/chat-messenger-overlay.js
@@ +1153,5 @@
>          }
>        }
>        else {
>          // Otherwise, get the appropriate string for this group.
> +        groupName = chatBundle.getString("log."+groupId);

Nit: spaces on either side of the +
Comment 16 Mike Conley (:mconley) 2013-01-17 16:00:30 PST
Comment on attachment 703270 [details] [diff] [review]
Patch file

I fixed the nit before landing this. Thanks adu!

Landed on comm-central as https://hg.mozilla.org/comm-central/rev/eceb4d4b7131

Note You need to log in before you can comment on or make changes to this bug.