Add an attachmentInfos noun to gloda messages, including, but not limited to, attachment url, name, size, and content-type

RESOLVED FIXED in Thunderbird 3.3a1

Status

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: protz, Assigned: protz)

Tracking

unspecified
Thunderbird 3.3a1
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
This is a highly wanted feature for: Thunderbird Air, the Conversation View, and possibly other extensions or experiments out there. Following discussion with Andrew, what will be added is an extra noun that does duplicate other information available (name and content-type), but that is not included in the faceted search results nor in the full text index (bottom line is: no one's really motivated to add a faceting on the attachment's sizes).

So something like glodaMsg.attachmentInfos = [{ name: ..., size: ..., url: ..., contentType: ..., ...}, ...]

The bug this depends on should be fixed shortly.
(Assignee)

Comment 1

8 years ago
... and assigning to me.
Assignee: nobody → jonathan.protzenko
Status: NEW → ASSIGNED
(Assignee)

Comment 2

8 years ago
So the plan is basically:
- add a new class (GlodaAttachment) inside datamodel.js and a corresponding NOUN_ATTACHMENT constant
- add the corresponding NOUN_ATTACHMENT inside gloda.js
- call defineNoun inside gloda.js
- call defineAttribute from fundattr.js
- fill the attribute's value from fundattr.js

Did I forget something?
- unit tests! :)

sounds reasonable.  you could also break out the noun definition out into its own noun_* file if you think the implementation is going to get complex.
(Assignee)

Comment 4

8 years ago
Created attachment 467181 [details] [diff] [review]
WIP (1)

Here's my first try at the patch. Please note that it applies on top of the patch from bug 561851 (I merely added extra tests -- sure, I could cancel the libmime patch and update it, but I think we both want to see that one checked in soon). Anyway, this patch adds super-duper tests, and yes, it does do as advertised: basically, it adds a attachmentInfos property (just like attachmentTypes and attachmentNames) that's not used for indexing but that simply exposes more information about that gloda message's attachments.

glodaMsg.attachmentsInfos[i] has the following properties: name, size, contentType and url so that you can stream the attachment into some iframe should you want to.

If there's anything else you want me to cram in, please speak now.

Andrew, should you find this patch to your taste, I'll remove the debug and ask for review.
Attachment #467181 - Flags: feedback?(bugmail)
Comment on attachment 467181 [details] [diff] [review]
WIP (1)

Looking good!

From http://reviews.visophyte.org/r/467181/:

on file: mailnews/db/gloda/modules/datamodel.js line 128
>       if (nounDef.toParamAndValue) {

strict mode can get unhappy about this.  it's better to do if
("toParamAndValue" in nounDef) unless you are going to add the attribute to
your implementation but set it to null or undefined or what not.


on file: mailnews/db/gloda/modules/datamodel.js line 846
>  * @class An attachment, with as much information as we can gather on it

you can lose @class; I originally was autogenerating documentation nightly
with a tool that required that syntax to produce useful results, but that is
no longer happening.


on file: mailnews/db/gloda/modules/datamodel.js line 858
>   // set by GlodaDatastore
>   _datastore: null,

I feel like this should not be needed since no managed persistence happens for
this class. (The bit in datastore.js that sets it could also go too.)


on file: mailnews/db/gloda/modules/datamodel.js line 860
>   get id() { return 42 },

Is this still needed?  I think we should drop this as a requirement since
we're also dropping the toParamAndValue case.

In the event we can't drop it, a comment explaining why and perhaps having the
getter throw if accessed might be reasonable.


on file: mailnews/db/gloda/modules/fundattr.js line 372
>       emptySetIsSignificant: true,

This should probably be false since we don't care about emitting anything
queriable for this attribute.  (Setting this to true makes a special sentinel
attribute be emitted when the array on the object is empty, which is not
desired.)


on file: mailnews/db/gloda/test/unit/base_index_messages.js line 452
>       for (k in expectedInfos[i])

where is k scoped?
Attachment #467181 - Flags: feedback?(bugmail) → feedback+
(Assignee)

Comment 6

8 years ago
Created attachment 467213 [details] [diff] [review]
WIP (2)

Thanks for the quick review. I've taken the comments into account and it all seems to work even without an ID (which I guess was just used for the table storing the attributes pairings).
Attachment #467181 - Attachment is obsolete: true
Attachment #467213 - Flags: review?(bugmail)
(Assignee)

Comment 7

8 years ago
Oh and about the message/rfc822 thing, you were right, I wasn't building the attached message properly. I'd done the same mistake before on another patch, which I'm fixing as a side-effect at the very end of the patch.
(Assignee)

Comment 8

8 years ago
Also, if you could just run the xpcshell test under your windows box, that'd be great. I've tried not to reproduce the same mistake and I'm allowing a small delta for the attachment size, but we're never sure (and thunderbird-try doesn't run xpcshell tests).
TEST-UNEXPECTED-FAIL | c:/rev_control/hg/comm-central/objdir-thunderbird-debug/mozilla/_tests/xpcshell/test_mailnewsglob
aldb/unit/test_mime_attachments_size.js | false == true - See following stack:
JS frame :: c:\rev_control\hg\comm-central\mozilla\testing\xpcshell\head.js :: do_throw :: line 317
JS frame :: c:\rev_control\hg\comm-central\mozilla\testing\xpcshell\head.js :: do_check_eq :: line 347
JS frame :: c:\rev_control\hg\comm-central\mozilla\testing\xpcshell\head.js :: do_check_true :: line 359
JS frame :: c:/rev_control/hg/comm-central/objdir-thunderbird-debug/mozilla/_tests/xpcshell/test_mailnewsglobaldb/unit/t
est_mime_attachments_size.js :: check_attachments :: line 213
JS frame :: c:/rev_control/hg/comm-central/objdir-thunderbird-debug/mozilla/_tests/xpcshell/test_mailnewsglobaldb/unit/t
est_mime_attachments_size.js :: anonymous :: line 232
JS frame :: resource:///modules/gloda/mimemsg.js :: anonymous :: line 134
TEST-INFO | (xpcshell/head.js) | exiting test
TEST-UNEXPECTED-FAIL | c:/rev_control/hg/comm-central/objdir-thunderbird-debug/mozilla/_tests/xpcshell/test_mailnewsglob
aldb/unit/test_mime_attachments_size.js | 2147500036 - See following stack:
JS frame :: c:\rev_control\hg\comm-central\mozilla\testing\xpcshell\head.js :: do_throw :: line 317
JS frame :: c:/rev_control/hg/comm-central/objdir-thunderbird-debug/mozilla/_tests/xpcshell/test_mailnewsglobaldb/unit/t
est_mime_attachments_size.js :: anonymous :: line 234
JS frame :: resource:///modules/gloda/mimemsg.js :: anonymous :: line 134
(that was windows; linux xpcshell tests pass)
(Assignee)

Comment 11

8 years ago
Created attachment 467575 [details] [diff] [review]
WIP (3)

This patch passes all tests. Looks good to me :).
Attachment #467213 - Attachment is obsolete: true
Attachment #467575 - Flags: review?(bugmail)
Attachment #467213 - Flags: review?(bugmail)
Comment on attachment 467575 [details] [diff] [review]
WIP (3)

As briefly discussed, we will need to bump the schema revision for this to take full effect (or people will need to blow away their databases).  We aren't going to bump the schema revision for this since nothing in core cares right now, but presumably something else will bump it sooner or later.
Attachment #467575 - Flags: review?(bugmail) → review+
(Assignee)

Comment 13

8 years ago
Yes, by the time 3.2 is released, I guess someone will probably bump the schema revision. Otherwise, I can run a query and get all messages that have attachments. From then on, can I request Gloda to re-index all of these?

Marking as checkin-needed, thx for the review.
Keywords: checkin-needed
(In reply to comment #13)
> From then on, can I request Gloda to re-index all of these?

yes.
Checked in: http://hg.mozilla.org/comm-central/rev/78ea3006c4c5
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.2a1
(Assignee)

Updated

8 years ago
Attachment #467575 - Flags: approval-thunderbird3.2a1?
Attachment #467575 - Flags: approval-thunderbird3.2a1?
You need to log in before you can comment on or make changes to this bug.