Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Attachment reminder doesn't recognize file names with matching file-type extensions

RESOLVED FIXED in Thunderbird 11.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: rsx11m, Assigned: jyeo)

Tracking

({regression, student-project})

Trunk
Thunderbird 11.0
regression, student-project
Bug Flags:
wanted-thunderbird +
in-testsuite +

Firefox Tracking Flags

(blocking-thunderbird5.0 -, thunderbird5.0 wanted, blocking-thunderbird3.1 -)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

7 years ago
This seems to have worked as introduced in bug 492555 comment #21, but while testing for bug 635938, I've noticed that something like "blah.doc" does no longer trigger the attachment reminder whereas "blah .doc" with as space does. Multi-token words like "semi-attached" still trigger based on "attached", but somewhere the "." handling must have got lost which is needed to correctly
match any file extensions in file names.

Tested in current Windows and Linux trunk builds.
(Reporter)

Comment 1

7 years ago
Interesting, it doesn't seem to work in Thunderbird 3.1.8 already, so this
must have sneaked in earlier. I don't have 3.0.x any more around.
blocking-thunderbird3.1: --- → ?
blocking-thunderbird5.0: --- → ?
Keywords: regressionwindow-wanted
Taking to put on my plate for regression window hunting..
Assignee: nobody → gary
Works:

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091110 Shredder/3.1a1pre

Fails:

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091111 Shredder/3.1a1pre

http://hg.mozilla.org/comm-central/pushloghtml?startdate=2009-11-10&enddate=2009-11-11

Highly likely to be bug 527018.
Assignee: gary → nobody
Blocks: 527018
Keywords: regressionwindow-wanted
I strongly agree.  And since we have some pretty decent test coverage in this area, this should be a simple bug for someone new (or a student) to fix, but I forget the magic whiteboard incantation for that…

Thanks for finding the problem commit!
(In reply to comment #4)
> I strongly agree.  And since we have some pretty decent test coverage in this
> area, this should be a simple bug for someone new (or a student) to fix, but I
> forget the magic whiteboard incantation for that…

"student-project" keyword will be the magic..

> Thanks for finding the problem commit!

No problem, took me a quite a while, but eventually got it nonetheless. :)
Keywords: student-project
Whiteboard: [good first bug]
(Reporter)

Comment 6

6 years ago
Interestingly, a link like http://blah.com/blah1.pdf still triggers the attachment reminder based on the ".pdf" part, per bug 647871.
blocking-thunderbird3.1: ? → -
blocking-thunderbird5.0: ? → -
status-thunderbird5.0: --- → wanted
Flags: wanted-thunderbird+
(Assignee)

Comment 7

6 years ago
Created attachment 560746 [details] [diff] [review]
635993patch
Attachment #560746 - Flags: review?(bwinton)
(Assignee)

Updated

6 years ago
Assignee: nobody → jasonyeo88
Comment on attachment 560746 [details] [diff] [review]
635993patch

Review of attachment 560746 [details] [diff] [review]:
-----------------------------------------------------------------

Given that we've already regressed this once, I think we should add some tests to make sure we're still doing the right thing.

I also don't see the attachment reminder pop up for _any_ text I type in, so, uh, this is extra-hard to make sure we've gotten right.

Thanks,
Blake.
Attachment #560746 - Flags: review?(bwinton) → review-
(Assignee)

Comment 9

6 years ago
Created attachment 565746 [details] [diff] [review]
635993#2.patch
Attachment #565746 - Flags: review?(bwinton)
(Assignee)

Updated

6 years ago
Attachment #560746 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
Created attachment 565750 [details] [diff] [review]
635993#3.patch
Attachment #565746 - Attachment is obsolete: true
Attachment #565746 - Flags: review?(bwinton)
Attachment #565750 - Flags: review?(bwinton)
(Assignee)

Comment 11

6 years ago
Created attachment 565751 [details] [diff] [review]
635993#4.patch
Attachment #565750 - Attachment is obsolete: true
Attachment #565750 - Flags: review?(bwinton)
Attachment #565751 - Flags: review?(bwinton)
(Assignee)

Comment 12

6 years ago
Created attachment 567597 [details] [diff] [review]
635993#5.patch

Trimmed my patch to less than 8 characters per line.
Attachment #565751 - Attachment is obsolete: true
Attachment #565751 - Flags: review?(bwinton)
Attachment #567597 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #567597 - Flags: review? → review?(bwinton)
(Assignee)

Comment 13

6 years ago
> Trimmed my patch to less than 8 characters per line.
i meant 80 characters...
Comment on attachment 567597 [details] [diff] [review]
635993#5.patch

Review of attachment 567597 [details] [diff] [review]:
-----------------------------------------------------------------

This is certainly an improvement, but I think there are a few more cases that we want to handle, so I'm going to say r-, because I want another chance to think up evil tests after you've fixed these ones.  ;)

Thanks,
Blake.

::: mail/base/modules/attachmentChecker.js
@@ -91,9 +91,9 @@
> > -    var start = IsCJK(kw.charCodeAt(0)) ? "" : ("(^|" + NOT_W + ")");
> > +    var re;
> > -    var end = IsCJK(kw.charCodeAt(kw.length - 1)) ? "" : ("(" + NOT_W + "|$)");
> > +    var matching;
NaN more ...
> > +      re = new RegExp("(([^\\s]*)\\b)" + kw + "\\b", "ig");
> > -    if (matching && !(/^http|^ftp/i.test(matching[0])))
> > +      matching = mailData.match(re);
> > -      // We're not worried about matching too much because we only add the
NaN more ...

Since this for loop has a few lines in its body, I'ld like to see {}s around it.

::: mail/base/test/unit/test_attachmentChecker.js
@@ -100,0 +86,6 @@
> > +  ["Simple keyword", "latte.ca", "latte", "latte"],
> > +  ["Extension", "testing document.pdf", ".pdf", "document.pdf"],
> > +  ["Two Extensions", "testing document.pdf and test.pdf", ".pdf",
> > +    "document.pdf,test.pdf"],
NaN more ...

// Some tests that fail that I think should pass...
["Should match", "I've attached the http/test.pdf file", ".pdf", "http/test.pdf"],
["Should still fail", "a https://test.pdf a", ".pdf", ""],
["Should match Japanese", "a test.添付 a", ".添付", "test.添付"],
["Should match Greek", "a test.Θεωρία a", ".Θεωρία", "test.Θεωρία"],
["Should match once", "a test.pdf.doc a", ".pdf,.doc", "test.pdf.doc"],
Attachment #567597 - Flags: review?(bwinton) → review-
(Assignee)

Comment 15

6 years ago
Created attachment 575888 [details] [diff] [review]
635993#6.patch

I've added Blake's test cases and made the patch pass them.
Attachment #567597 - Attachment is obsolete: true
Attachment #575888 - Flags: review?(bwinton)
Comment on attachment 575888 [details] [diff] [review]
635993#6.patch

Review of attachment 575888 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!  The code is now complicated enough that I'm not entirely sure it's correct, but the test coverage is awesome, and so I believe that we're doing the right thing.

r=me!
Attachment #575888 - Flags: review?(bwinton) → review+
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/d2d1125046e9
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 11.0
You need to log in before you can comment on or make changes to this bug.