Closed
Bug 786648
Opened 9 years ago
Closed 8 years ago
Stop stealing strings for log viewer
Categories
(Thunderbird :: Instant Messaging, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 21.0
People
(Reporter: mconley, Assigned: adarshdinesh)
Details
Attachments
(1 file, 4 obsolete files)
3.06 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
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.
Severity: normal → trivial
Whiteboard: [trivial][mentor=mconley][lang=xul] → [good first bug][mentor=mconley][lang=xul]
Comment 2•8 years ago
|
||
(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
Reporter | ||
Comment 4•8 years ago
|
||
Comment on attachment 700404 [details] [diff] [review] First Patch Adding to my review queue.
Attachment #700404 -
Flags: review?(mconley)
Please check my second patch.
Attachment #700404 -
Attachment is obsolete: true
Attachment #700404 -
Flags: review?(mconley)
Reporter | ||
Comment 6•8 years ago
|
||
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)
Reporter | ||
Comment 7•8 years ago
|
||
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-
Please review my Third Patch.
Attachment #701448 -
Flags: review?(mconley)
Reporter | ||
Comment 9•8 years ago
|
||
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•8 years ago
|
||
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)
Reporter | ||
Comment 11•8 years ago
|
||
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-
Comment 12•8 years ago
|
||
I think it would make sense to rename the msgBundle variable to chatBundle.
Assignee | ||
Comment 13•8 years ago
|
||
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 14•8 years ago
|
||
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)
Reporter | ||
Comment 15•8 years ago
|
||
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 +
Reporter | ||
Comment 16•8 years ago
|
||
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+
Reporter | ||
Updated•8 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 21.0
Reporter | ||
Updated•8 years ago
|
Whiteboard: [good first bug][mentor=mconley][lang=xul]
You need to log in
before you can comment on or make changes to this bug.
Description
•