Closed Bug 244459 Opened 16 years ago Closed 13 years ago

Drag and drop ical (.ics) attachments from mail onto calendar

Categories

(Calendar :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: gekacheka, Assigned: michael.buettner)

References

Details

Attachments

(4 files, 3 obsolete files)

Dragging and dropping ical attachments from email or news is a useful subgoal of
bug 137093, which covers both whole emails and ical attachments.  

Note that copying and pasting VCALENDAR text from the mail view of the
attachment currently works (it brings up and initializes an event dialog for
each event).

Steps to reproduce:
1. bring up mozilla mailnews and calendar.
2. create a test event
3. email test event to self (context menu "email selected events")
4. delete test event from calendar
5. receive in mozilla mailnews, should have CalendarEvent.ics attachment.
6. drag the CalendarEvent.ics attachment onto the calendar grid.

Actual:  nothing happens
Expected:  Event dialog appears filled with test event data.
(as occurs when you view event.ics text in message pane, copy it, and paste
anywhere in calendar grid.)
This patch is a partial workaround.  It disables advertising the data flavors
for which receiving code is not implemented.  The result is that bestFlavor
becomes application/x-moz-file instead of text/x-moz-url.   

With this patch, dragging the ics attachment from the email pane and dropping
on Day view or Week view grid works (but fails on Multiweek view and Month
view).	Since it receives it as a file, it produces an "Saving" progress
dialog, which is a little annoying, but that is better than not working.

(Issue: why doesn't mail provide the attachment data using the contentType of
the attachment, which is text/calendar?)
Blocks: 137093
Comment on attachment 149171 [details] [diff] [review]
dragDrop.js patch: disable unimplemented drag data flavors( checked in )

Patch checked in.
Attachment #149171 - Attachment description: dragDrop.js patch: disable unimplemented drag data flavors → dragDrop.js patch: disable unimplemented drag data flavors( checked in )
Attachment #149171 - Flags: first-review+
I can confirm this on Windows/NT sp6a w/ Mozilla 1.7.2 and Calendar 2004080916.


I'm providing a Screen Shot showing Saving and Alert widgets upon DnD attempt
of ics Mail Attachment file to Calendar (w/ MultiWeek view displaying). (Note,
nothing is actually saved on disk, so no wonder it fails. After clicking OK in
Alert an new Alert w/ "aDataStream is not defined" is displayed.)

BTW, should saving actually be done in 'C:' by default, as indicated by the
screen shot? IMHO, wouldn't e.g. 'C:\temp\' be a better place for this on
Windows? (as it IS a temporary file).
The previous screen shot is valid for Day view, Week view, and MultiWeek view.
Note, that happens for all views, but for the Month view.

This screen shot displays what happens when the Month view is active.
I.e. the ics attachment is actually saved on disk (as 'C:\CalendarEvent.ics'),
but then Calendar launches the 'Add New Calendar' widget instead of importing
the event (which I, of course, don't wan't to do).
QA Contact: gurganbl → general
Attached patch fun with dropping (obsolete) — Splinter Review
Patch implements support for dropping local files, urls, text, and mail attachments in Sunbird and Lightning (mail attachments only work in Lightning).
Assignee: mostafah → jminta
Status: NEW → ASSIGNED
Attachment #226264 - Flags: first-review?(dmose)
Could you write down (spec) what dropping should do? A patch like this is fine, but i'd like to know what you think it should do.
(In reply to comment #6)
> Could you write down (spec) what dropping should do? A patch like this is fine,
> but i'd like to know what you think it should do.
> 
http://wiki.mozilla.org/Calendar:Feature_Implementations:Drag%27n%27Drop Is this what you're looking for?
Comment on attachment 226264 [details] [diff] [review]
fun with dropping

Moving reviews to ctalbert and lilmatt per dmose
Attachment #226264 - Flags: second-review?(lilmatt)
Attachment #226264 - Flags: first-review?(dmose)
Attachment #226264 - Flags: first-review?(cmtalbert)
(In reply to comment #5)
> Created an attachment (id=226264) [edit]
> fun with dropping
> 
> Patch implements support for dropping local files, urls, text, and mail
> attachments in Sunbird and Lightning (mail attachments only work in Lightning).
> 
This looks like a great fix, but would you mind un-bitroting it?
Here is a listing of the errors when I attempt to apply it, if that is helpful. (Attempted on a 20061023 Sunbird tree that has been updated to current base).

Thanks!

E:\BUILD\sunbird\mozilla\calendar>patch -p0 <dndpatch.txt
(Stripping trailing CRs from patch.)
patching file base/content/calDropListener.js
(Stripping trailing CRs from patch.)
patching file lightning/jar.mn
Hunk #1 FAILED at 24.
1 out of 1 hunk FAILED -- saving rejects to file lightning/jar.mn.rej
(Stripping trailing CRs from patch.)
patching file lightning/content/messenger-overlay-sidebar.xul
Hunk #1 FAILED at 63.
Hunk #2 FAILED at 112.
Hunk #3 succeeded at 204 (offset 43 lines).
2 out of 3 hunks FAILED -- saving rejects to file lightning/content/messenger-overlay-sidebar.xul.rej
(Stripping trailing CRs from patch.)
patching file resources/jar.mn
Hunk #1 FAILED at 13.
1 out of 1 hunk FAILED -- saving rejects to file resources/jar.mn.rej
(Stripping trailing CRs from patch.)
can't find file to patch at input line 354
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: resources/content/calendar.xul
|===================================================================
|RCS file: /cvsroot/mozilla/calendar/resources/content/calendar.xul,v
|retrieving revision 1.207
|diff -p -U8 -r1.207 calendar.xul
|--- resources/content/calendar.xul     19 Jun 2006 17:03:47 -0000      1.207
|+++ resources/content/calendar.xul     20 Jun 2006 01:17:51 -0000
--------------------------
File to patch: sunbird/base/content/calendar.xul
patching file sunbird/base/content/calendar.xul
Hunk #1 FAILED at 232.
Hunk #2 succeeded at 440 with fuzz 2 (offset -52 lines).
1 out of 2 hunks FAILED -- saving rejects to file sunbird/base/content/calendar.xul.rej
(Stripping trailing CRs from patch.)
patching file sunbird/base/content/calendar-scripts.inc
Hunk #1 FAILED at 77.
1 out of 1 hunk FAILED -- saving rejects to file sunbird/base/content/calendar-scripts.inc.rej
(Stripping trailing CRs from patch.)
patching file sunbird/base/content/calendar.xul
Hunk #1 FAILED at 418.
1 out of 1 hunk FAILED -- saving rejects to file sunbird/base/content/calendar.xul.rej
I'd like to wait on bug 224977 before attaching a new version of this patch, since nsTransferable is going away.
(In reply to comment #10)
It's gone. :)
This is a new patch. It doesn't add or subtract anything from jminta's previous patch. I have carried the reviews forward on this patch, and marked the previous one as obsolete.

In doing this merge, I have reviewed the code, and I found it ok. However, when I applied it to my Lightning build. I found that dragging a text file with a VEVENT encapsulated in it from my file system into the month view of lightning caused this error (attempted with both a .txt and a .ics file on Mac OS X):
JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "JS frame :: chrome://calendar/content/calDropListener.js :: calDNDDrop :: line 68"  data: no]

Then I attempted to drag an ICS attachment from a mail message to the month view of Lightning, and I got these errors:
###!!! ASSERTION: NS_Alloc of size 0: 'size', file /Users/ctalbert/Programs/lightning/mozilla/xpcom/base/nsMemoryImpl.cpp, line 335
Break: at file /Users/ctalbert/Programs/lightning/mozilla/xpcom/base/nsMemoryImpl.cpp, line 335
Error parsing: '                    ': 7 (FILE: An operation on a file failed. Check errno for more detail.)
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x804a0107 [calIICSService.parseICS]"  nsresult: "0x804a0107 (<unknown>)"  location: "JS frame :: file:///Users/ctalbert/Library/Thunderbird/Profiles/8ak3slaf.TEST2/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/js/calIcsImportExport.js :: ics_importFromStream :: line 104"  data: no]
************************************************************
JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x804a0107 [calIICSService.parseICS]"  nsresult: "0x804a0107 (<unknown>)"  location: "JS frame :: file:///Users/ctalbert/Library/Thunderbird/Profiles/8ak3slaf.TEST2/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/js/calIcsImportExport.js :: ics_importFromStream :: line 104"  data: no]
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "gMsgFolderSelected has no properties" {file: "chrome://messenger/content/msgMail3PaneWindow.js" line: 227}]' when calling method: [nsIFolderListener::OnItemEvent]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
************************************************************

So, I am going to attach this patch, but I want to withhold my review until we can figure out what is going on here with these errors.
Attachment #226264 - Attachment is obsolete: true
Attachment #244974 - Flags: second-review?(lilmatt)
Attachment #244974 - Flags: first-review?(cmtalbert)
Attachment #226264 - Flags: second-review?(lilmatt)
Attachment #226264 - Flags: first-review?(cmtalbert)
Comment on attachment 244974 [details] [diff] [review]
An updated patch that applies easier.

Removing r2 request, since this patch needs the errors fixed.
Attachment #244974 - Flags: second-review?(lilmatt)
Blocking 0.5?
Flags: blocking-calendar0.5?
We won't hold 0.5 for this.
Flags: blocking-calendar0.5? → blocking-calendar0.5-
Attachment #244974 - Flags: first-review?(ctalbert.moz) → review?(ctalbert.moz)
Duplicate of this bug: 320793
Duplicate of this bug: 315100
Duplicate of this bug: 137093
Duplicate of this bug: 382819
Attached patch final patch v1 (obsolete) — Splinter Review
This is an updated version of the patch including all the necessary bits and pieces to actually make this work. There's just the skeleton of the original patch left intact, I rewrote a lot of this stuff. Dropping an arbitrary ics file from the local file system as well as throwing an email attachment into the calendar view now works as advertised.
Assignee: jminta → michael.buettner
Attachment #244974 - Attachment is obsolete: true
Attachment #268938 - Flags: review?(daniel.boelzle)
Attachment #244974 - Flags: review?(ctalbert)
Comment on attachment 268938 [details] [diff] [review]
final patch v1

>+            case "text/calendar":
>+                var icssrv = Cc["@mozilla.org/calendar/ics-service;1"]
>+                               .getService(Ci.calIICSService);
>+                var calComp = icssrv.parseICS(data);
...
use calIcsParser for that; it minds contained exception records which this code does not.

>+            case "text/unicode":
...
try {
>+
>+                var items = importer.importFromStream(inputStream, {});
} finally {
>+                inputStream.close();

mind exception safety here, too; who calls endBatchTransaction in case doTransaction throws an exception?
>+                startBatchTransaction();
>+                for each (item in items) {
>+                    doTransaction('add', item, destCal, null, null);
>+                }
>+                endBatchTransaction();
>+                break;

>+            case "application/x-moz-file-promise":
>+            case "text/x-moz-url":
>+                var ioService = Components.classes["@mozilla.org/network/io-service;1"]
>+                                          .getService(Components.interfaces.nsIIOService);
>+                var uri = ioService.newURI(data.toString(), null, null);
>+                var channel = ioService.newChannelFromURI(uri);
>+                channel.loadFlags |= Components.interfaces.nsIRequest.LOAD_BYPASS_CACHE;

>+                var streamLoader = Components.classes["@mozilla.org/network/stream-loader;1"]
>+                                             .createInstance(Components.interfaces.nsIStreamLoader);
...
Reading unicode data, I recommend to use unichar-stream-loader (http://www.xulplanet.com/references/xpcomref/comps/c_networkunicharstreamloader1.html), so no stream conversion code is necessary.
Attachment #268938 - Flags: review?(daniel.boelzle) → review-
This should go into 0.7
Flags: blocking-calendar0.7+
OS: Windows 2000 → All
Hardware: PC → All
Target Milestone: --- → 0.7
Whiteboard: [roadmap 0.7]
Whiteboard: [roadmap 0.7]
Whiteboard: [patch in hand]
Mickey, are you going to finish this for 0.7 or can we live without it? Does the patch take the new UI (different modes) into account?
(In reply to comment #23)
> Mickey, are you going to finish this for 0.7 or can we live without it? Does
> the patch take the new UI (different modes) into account?
I'll come up with a revised version of this patch probably tomorrow (08-29).
Attached patch final patch v2Splinter Review
This is an updated version of the patch with the above comments being addressed.
Attachment #268938 - Attachment is obsolete: true
Attachment #279068 - Flags: review?(daniel.boelzle)
Comment on attachment 279068 [details] [diff] [review]
final patch v2

>+   onDrop: function calDNDDrop(aEvent, aTransferData, aDragSession) {
indent one space

>+                var parser = Cc["@mozilla.org/calendar/ics-parser;1"].
>+                             createInstance(Ci.calIIcsParser);
move dot to second line

>+                try {
>+                    startBatchTransaction();
move startBatchTransaction() before the try-block: in case it throws, we must not call endBatchTransaction()

>+                var inputStream = Components.classes[
>+                                  "@mozilla.org/network/file-input-stream;1"]
same line IMO looks better

>+                inputStream.init(localFileInstance, MODE_RDONLY, 0444, {});
IMO last parameter behaviourFlags doesn't fit, see <http://www.xulplanet.com/references/xpcomref/ifaces/nsIFileInputStream.html#method_init>

>+                //XXX support csv
>+                var importer = Cc["@mozilla.org/calendar/import;1?type=ics"]
>+                                .getService(Ci.calIImporter);
move importer into try-block below

>+                try {
>+                    var items = importer.importFromStream(inputStream, {});
>+                    try {
>+                        startBatchTransaction();
startBatchTransaction() ^^^^

>+                        for each (item in items) {
>+                            doTransaction('add', item, destCal, null, null);
>+                        }
>+                    }
>+                    finally {
>+                        endBatchTransaction();
>+                    }
>+                }
>+                finally {
>+                    inputStream.close();
>+                }
>+
>+                break;
>+            case "application/x-moz-file-promise":
>+            case "text/x-moz-url":
>+                var ioService = Components.classes["@mozilla.org/network/io-service;1"]
>+                                          .getService(Components.interfaces.nsIIOService);
use Cc/Ci exor verbose names in the file; I prefer the latter

>+                var uri = ioService.newURI(data.toString(), null, null);
>+                var loader = Components.classes["@mozilla.org/network/unichar-stream-loader;1"]
>+                                       .createInstance(Components.interfaces.nsIUnicharStreamLoader);
>+                channel = ioService.newChannelFromURI(uri);
>+                channel.loadFlags |= Components.interfaces.nsIRequest.LOAD_BYPASS_CACHE;
>+        
>+                var listener = {
>+
>+                  // nsIUnicharStreamLoaderObserver:
>+                  onDetermineCharset: function(loader, context, firstSegment, length) {
indentation

>+                          try {
>+                              startBatchTransaction();
^^^^

>+                              for each (var item in parser.getItems({})) {
>+                                  doTransaction('add', item, destCal, null, null);
>+                              }
>+                          }
>+                          finally {
>+                              endBatchTransaction();
>+                          }

>+                try {
>+                    loader.init(channel, listener, null, 0);
>+                } catch(e) {}
I'd prefer Component.utils.reportError(e);

>+                break;
>+            default:
>+                dump("unknown data flavour:" + bestFlavor.value+'\n');
spend a break; .)

r=dbo with that fixed
Attachment #279068 - Flags: review?(daniel.boelzle) → review+
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
No longer blocks: 137093
Michael, could you add a few sentence hot this feature is supposed to work? For example what can be dragged and where can it be dropped? This would make it easier for testing and validation.
Whiteboard: [patch in hand]
(In reply to comment #28)
> Michael, could you add a few sentence hot this feature is supposed to work? For
> example what can be dragged and where can it be dropped?
Sure. This patch allows ics files to be dropped onto any available calendar view (day, week, multiweek, month). The source can be a file (from the desktop, explorer, etc.) or an email attachment (from thunderbird's compose window). This currently works for ics files only, but the infrastructure for other formats is now in place as well.
Verified with lightning 2007090504
Status: RESOLVED → VERIFIED
Flags: blocking-calendar0.7+
Flags: blocking-calendar0.5-
You need to log in before you can comment on or make changes to this bug.