Closed Bug 388018 Opened 17 years ago Closed 17 years ago

Mode Toolbar: Prepare Items to perform as Drop Target

Categories

(Calendar :: Calendar Frontend, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: chris.j.bugzilla, Assigned: michael.buettner)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

It should be possible to convert 

- E-Mails to Events, Tasks
- Tasks to E-Mails, Events
- Events to E-Mails, Tasks

by dragging and dropping items onto the Mode Toolbar

The spec will be written here:
http://wiki.mozilla.org/Calendar:Convertion_of_E-Mails_Tasks_and_Events
This was decided to be on the 0.7 roadmap as per F2F meeting
Blocks: 247764
Severity: normal → enhancement
Flags: blocking-calendar0.7+
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → 0.7
Assignee: nobody → michael.buettner
Christian, it would be great if you could come up with a description on which this feature should work. The pointer to the specification currently just leads to the newsgroup discussion without a clear consensus. As I'm having a hell of a lot of other stuff on my plate, I'm going to assign this to nobody and removing this from the blocker list. Just spend me more heads (in addition to the two I'm already owning) and I take it back...
Assignee: michael.buettner → nobody
Flags: blocking-calendar0.7+
Flags: wanted-calendar0.8?
Target Milestone: 0.7 → ---
This is indeed one of the features that we probably want to make it for the next release -> Reassigning.
Assignee: nobody → michael.buettner
It would be good to have this in 0.8
Flags: wanted-calendar0.8? → wanted-calendar0.8+
Attached patch patch v1 (obsolete) — Splinter Review
This patch buys us the following list of features:

- drag email onto calendar button -> create new event (title = email subject)
- drag email onto task button -> create new task with email subject as title
- drag event (from any view) onto task button -> create task from event
- drag event (from any view) onto mail button -> sends mail to organizer
- drag task (from unifinder todo or task view) onto calendar button -> new event
- drag task (from unifinder todo or task view) onto mail button -> new mail

the patch builds upon the already existing calendarDNDObserver but adds some derived classes that implement the specific behaviors that are needed for the mode toolbar buttons. the existing observer has been modified to allow it to be used as a base class.

in addition to the drop-handlers this patch contains a fair amount of code to allow the drag operations to be initiated. the most complex one is (as always) the multiday view as it's already possible to drag events around in order to move them to another time. this patch detects if the event leaves the view area and initiates a (real) drag operation in this case. unfortunately, there's no way of canceling a drag operation once started. that's why it's not possible to restore the drag operation inside of the view once the cursor left the view (this feature is available on trunk - but not on branch18, btw). the other views (as well as the unifinder todo) just needed minor modifications.

this patch doesn't contain the changes for the context menues (convert x to y) as this bug is only concerned with the mode toolbar buttons. i'll provide the appropriate context menu patch in a separate bug.
Attachment #292039 - Flags: ui-review?(christian.jansen)
Attachment #292039 - Flags: review?(daniel.boelzle)
I just had a discussion with Daniel regarding an issue with dragging occurrences around. With the current version of the patch we have a problem when taking a single occurrence (e.g. from the views) and dropping it, for example, on the task button. The ics fragment that will be received at the drop target is a vcalendar with a single occurence (a vevent with a recurrence-id but no parent). The ics parser currently swallows this item silently and the drop target code won't see anything when asking for the parsed items. This is just the same underlying problem as with bug 357399.

A workaround could be the following piece of code that creates a stand-alone event from an occurrence when initiating the drag operation:

>              var item = dragState.dragOccurrence;
>              item = item.clone();
>              item.id = getUUID();
>              item.recurrenceId = null;
>              item.recurrenceInfo = null;

This just works but is an island solution tied to this particular use case. I'd favor a solution that also solves the problem in a more general way. Any ideas?
Christian was so kind to provide a conversion table that specifies which properties are getting mapped to each other when converting items into each other, see [1] for details.

[1] http://wiki.mozilla.org/Calendar:Mail-Event-Task_Conversion
Attached patch patch v2 (obsolete) — Splinter Review
This version of the patch incorporates the previously missing bits and pieces that  the conversion table (see previous comment) now specifies. The most notable additions are as follow:

- Constructing calendar items (events/items) from messages now have automatically the list of attendees prepared from the to/cc fields of the message.
- Constructing calendar items (events/items) from messages now incorporates the body of the original message (html converted to plain text, of course).
Attachment #292039 - Attachment is obsolete: true
Attachment #292929 - Flags: ui-review?(christian.jansen)
Attachment #292929 - Flags: review?(daniel.boelzle)
Attachment #292039 - Flags: ui-review?(christian.jansen)
Attachment #292039 - Flags: review?(daniel.boelzle)
Comment on attachment 292929 [details] [diff] [review]
patch v2

>+  	    var content = document.getElementById("messagepane");
>+  	    if (content) {
>+      	    var messagePrefix = /^mailbox-message:|^imap-message:|^news-message:/i;
>+      	    if (messagePrefix.test(GetLoadedMessage())) {
>+	              var message = content.contentDocument;
>+	              var body = message.body;
>+	              if (body) {
>+                    aItem.setProperty(
>+                        "DESCRIPTION",
>+                        htmlToPlainText(body.innerHTML));
>+	              }
>+      	    }
>+  	    }
>+    
>+    },
>+
some copy/paste tabs and trailing spaces above

>+calendarViewDNDObserver.prototype = new calendarDNDBaseObserver();
>+calendarViewDNDObserver.prototype.constructor = calendarViewDNDObserver();

Id' rather prefer to do inheritance like we do in calEvent/calItemBase or in the providers, assigning the __proto__ property and defining a base initializer function for calendarDNDBaseObserver that is explicitly called in both ctor functions, because once someone adds ctor code to calendarDNDBaseObserver(), we loose that in calendarViewDNDObserver.

