955278 - Message notifications do not take /me into account

Status: RESOLVED FIXED

(Instantbird Graveyard :: Other, defect)

Description

[Patrick Cloke [:clokep]]

*** Original post on bio 1845 at 2012-12-03 17:33:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***

Comment 1

[Patrick Cloke [:clokep]]

Attachment: 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.

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: 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.

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: 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.

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

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

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: 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

[aleth [:aleth]]

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

Comment 7

[aleth [:aleth]]

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)

Comment 8

[aleth [:aleth]]

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)

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

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

Comment 10

[Benedikt Pfeifer]

*** 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!".

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: 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.

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

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

Comment 13

[Patrick Cloke [:clokep]]

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 <[e-mail]>
>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.

Comment 14

[(inactive) Nihanth Subramanya [:nhnt11]]

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

Comment 15

[(inactive) Nihanth Subramanya [:nhnt11]]

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

Comment 16

[aleth [:aleth]]

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.

Comment 17

[(inactive) Nihanth Subramanya [:nhnt11]]

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

Comment 18

[(inactive) Nihanth Subramanya [:nhnt11]]

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

Comment 19

[(inactive) Nihanth Subramanya [:nhnt11]]

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

Comment 20

[(inactive) Nihanth Subramanya [:nhnt11]]

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

Comment 21

[(inactive) Nihanth Subramanya [:nhnt11]]

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

Comment 22

[(inactive) Nihanth Subramanya [:nhnt11]]

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

Comment 23

[aleth [:aleth]]

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!

Comment 24

[aleth [:aleth]]

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

Comment 25

[aleth [:aleth]]

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

Comment 26

[Patrick Cloke [:clokep]]

*** Original post on bio 1845 at 2013-05-22 23:07:26 UTC ***

Fixed in http://hg.instantbird.org/instantbird/rev/d09ed02dacf6

Thanks!