Last Comment Bug 786648 - Stop stealing strings for log viewer
: Stop stealing strings for log viewer
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: Trunk
: x86 All
-- trivial (vote)
: Thunderbird 21.0
Assigned To: adu
Depends on:
  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:
QA Whiteboard:
Iteration: ---
Points: ---

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 User image 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 User image adu 2013-01-09 05:23:46 PST
Can i pick this Bug?
Comment 2 User image 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 User image 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 User image 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 User image adu 2013-01-11 05:51:17 PST
Created attachment 701040 [details] [diff] [review]
Second Patch

Please check my second patch.
Comment 6 User image 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.

Comment 7 User image 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/
@@ +89,5 @@
>  # %1$S is the date, %2$S is the date.
>  dateTime=%1$S %2$S
> +
> +# LOCALIZATION NOTE (, 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 User image adu 2013-01-12 03:34:59 PST
Created attachment 701448 [details] [diff] [review]
Third Patch

Please review my Third Patch.
Comment 9 User image 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:

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

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.


Comment 10 User image 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 User image 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
>      //, 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:

You need to change:

groupName = msgBundle.getString(groupId);


groupName = msgBundle.getString("log." + groupId);
Comment 12 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2013-01-15 07:02:51 PST
I think it would make sense to rename the msgBundle variable to chatBundle.
Comment 13 User image 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);
 groupName = chatBundle.getString("log."+groupId);
Comment 14 User image 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 User image 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 User image 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

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