Closed Bug 955164 Opened 10 years ago Closed 10 years ago

Smiley regular expression problem in custom smiley themes

Categories

(Instantbird Graveyard :: Conversation, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bugzilla, Assigned: aleth)

Details

Attachments

(2 files, 1 obsolete file)

*** Original post on bio 1734 by Steffen <steffen2705 AT web.de> at 2012-10-19 19:00:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
*** Original post on bio 1734 as attmnt 1995 by steffen2705 AT web.de at 2012-10-19 19:00:00 UTC ***

The problem is with smileys that got a "." in them, like o.O

for the actual string of the smiley the replacement works fine
"o.O" -> smiley picture

for anything like "oXO" where X can be anything but ".", the replacement fails somehow and just a image-not-found-dummy is shown

it has something to do with the regular expression to find smileys in a conversation
*** Original post on bio 1734 at 2012-10-19 19:06:12 UTC ***

The faulty regex is http://lxr.instantbird.org/instantbird/source/chat/modules/imSmileys.jsm#120

A candidate replacement is http://lxr.instantbird.org/instantbird/source/chat/modules/jsProtoHelper.jsm#511, but there may be differences due to the different use case.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1734 as attmnt 1996 at 2012-10-19 20:18:00 UTC ***

So this uses the same regex for both cases. 

imSmileys: Add .{} to the regex and use newer $&

jsProtoHelper: Remove ,- from regex as they only have special meaning inside brackets, and we escape those. Similarly # is only special for regex comments, but we already escape ?. I also removed \s (which was probably there as the nick is between \b's, but is actually not needed, unless I am missing something).
Attachment #8353755 - Flags: review?(florian)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Attached patch PatchSplinter Review
*** Original post on bio 1734 as attmnt 1997 at 2012-10-19 20:32:00 UTC ***

Sorry for forgetting about this update.
Attachment #8353756 - Flags: review?(florian)
Comment on attachment 8353755 [details] [diff] [review]
Patch

*** Original change on bio 1734 attmnt 1996 at 2012-10-19 20:32:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353755 - Attachment is obsolete: true
Attachment #8353755 - Flags: review?(florian)
Hardware: x86 → All
Version: 1.2 → trunk
Comment on attachment 8353756 [details] [diff] [review]
Patch

*** Original change on bio 1734 attmnt 1997 at 2012-10-19 21:03:04 UTC ***

This looks OK to me, although:
- I haven't tested this.
- This really feels like code that needs unit tests.

Thanks for fixing this! :-)
Attachment #8353756 - Flags: review?(florian) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 1734 at 2012-10-20 05:26:13 UTC ***

Comment on attachment 8353756 [details] [diff] [review] (bio-attmnt 1997)
Patch

>diff --git a/modules/imSmileys.jsm b/modules/imSmileys.jsm
>-  let exp = /([\][)(\\|?^$*+])/g;
>+  let exp = /[[\]{}()*+?.\\^$|]/g;
Doesn't this need to be \. to physically match a . to replace it with a \.?
*** Original post on bio 1734 at 2012-10-20 16:06:33 UTC ***

(In reply to comment #5)
> Comment on attachment 8353756 [details] [diff] [review] (bio-attmnt 1997) [details]
> Patch
> 
> >diff --git a/modules/imSmileys.jsm b/modules/imSmileys.jsm
> >-  let exp = /([\][)(\\|?^$*+])/g;
> >+  let exp = /[[\]{}()*+?.\\^$|]/g;
> Doesn't this need to be \. to physically match a . to replace it with a \.?

You can check, but inside a [] I don't think we need to escape '.'.
*** Original post on bio 1734 at 2012-10-25 12:12:13 UTC ***

http://hg.instantbird.org/instantbird/rev/df0392aaf7af

Thanks for fixing this!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: