The default bug view has changed. See this FAQ.

Stop stealing strings for log viewer

RESOLVED FIXED in Thunderbird 21.0

Status

Thunderbird
Instant Messaging
--
trivial
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mconley, Assigned: adu)

Tracking

Trunk
Thunderbird 21.0
x86
All

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.

Updated

5 years ago
Severity: normal → trivial
Whiteboard: [trivial][mentor=mconley][lang=xul] → [good first bug][mentor=mconley][lang=xul]
(Assignee)

Comment 1

4 years ago
Can i pick this Bug?
(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.
Assignee: nobody → adarshdinesh
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
Created attachment 700404 [details] [diff] [review]
First Patch

I have attached the patch. Please check.
Comment on attachment 700404 [details] [diff] [review]
First Patch

Adding to my review queue.
Attachment #700404 - Flags: review?(mconley)
(Assignee)

Comment 5

4 years ago
Created attachment 701040 [details] [diff] [review]
Second Patch

Please check my second patch.
Attachment #700404 - Attachment is obsolete: true
Attachment #700404 - Flags: review?(mconley)
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
Attachment #701040 - Flags: review?(mconley)
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.
Attachment #701040 - Flags: review?(mconley) → review-
(Assignee)

Comment 8

4 years ago
Created attachment 701448 [details] [diff] [review]
Third Patch

Please review my Third Patch.
Attachment #701448 - Flags: review?(mconley)
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
Attachment #701448 - Flags: review?(mconley) → review-
(Assignee)

Comment 10

4 years ago
Created attachment 702217 [details] [diff] [review]
Patch File

hope this will fix the bug .Please review my patch.
Attachment #701040 - Attachment is obsolete: true
Attachment #701448 - Attachment is obsolete: true
Attachment #702217 - Flags: review?(mconley)
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);
Attachment #702217 - Flags: review?(mconley) → review-
I think it would make sense to rename the msgBundle variable to chatBundle.
(Assignee)

Comment 13

4 years ago
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);
Attachment #702217 - Attachment is obsolete: true
Attachment #703270 - Flags: review?(clokep)
Comment on attachment 703270 [details] [diff] [review]
Patch file

It would be better for mconley to review this.
Attachment #703270 - Flags: review?(clokep) → review?(mconley)
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 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
Attachment #703270 - Flags: review?(mconley) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 21.0
Whiteboard: [good first bug][mentor=mconley][lang=xul]
You need to log in before you can comment on or make changes to this bug.