Closed
Bug 955278
Opened 10 years ago
Closed 10 years ago
Message notifications do not take /me into account
Categories
(Instantbird Graveyard :: Other, defect)
Instantbird Graveyard
Other
Tracking
(Not tracked)
RESOLVED
FIXED
1.5
People
(Reporter: clokep, Assigned: nhnt11)
References
Details
Attachments
(4 files, 7 obsolete files)
*** Original post on bio 1845 at 2012-12-03 17:33:00 UTC *** *** Due to BzAPI limitations, the initial description is in comment 1 ***
Reporter | ||
Comment 1•10 years ago
|
||
*** Original post on bio 1845 as attmnt 2150 at 2012-12-03 17:33:00 UTC *** Message notifications look kind of funky when there's a /me message, as they display the /me at the beginning instead of formatting it as an action, etc.
Assignee | ||
Comment 2•10 years ago
|
||
*** Original post on bio 1845 as attmnt 2410 at 2013-04-24 22:29:00 UTC *** Notification now displays as follows: * <from> <message> This whole string is stored in messageText, and the notification handler sees it to be from "" - in true third person style.
Attachment #8354177 -
Flags: review?(clokep)
Assignee | ||
Comment 3•10 years ago
|
||
*** Original post on bio 1845 as attmnt 2411 at 2013-04-24 22:34:00 UTC *** Alternate fix. The format is as follows: <from> <message> The "/me " is simply removed.
Attachment #8354178 -
Flags: review?(clokep)
Assignee | ||
Comment 4•10 years ago
|
||
*** Original post on bio 1845 as attmnt 2412 at 2013-04-24 23:01:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Assignee | ||
Comment 5•10 years ago
|
||
*** Original post on bio 1845 as attmnt 2413 at 2013-04-24 23:03:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Comment 6•10 years ago
|
||
*** Original post on bio 1845 at 2013-04-25 12:27:09 UTC *** After some discussion on IRC, could we try <from> <from> <message> ie. keeping the title (so that the notifications don't look dissimilar from other messages) and simply substituting the /me with the actual name? (Sadly it seems there is no reliable way to use italics here.)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → nhnt11
Comment 7•10 years ago
|
||
Comment on attachment 8354177 [details] [diff] [review] Patch to fix *** Original change on bio 1845 attmnt 2410 at 2013-04-27 14:39:28 UTC *** (See previous comment)
Attachment #8354177 -
Flags: review?(clokep) → review-
Comment 8•10 years ago
|
||
Comment on attachment 8354178 [details] [diff] [review] Alternate fix *** Original change on bio 1845 attmnt 2411 at 2013-04-27 14:39:34 UTC *** (See previous comment)
Attachment #8354178 -
Flags: review?(clokep) → review-
Assignee | ||
Comment 9•10 years ago
|
||
*** Original post on bio 1845 as attmnt 2421 at 2013-04-28 00:52:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Comment 10•10 years ago
|
||
*** Original post on bio 1845 at 2013-04-28 07:28:07 UTC *** Comment on attachment 8354188 [details] [diff] [review] (bio-attmnt 2421) Alternate fix, as per suggestion >+ // Handle third person messages >+ let name = aMessage.alias || aMessage.who; >+ if (/^\/me/.test(messageText)) { You'll also have to require a word-break, white-space or what else might fit the bill behind the /me in this test. Example why this is needed: if I send a message like "/memoserv isn't working on irc.mozilla.org!" on a protocol that doesn't recognize the "/memoserv" as command, it would reach your code and result in "<my name>moserv isn't working on irc.mozilla.org!".
Assignee | ||
Comment 11•10 years ago
|
||
*** Original post on bio 1845 as attmnt 2433 at 2013-05-16 19:05:00 UTC *** Oops. I had taken whitespace into account, but then removed it since only the "/me" part should be replaced by the name (i.e. "/me hello" should not be replaced by "nhnt11hello"). Changed it in the test as well without thinking.
Assignee | ||
Comment 12•10 years ago
|
||
Comment on attachment 8354188 [details] [diff] [review] Alternate fix, as per suggestion *** Original change on bio 1845 attmnt 2421 at 2013-05-16 19:05:08 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354188 -
Attachment is obsolete: true
Reporter | ||
Comment 13•10 years ago
|
||
Comment on attachment 8354200 [details] [diff] [review] Alternate fix #2 for bug 1845, taking whitespace into account *** Original change on bio 1845 attmnt 2433 at 2013-05-16 19:10:53 UTC *** ># HG changeset patch ># Parent 71f11f2e226f1a4e90f0e7b5539806d114296f66 ># User Nihanth Subramanya <nhnt11@gmail.com> >Alternate fix #2 for bug 955278 (bio 1845) Could you format this as the bug title? (E.g. "Bug 955278 (bio 1845) - Message notifications do not take /me into account, r=<reviewer's name>.") Although if you don't know the reviewer, just leave that part off. >diff --git a/instantbird/modules/ibNotifications.jsm b/instantbird/modules/ibNotifications.jsm >+ // Handle third person messages >+ let name = aMessage.alias || aMessage.who; >+ if (/^\/me\s/.test(messageText)) { >+ messageText = messageText.replace(/^\/me/, name); >+ } Nit: Don't enclose single line statements inside of { }. > Components.classes["@mozilla.org/alerts-service;1"] > .getService(Components.interfaces.nsIAlertsService) > .showAlertNotification(icon, aMessage.alias || aMessage.who, If you're calculating name above, use it here.
Attachment #8354200 -
Flags: review-
Assignee | ||
Comment 14•10 years ago
|
||
*** Original post on bio 1845 as attmnt 2434 at 2013-05-16 19:24:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354201 -
Flags: review?(aleth)
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8354200 [details] [diff] [review] Alternate fix #2 for bug 1845, taking whitespace into account *** Original change on bio 1845 attmnt 2433 at 2013-05-16 19:24:01 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354200 -
Attachment is obsolete: true
Comment 16•10 years ago
|
||
Comment on attachment 8354201 [details] [diff] [review] Alternate fix #2, fixed nits. *** Original change on bio 1845 attmnt 2434 at 2013-05-17 15:18:16 UTC *** >+ if (/^\/me\s/.test(messageText)) >+ messageText = messageText.replace(/^\/me/, name); I don't think we want \s here, as we don't want to match all possible kinds of whitespace - just use a space. Also, consistency requires we don't match cases we don't match in http://lxr.instantbird.org/instantbird/source/chat/modules/imThemes.jsm#362.
Attachment #8354201 -
Flags: review?(aleth) → review-
Assignee | ||
Comment 17•10 years ago
|
||
*** Original post on bio 1845 as attmnt 2439 at 2013-05-21 20:57:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354206 -
Flags: review?(aleth)
Assignee | ||
Comment 18•10 years ago
|
||
Comment on attachment 8354201 [details] [diff] [review] Alternate fix #2, fixed nits. *** Original change on bio 1845 attmnt 2434 at 2013-05-21 20:57:58 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354201 -
Attachment is obsolete: true
Assignee | ||
Comment 19•10 years ago
|
||
*** Original post on bio 1845 as attmnt 2440 at 2013-05-21 21:07:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354207 -
Flags: review?(aleth)
Assignee | ||
Comment 20•10 years ago
|
||
Comment on attachment 8354206 [details] [diff] [review] Replaced \s with just a space for consistency. *** Original change on bio 1845 attmnt 2439 at 2013-05-21 21:07:50 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354206 -
Attachment is obsolete: true
Attachment #8354206 -
Flags: review?(aleth)
Assignee | ||
Comment 21•10 years ago
|
||
*** Original post on bio 1845 as attmnt 2441 at 2013-05-22 04:24:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354208 -
Flags: review?(aleth)
Assignee | ||
Comment 22•10 years ago
|
||
Comment on attachment 8354207 [details] [diff] [review] Use startsWith for better readability. *** Original change on bio 1845 attmnt 2440 at 2013-05-22 04:24:46 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354207 -
Attachment is obsolete: true
Attachment #8354207 -
Flags: review?(aleth)
Comment 23•10 years ago
|
||
Comment on attachment 8354208 [details] [diff] [review] Fix unclosed parentheses *** Original change on bio 1845 attmnt 2441 at 2013-05-22 10:51:43 UTC *** Thanks!
Attachment #8354208 -
Flags: review?(aleth) → review+
Updated•10 years ago
|
Whiteboard: [checkin-needed]
Comment 24•10 years ago
|
||
Comment on attachment 8354178 [details] [diff] [review] Alternate fix *** Original change on bio 1845 attmnt 2411 at 2013-05-22 10:52:28 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354178 -
Attachment is obsolete: true
Comment 25•10 years ago
|
||
Comment on attachment 8354177 [details] [diff] [review] Patch to fix *** Original change on bio 1845 attmnt 2410 at 2013-05-22 10:52:32 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354177 -
Attachment is obsolete: true
Reporter | ||
Comment 26•10 years ago
|
||
*** Original post on bio 1845 at 2013-05-22 23:07:26 UTC *** Fixed in http://hg.instantbird.org/instantbird/rev/d09ed02dacf6 Thanks!
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
You need to log in
before you can comment on or make changes to this bug.
Description
•