Message notifications do not take /me into account

RESOLVED FIXED in 1.5

Status

Instantbird
Other
--
minor
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: clokep, Assigned: nhnt11)

Tracking

Details

Attachments

(4 attachments, 7 obsolete attachments)

(Reporter)

Description

4 years ago
*** 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

4 years ago
Created attachment 8353912 [details]
Screenshot of the issue

*** 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

4 years ago
Created attachment 8354177 [details] [diff] [review]
Patch to fix

*** 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

4 years ago
Created attachment 8354178 [details] [diff] [review]
Alternate fix

*** 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

4 years ago
Created attachment 8354179 [details]
Screenshot of first fix

*** 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

4 years ago
Created attachment 8354180 [details]
Screenshot of alternate fix

*** 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

4 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

4 years ago
Assignee: nobody → nhnt11

Comment 7

4 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

4 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

4 years ago
Created attachment 8354188 [details] [diff] [review]
Alternate fix, as per suggestion

*** 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 ***
*** 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

4 years ago
Created attachment 8354200 [details] [diff] [review]
Alternate fix #2 for bug 1845, taking whitespace into account

*** 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

4 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

4 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

4 years ago
Created attachment 8354201 [details] [diff] [review]
Alternate fix #2, fixed nits.

*** 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

4 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

4 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

4 years ago
Created attachment 8354206 [details] [diff] [review]
Replaced \s with just a space for consistency.

*** 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

4 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

4 years ago
Created attachment 8354207 [details] [diff] [review]
Use startsWith for better readability.

*** 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

4 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

4 years ago
Created attachment 8354208 [details] [diff] [review]
Fix unclosed parentheses

*** 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

4 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

4 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

4 years ago
Whiteboard: [checkin-needed]

Comment 24

4 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

4 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

4 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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
(Reporter)

Updated

4 years ago
Duplicate of this bug: 954046
You need to log in before you can comment on or make changes to this bug.