Closed
Bug 738078
Opened 12 years ago
Closed 12 years ago
Make use of BigFiles/FileLink to attach files in the event dialog
Categories
(Calendar :: Dialogs, defect)
Calendar
Dialogs
Tracking
(Not tracked)
RESOLVED
FIXED
1.8
People
(Reporter: Fallen, Assigned: Fallen)
References
Details
Attachments
(1 file, 1 obsolete file)
17.76 KB,
patch
|
mmecca
:
review+
|
Details | Diff | Splinter Review |
Now that Thunderbird supports BigFiles/FileLink, we could very well make use of it to offer makeshift file attachments. This is actually quite easy, I have a work in progress patch that implements the basic features. I just have to clean up error reporting, finish the UI work and some improvements on how the async operations are handled. Something I feel quite uncomfortable with is saving the cloud provider account key and original filename on the attachment object. It will be serialized as: ATTACH;X-MOZ-CLOUD-PROVIDER=account1;X-MOZ-CLOUD-FILENAME=08sim.pdf:http:/ /db.tt/FUsxqHrR I could likely get rid of the provider parameter, but how would I get the filename? Also, deleting attachments directly on the remote store only works within one event dialog session, as there is no method to delete a file using its public URL.
Assignee | ||
Comment 1•12 years ago
|
||
I've decided to push the strings for this bug in comm-central changeset dd788105a464 before the merge, this has been work in progress long enough. Lets get this show on the road, I promise^H^H^H^H^H^H^Hthink I'll fix the patch shortly after the merge.
Version: Trunk → Sunbird 0.9
Assignee | ||
Updated•12 years ago
|
Version: Sunbird 0.9 → Trunk
Assignee | ||
Comment 2•12 years ago
|
||
There we go, here is the patch for comm-central. Please test thoroughly. There is one issue remaining, I use the FILENAME attribute from http://tools.ietf.org/html/draft-daboo-caldav-attachments-01 but I haven't yet found a good solution for the security issue. Theoretically there could be evil crafting something like this: ATTACH;FILENAME=your_bill.pdf:http://example.com/evil.scr to trick the user into downloading the file. I do have the tooltip set to the full uri, but I don't know if thats enough or how else we can prevent this.
Attachment #644626 -
Flags: review?(matthew.mecca)
Comment 3•12 years ago
|
||
Comment on attachment 644626 [details] [diff] [review] Fix - v1 Review of attachment 644626 [details] [diff] [review]: ----------------------------------------------------------------- Overall it works well, but there are a few issues: - The image in the attachment list doesn't show up for me until the file upload is completed, the loading icon doesn't ever appear. - When the upload fails, I don't see the error image either, the file isn't removed from the attachment list, and the following appears in the error console: Error: TypeError: documentLink.removeItem is not a function Source file: chrome://calendar/content/calendar-event-dialog.js Line: 1819 - With Storage calendar items, the file URL appears in the attachment list instead of the file name after restarting Thunderbird. ::: calendar/base/content/dialogs/calendar-event-dialog.js @@ +1782,4 @@ > return name; > } > > +function uploadCloudAttachment(attachment, cloudProvider, item) { Using "item" as the argument name is a little confusing here - when it is used in other functions in the file it usually refers to the calendar item, while in this case it refers to a list item element. A function comment might be useful here also.
Attachment #644626 -
Flags: review?(matthew.mecca) → review-
Assignee | ||
Comment 4•12 years ago
|
||
(In reply to Matthew Mecca [:mmecca] from comment #3) Fixed all issues, except for: > - With Storage calendar items, the file URL appears in the attachment list > instead of the file name after restarting Thunderbird. > This is very unfortunate. Fixing the problem makes me want to fix so many other things though: * the prop is lost, because the storage calendar doesn't save it * this makes me want to change the cal_attachments table to save the whole icalString, but there is no setter for this in calIIcalProperty * this makes me want to add a such setter, but that means changing our C++ code * this makes me want to get rid of our C++ code * it also makes me want to rewrite the storage provider so the whole item is in one blob and only indexed values are saved in extra fields so you see, I think its easier to just accept this problem for now :) > Using "item" as the argument name is a little confusing here - when it is > used in other functions in the file it usually refers to the calendar item, > while in this case it refers to a list item element. A function comment > might be useful here also. Changed to listItem and added a function comment. Do you think you could review this in the next few days? I'd like to push this down to beta since we also have the strings there.
Attachment #644626 -
Attachment is obsolete: true
Attachment #655288 -
Flags: review?(matthew.mecca)
Comment 5•12 years ago
|
||
Comment on attachment 655288 [details] [diff] [review] Fix - v2 r=mmecca
Attachment #655288 -
Flags: review?(matthew.mecca) → review+
Assignee | ||
Comment 6•12 years ago
|
||
Pushed to comm-central changeset 58cd3591ca84
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.9
Assignee | ||
Comment 7•12 years ago
|
||
Backported to releases/comm-aurora changeset 4ab2340c2d04
Target Milestone: 1.9 → 1.8
Assignee | ||
Comment 8•12 years ago
|
||
The comm-aurora patch obviously doesn't have the string changes.
You need to log in
before you can comment on or make changes to this bug.
Description
•