312736 - Cannot move an Event or Item in month view using drag & drop

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Description

[Chin Kuan, Low]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051008 Firefox/1.4.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051008 Firefox/1.4.1

User can move an Item or Event using drag & drop in multiday view, but not in
month view.

Reproducible: Always

Comment 1

[Chin Kuan, Low]

Attachment: drag & drop patch for month view

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

cklow: I'll try and review this tomorrow (October 18).

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 199831 [details] [diff] [review]
drag & drop patch for month view

Vlad and I talked this over today, and we've come to the conclusion that
cloning the multiday-view drag-n-drop code probably isn't really the right
approach here.	In particular, the reason that the drag-n-drop code there was
hand-rolled with <stack>s etc. was because there's some fairly complex drawing
going on during the drag.  

In general, when we can, we'll probably do well to use the toolkit drag-n-drop
stuff (eg toolkit/content/nsDragAndDrop.js).   This will have two advantages:
the code should be simpler (we don't need to worry about all hand-rolled stuff
ourselves), and it should be easier to play nicely with other drop targets in
the rest of Thunderbird (and Firefox even).

Since the month view is a much more standard-style  drag-n-drop, chances are
good that the toolkit drag-n-drop will be sufficient, so if you could try that
approach, that'd be great.

Comment 4

[Chin Kuan, Low]

Attachment: updated patch using drag-n-drop toolkit

Comment 5

[Chin Kuan, Low]

Attachment: DND Observer for Month View

Comment 6

[Joey Minta]

Comment on attachment 200708 [details] [diff] [review]
updated patch using drag-n-drop toolkit

You need to ask for review from someone specific if you want this to get looked over.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 200708 [details] [diff] [review]
updated patch using drag-n-drop toolkit

>? base/content/calendarDragDrop.js
>Index: base/content/calendar-month-view.xml
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/base/content/calendar-month-view.xml,v
>retrieving revision 1.5
>diff -u -p -8 -r1.5 calendar-month-view.xml
>--- base/content/calendar-month-view.xml	28 Jun 2005 20:36:24 -0000	1.5
>+++ base/content/calendar-month-view.xml	25 Oct 2005 06:44:41 -0000
>@@ -121,44 +121,48 @@
> 
>       <handler event="dblclick"><![CDATA[
>         if (this.calendarView && this.calendarView.controller) {
>             event.preventBubble();
>             var occurrence = (event.ctrlKey) ? this.item.parentItem : this.item;
>             this.calendarView.controller.modifyOccurrence(occurrence);
>         }
>       ]]></handler>
>+      
>+      <handler event="draggesture"><![CDATA[
>+        nsDragAndDrop.startDrag(event, calendarDNDObserver);
>+      ]]></handler>
>     </handlers>
>   </binding>
> 
>   <binding id="calendar-month-day-box">
>     <content>
>       <xul:vbox>
>         <xul:label anonid="day-label" crop="end" class="calendar-month-day-box-date-label"/>
>         <xul:vbox anonid="day-items"/>
>       </xul:vbox>
>     </content>
> 
>     <implementation>
>       <field name="mDate">null</field>
>       <!-- mItemData will always be kept sorted in display order -->
>       <field name="mItemData">[]</field>
>-      <field name="mMonthView">null</field>
>+      <field name="mCalendarView">null</field>

Why the name change here?  I'm not necessarily against it, but I would like to know the reasoning.