>+calendarCalendarButtonDNDObserver.prototype.onDropItems = function(aItems) {
...
>+            var oldItem = newItem.clone();
>+            newItem = createEvent();
>+            oldItem.wrappedJSObject.cloneItemBaseInto(newItem.wrappedJSObject);
>+            newItem.startDate = oldItem.entryDate || now();
>+            newItem.endDate = oldItem.dueDate || now();
why don't you use plain calIItemBase::clone() and reset the id attribute (to null) resp. deleteProperty("UID")?

>+              var flavourProvider = {
>+                  QueryInterface: function(aIID) {
>+                      if (aIID.equals(Components.interfaces.nsIFlavorDataProvider) ||
>+                          aIID.equals(Components.interfaces.nsISupports)) {
>+                          return this;
>+                      }
>+                      throw Components.results.NS_NOINTERFACE;
>+                  },
Please use doQueryInterface or ensureIid for brevity.

>+                  item: item,
>+
>+                  getFlavorData: function(aInTransferable, aInFlavor, aOutData, aOutDataLen) {
>+                      if ((aInFlavor == "application/vnd.x-moz-cal-event") ||
>+                          (aInFlavor == "application/vnd.x-moz-cal-task")) {
>+                          aOutData.value = this.item;
>+                          aOutDataLen.value = 1;
>+                      } else {
>+                          alert("error:"+aInFlavor);
Please use ASSERT or the like.

>+        } catch(ex) {
>+          dump("Error while building selection region: " + ex + "\n");
Please use ASSERT.

>+        var flavourProvider = {
>+            QueryInterface: function(aIID) {
>+                if (aIID.equals(Components.interfaces.nsIFlavorDataProvider) ||
>+                    aIID.equals(Components.interfaces.nsISupports)) {
>+                    return this;
>+                }
doQueryInterface/ensureIid

just small nits, r=dbo
Attachment #292929 - Flags: review?(daniel.boelzle) → review+
Comment on attachment 292929 [details] [diff] [review]
patch v2

r=christian

Please find the updated conversion table here.
http://spreadsheets.google.com/pub?key=plCAueWeXt4ibydy7QatMFw
Attachment #292929 - Flags: ui-review?(christian.jansen) → ui-review+
Attached patch patch v3Splinter Review
Patch with above comments addressed, carrying forward r+ & ui-r+.

I changed the inheritance chain as suggested. I also had a discussion regarding cloneItemBaseInto() since I wouldn't know how to call calIItemBase::clone() to convert events to task (and visa versa). We decided to use setItemBaseFromICS(), the code snippet looks like this:

>        if (!isToDo(item)) {
>            var newItem = createTodo();
>            newItem.wrappedJSObject.setItemBaseFromICS(
>                item.icalComponent);
>            newItem.entryDate = item.startDate || now();
>            newItem.dueDate = item.endDate || now();
>            createTodoWithDialog(null, null, null, newItem);
>        }

I'm going to land this patch now...
Attachment #292929 - Attachment is obsolete: true
Attachment #293131 - Flags: ui-review+
Attachment #293131 - Flags: review+
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Verified with Lt 2007121501
Status: RESOLVED → VERIFIED
Blocks: 408466
Depends on: 417429
This change broke Sunbird, see Bug 417429.
Comment on attachment 293131 [details] [diff] [review]
patch v3

>--- mozilla_ref/calendar/base/src/calUtils.js	Thu Dec 13 10:34:22 2007
>+++ mozilla/calendar/base/src/calUtils.js	Fri Dec 14 14:35:33 2007
> 
> function sendMailTo(aRecipient, aSubject, aBody) {
>-    if (!aRecipient || aRecipient.length < 1) {
>-        return;
>-    }
> 
>     if (Components.classes["@mozilla.org/messengercompose;1"]) {
>         // We are in Thunderbird, we can use the compose interface directly
>         var msgComposeService = Components.classes["@mozilla.org/messengercompose;1"]
>                                 .getService(Components.interfaces.nsIMsgComposeService);
>         var msgParams = Components.classes["@mozilla.org/messengercompose/composeparams;1"]
>                         .createInstance(Components.interfaces.nsIMsgComposeParams);
>         var composeFields = Components.classes["@mozilla.org/messengercompose/composefields;1"]
>@@ -1320,18 +1317,21 @@ function sendMailTo(aRecipient, aSubject
>     } else {
>         // We are in a place without a composer. Use the external protocol
>         // service.
>         var protoSvc = Components.classes["@mozilla.org/uriloader/external-protocol-service;1"]
>                        .getService(Components.interfaces.nsIExternalProtocolService);
>         var ioService = Components.classes["@mozilla.org/network/io-service;1"]
>                         .getService(Components.interfaces.nsIIOService);
> 
>-        var uriString = "mailto:" + aRecipient;
>+        var uriString = "";
>         var uriParams = [];
>+        if (!aRecipient || aRecipient.length < 1) {
>+            uriString = "mailto:" + aRecipient;
>+        }

Mickey, what was the purpose of this change? This probably caused bug 417429?
(In reply to comment #16)
>-        var uriString = "mailto:" + aRecipient;
>+        var uriString = "";
>         var uriParams = [];
>+        if (!aRecipient || aRecipient.length < 1) {
>+            uriString = "mailto:" + aRecipient;
>+        }
> 
> Mickey, what was the purpose of this change? This probably caused bug 417429?
The purpose was to make the function sendMailTo() allow sending mails without being forced to specify a dedicated recipient. Assume we drop an event without an organizer onto the mail button, in this case the mail compose window should come up with an empty recipient field. Without the above change, sendMailTo() just silently failed.
You need to log in before you can comment on or make changes to this bug.