>       <property name="displayCalendar">
>         <getter><![CDATA[
>            return this.mCalendar;
>         ]]></getter>
>         <setter><![CDATA[
>-           if (this.mCalendar != val) {
>-             this.mCalendar = val;
>-             this.refresh();
>-           }
>-           return val;
>+          if (this.mCalendar)
>+              this.mCalendar.removeObserver (this.mObserver);
>+          this.mCalendar = val;
>+          this.mCalendar.addObserver (this.mObserver);
>+
>+          this.refresh();

Ooh, good catch!  I had wondered why updates to the month view weren't displaying.

>+<script type="application/x-javascript" src="chrome://global/content/nsDragAndDrop.js"/>
>+<script type="application/x-javascript" src="chrome://global/content/nsTransferable.js"/>

I don't think we actually need to include nsTransferable.js here, as the toolkit version of nsDragAndDrop.js seems to have the entire nsTransferable.js cut-n-pasted inside of it (yech).

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 200710 [details]
DND Observer for Month View

In general, this (both attachments) look good.  A few comments...

>/* -*- Mode: javascript; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

A tab-width of 20 would be nice, because then it makes it really obvious when someone inadvertantly checks in a tab.

So the naming of this implies that you think we might someday end up using it for something besides just the month view, which seems plausible to me.  Was that your intent?  If so, it might be worth adding a comment here pointing out that even though this is used by some views, it's not used by all of them (eg multiday) to help the casual reader understand it better.

>var calendarDNDObserver = {
>
>    onDragStart: function (aEvent , aTransferData, aAction)
>    {
>        var bIsEvent = false, bIsTodo = false;

In general, local style doesn't use type-based hungarian named (eg b for boolean) just scope-based hungarian naming (eg k for constant, s for static, a for arg).  So if you could modify these names to match local style, that would be great.

>        var elemItem = aEvent.target;
>
>        if ( "item" in elemItem )
>        {
>            bIsEvent = (elemItem.item instanceof Components.interfaces.calIEvent);
>            bIsTodo = (elemItem.item instanceof Components.interfaces.calITodo);
>            
>            if ( bIsEvent || bIsTodo )
>            {
>                aTransferData.data = new TransferData();
>                aTransferData.data.addDataForFlavour("text/x-moz-" + (bIsEvent? "event" : "todo"), elemItem.item.id);

Instead of just passing around the item, how about passing around the interface pointer to the item instead?  Eg, have the flavours be defined "application/x-moz-{event,todo}" and then pass in the interface pointer.  This means that at the drop, there's no need to re-get the item from the calendar at all.  http://www.xulplanet.com/tutorials/xultu/dragwrap.html has an example of this using the application/x-moz-file flavour.

Thanks again for the patch!

Comment 9

[Joey Minta]

Attachment: work in progress

Work in progress, posting it because it was a real pain to write it but i'm too tired to mess around further with it tonight.  Basic dragging does work, kinda.  There seems to be a bug in the view-controller where the dates don't get saved properly.  Drop-shadows don't work yet either.  Still, the major obstacle of actually transfering an arbitrary interface has been kinda worked around by grabbing the sourceNode of the drag session and taking the occurrence off of it.  (The specialized flavorprovider *might* also work, but I'd be surprised.)

Comment 10

[Joey Minta]

Attachment: sourceNode workaround, but ready otherwise

Patch implements drag and drop for the month (and multiweek) view.  It handles events spanning multiple days gracefully.  It should also handle tasks, once the calIViewControllers are updated.  (This wants to wait until they are unified in bug 337941.)

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 226188 [details] [diff] [review]
sourceNode workaround, but ready otherwise

>+      <property name="parentBox">
>+        <getter><![CDATA[
>+          return this.mParentBox;
>+        ]]></getter>
>+        <setter><![CDATA[
>+          this.mParentBox = val;
>+        ]]></setter>
>+      </property>

Skip the CDATA noise and just use getter="" and setter="" for the one-liners?

>+        if (item instanceof Components.interfaces.calIEvent) {
>+          transfer.addDataFlavor("application/x-moz-cal-event");
>+        } else if (item instanceof Components.interfaces.calITodo) {
>+          transfer.addDataFlavor("application/x-moz-cal-task");
>+        }

Can an item not be both event and todo?  I guess not, and you'll have to
spend weeks chasing those forced dichotomy cases anyway, should that change.

The faked-up MIME types should have "vnd" in it somewhere, I think.  If they're
in use in other places -- suspect so -- then please file on fixing it.

>+        var flavorProvider = {

Ahem: "flavour".

("Lightning: it's gonna provide you with _flava_")

>+      <!-- While you might expected 'dragexit' to be fired when we drag outside

"expect".

>+         - the node in question, this isn't actually the case.  So, we need 
>+         - to keep track of these shadows on the month-view itself, so they can
>+         - be properly removed

Sentences end in full stops, young man.

>+           newDate.year = boxDate.year;
>+           return newDate;
>+         }
>+         if (!session.sourceNode || !session.sourceNode.occurrence) {
>+             return;
>+         }

Vertical


whitespace




is sacred.

>+         if (item instanceof Components.interfaces.calIEvent) {
>+           var duration = item.duration;
>+           newStart = adjustDate(item.startDate);
>+           newEnd = newStart.clone();
>+           newEnd.addDuration(duration);
>+         } else {
>+           if (item.entryDate && item.dueDate) {
>+             var duration = item.duration;
>+             newStart = adjustDate(item.entryDate);
>+             newEnd = newStart.clone();
>+             newEnd.addDuration(duration);
>+           } else if (item.entryDate) {
>+             newStart = adjustDate(item.entryDate);
>+           } else { // only due date
>+             newEnd = adjustDate(item.dueDate);
>+           }
>+         }

You else-if to check explicitly for todos elsewhere, should probably do it
here as well.  (VJOURNAL, here we come!)

r=shaver if you spit and polish.

Comment 12

[Joey Minta]

Patch checked in.  Can we get QA to verify this?

Comment 13

[Damian Szczepanik]

verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060827 Calendar/0.3a2+