Closed Bug 416138 Opened 12 years ago Closed 11 years ago

Moving of all-day-events in weekview not possible, but shows possibility

Categories

(Calendar :: Calendar Views, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: thetux.moz, Assigned: berend.cornelius09)

References

Details

Attachments

(2 files, 4 obsolete files)

Steps to Perform:

   1. Switch to week view
   2. Create new event all-day-event
   3. Mouse over created event and press mouse button
   4. Move mouse over next day
   5. Release mouse button

Results:

    * Moving the event not possible

Expected Results:

    * Calendar is in week view mode
    * Event is visible in main view
    * Event is selected and its background color has changed
    * New From and To date are visible
    * Event has new start date and is visible in new day

See also https://litmus.mozilla.org/show_test.cgi?id=2595
Additional info:

Error console is empty.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12pre) Gecko/20080206 Calendar/0.8pre 2008020619
This is _NO_ duplicate of bug 350603. Some changes (maybe the new dnd functionality in Lightning) have enabled dnd of all day events, but it should not be shown as being possible in the calendar view.

This is misleading UI and could result in many bug reports after the release. QA team nominates this bug for blocking-calendar0.8.
Flags: blocking-calendar0.8?
OS: Windows XP → All
Hardware: PC → All
Summary: Moving of all-day-events in weekview not possible → Moving of all-day-events in weekview not possible, but shows possibility
Works for me in Lightning 2008020719 and TB2.0.0.9 on WinXP.

Maybe the DND possibility for all-day events in day view may be misleading, but it's still possible to do Event to Task/Mail conversion through DND.
True, there are many ways to edit the event.  This is something we should fix, but because there are easy workarounds, we are not going to block on this.
Flags: blocking-calendar0.8? → blocking-calendar0.8-
Attached patch Work in Progress - v1 (obsolete) — Splinter Review
First try at fixing this, no idea if this is the right thing to do and I'm sure there is a better way. 

What was never implemented in the dnd listener was the canDrop method, so the generic method from nsDragAndDrop was used. This patch implements canDrop by checking the target element for a canDrop attribute, and evaluating the attribute value as code. 

If the target doesn't have a canDrop attribute, the DOM tree is traversed upwards until an element has the canDrop attribute. This is useful for i.e the today pane, where the canDrop is on the vbox, and it should be possible to drop on anything in the vbox. While this works nicely, I'm a bit sceptical with the traversal, since the whole DOM branch is traversed upwards to the root element, if the element under the cursor isn't something that should be dropped on.

Current users of the canDrop attribute only return true, but theoreticaly we should check if the data from the drag session to see if the data is actually something we can drop on the respective elements.

I'd be glad if someone else could take over on this.
Flags: blocking-calendar0.8- → wanted-calendar0.9?
Flags: wanted-calendar0.9? → wanted-calendar0.9+
Flags: blocking-calendar0.9?
Flags: blocking-calendar0.9? → blocking-calendar0.9-
Blocks: 453052
Taking over...
Assignee: nobody → Berend.Cornelius
Attached patch patch v. #2 (obsolete) — Splinter Review
Attachment #309930 - Attachment is obsolete: true
Attachment #338279 - Flags: review?(philipp)
When I started to work on this issue I noticed that the functionality that I need has actually already been implemented within the monthview, where dragging of eventboxes between day-box-containers is already possible.
This patch adds a new widget capable that may serve as a droptarget and is capable of carrying drop drop shadows similar to the current drag'n drop behaviour in the month-view. The introduced widget should be used for all kind of possible droptargets, where the events have to be adapted to a certain period. It still offers lot of room for improvement but  I hope that it's sufficient for a first step.
Some implementation notes:
- The patch has become quite big  because I consolidated a lot of month-view-code into it and did some other refactoring on the way.
- It only supports drag'ndrop for one single event box like the current implementation also does. Maybe we can improve this for the future and that's why I already keep the dropshadows in an array.
I removed the following code snippet  and could not see any bad side effects... .If necessary I have to reintroduce it.
-I was hesitating to add another two functions to bloated calUtils.js, but looking over that file I found that rather other – more ui-centric – functions should be shifted to calendar-ui-utils than my two functions. I think we should do this in another issue.
>        // Mozilla doesn't provide any generic way for us to listen for the
>        // drag-end.  Add this listener for a best approximation of listening
>        // for that case.
>        var monthbox = this.parentBox;
>         monthbox.calendarView.dropListener = function checkStillDragging() {
>            try {
>                var session = getDragService().getCurrentSession();
>            } catch (ex) {}
>            if (!session) {
>               monthbox.removeDropShadows();
>               monthbox.addItem(item);
>            }
>        }; 
>       window.addEventListener("mouseover",
>                                monthbox.calendarView.dropListener, true);

- I currently only took care that all day events may be dragged and drop from all-day-containers in the week view. 
This patch is certainly not relevant for the 0.9 release!
Status: NEW → ASSIGNED
Comment on attachment 338279 [details] [diff] [review]
patch v. #2

While testing this patch I noticed that if you mouse over the shadows, the shadows flicker slowly. I couldn't reproduce bug 343190 which introduced the code snippet you posted, so I think is safe to remove that code for now. What I did notice though, that if you move around the item then the shadows get removed though and don't come back until you go to the next box. May be a side effect of flickering though.



>-        var action = dragService.DRAGDROP_ACTION_MOVE;
>+        var action = getDragService().DRAGDROP_ACTION_MOVE;
You don't need the whole drag service for this, go ahead and use
Components.interfaces.nsIDragService.DRAGDROP_ACTION_MOVE.

>         // Mozilla doesn't provide any generic way for us to listen for the
>         // drag-end.  Add this listener for a best approximation of listening
>         // for that case.
>         var monthbox = this.parentBox;
If you are removing the code with the drag-end, then please also remove the above comment.

>+            dateBoxes[0].showMonthLabel = true;
>+            dateBoxes[dateBoxes.length-1].showMonthLabel = true;
While you are at it, spaces around operator.

>+      <method name="onDragItem">
>+        <parameter name="aItem"/>
>+        <body><![CDATA[
>+         var newItem =moveItem(aItem, this.mDate);
Missing space

>+dragndropContainer {
>+  -moz-binding: url(chrome://calendar/content/widgets/calendar-widgets.xml#dragndropContainer);
>+}
Is this container used on its own? if not, then afaik no css rule is needed, its possible to extend other bindings without it.

>+      <method name="onDragItem">
>+        <parameter name="aItem"/>
>+        <body><![CDATA[
>+            // method that may be overwritten by derived bindings...
>+        ]]></body>
>+      </method>
May it be, or *must* it be overridden? If the latter, then I'd suggest throwing NS_ERROR_NOT_IMPLEMENTED there. Also please correct the typo (overwritten vs overridden). In any case, it would be nice to add some documentation what exactly a derived binding should do in this method, or in which cases it is invoked.

>+#ifdef XP_UNIX
>+                      var self = this;
>+                      setTimeout(function addDropShadows() { self.addDropShadows(); }, 5);
>+#else
>+                      this.addDropShadows();
>+#endif
What was the reason for this workaround? It would be nice to get rid of it if possible ;-)

>+          return XULElement.prototype.setAttribute.call (this, aAttr, aVal);
Remove space after |call|
>   /**
>+   * replaces an occurrence.
>+   * @param aOldOccurrence      The occurrence to be replaced
>+   * @param aNewOccurrence      The occurrence bound to replace 'aOldOccurrence'
>+   */
>+void replaceOccurrence (in calIItemBase aOldOccurrence,
>+                         in calIItemBase aNewOccurrence);
>+
>+  /**
>    * Modify aOccurrence.  If aNewStartTime and aNewEndTime are given,
>    * update the event to those times.  If aNewTitle is given, modify the title
>    * of the item.  If no parameters are given, ask the user to modify.
>    */
>   void modifyOccurrence (in calIItemBase aOccurrence,
It would be nice if you could make more clear what the difference between replaceOccurrence and modifyOccurrence is in this documentation.



r- for now since the flickering needs to go away, but I'll gladly r+ a patch that doesn't flicker ;-)
Attachment #338279 - Flags: review?(philipp) → review-
Flags: wanted-calendar1.0+
Flags: wanted-calendar0.9+
Flags: blocking-calendar0.9-
As Martin mentioned in another bug we have a new Drag'n Drop API described in
http://developer.mozilla.org/en/DragDrop/Drag_and_Drop

As I noticed there is now a "dragleave" handler instead of a "dragexit" hanbdler provided. This could be helpful to resolve the remaining problems
Attached patch patch v. #3 (obsolete) — Splinter Review
I worked in the issues from Philipp's comment #9 and also found away to avoid the flickering that was caused by the transition of the mouse pointer over the internal anonymous elements. I added a lot of consolidation and tried to make the code of the calendar-month-view a bit more understandable.
Attachment #338279 - Attachment is obsolete: true
Attachment #349956 - Flags: review?(philipp)
Attached patch patch v. #4 (obsolete) — Splinter Review
I found out that drag'n drop in the multiday view was also related to my change and therefor regressed. Fixed and consolidated that.
Attachment #349980 - Flags: review?(philipp)
Attachment #349956 - Attachment is obsolete: true
Attachment #349956 - Flags: review?(philipp)
I found a problem:

* Drag event into calendar list, unifinder, or outside of window (i.e gnome-terminal) and drop it.

Actual:
* Dropshadows still exists

Expected:
* Dropshadows should go away.
Comment on attachment 349980 [details] [diff] [review]
patch v. #4

>+    let serializer = Components.classes["@mozilla.org/calendar/ics-serializer;1"].
>+                                createInstance(Components.interfaces.calIIcsSerializer);
Wrap dot to next line here and elsewhere

>+    transfer.setTransferData("text/calendar", supportsString, supportsString.data.length*2);
>+    transfer.setTransferData("text/unicode", supportsString, supportsString.data.length*2);
Spaces around operator


>+          var icon = document.getAnonymousElementByAttribute(this, "anonid", "item-icon");
use let instead of var

>             var monthName = calGetString("dateFormat", "month." + (aDate.month+1) + ".Mmm");
Space around operator and use of let, while you are here.

>-            for each (var itemData in box.box.mItemData) {
>+            for each (var itemData in box.mItemData) {
let

>+// We may run into certain racing conditions under Unix that may lead to the freezing
>+// of a complete session. That's why we have to set a timeout for this platform.
Do we still have this problem? If so, I'd actually go for just doing this on a timeout anyway and get rid of the ifdef's.

>+    moveItem: function cal_moveItem(aOldItem, aNewDate) {
>+         let newItem = aOldItem.clone();
>+         let start = (aOldItem[calGetStartDateProp(aOldItem)] || aOldItem[calGetEndDateProp(aOldItem)]).clone();
>+         let isDate = start.isDate;
>+         start.resetTo(aNewDate.year, aNewDate.month, aNewDate.day,
>+                       start.hour, start.minute, start.second,
>+                       start.timezone);
>+        start.isDate = isDate;
>+        if (newItem[calGetStartDateProp(newItem)]) {
Indentation in this function seems a bit mixed.

>diff --git a/calendar/base/public/calICalendarView.idl b/calendar/base/public/calICalendarView.idl
>--- a/calendar/base/public/calICalendarView.idl
>+++ b/calendar/base/public/calICalendarView.idl
>@@ -157,4 +157,10 @@ interface calICalendarView : nsISupports
>    * Setting this does not refresh the view.
>    */
>   attribute AUTF8String timezone;
>+
>+  /**
>+   * removes the dropshadows that are inserted into childelements during a
>+   * drag and drop session
>+   */
>+  void removeDropShadows();
> };
Do we really need this in the interface? Sounds like an internal detail to me...


>-box[dropbox="true"] {
>+box[dropshadow="true"] {
Hmm, can't we rather use a .dropshadow-box selector along with the corresponding class? This could minimally improve performance.


r=philipp with comments considered.
Attachment #349980 - Flags: review?(philipp) → review+
I worked in the issues as commented by Philipp into the attached patch.
Some notes:

>Do we still have this problem? If so, I'd actually go for just doing this on a
>timeout anyway and get rid of the ifdef's.

You seem to be damn right with your assumption. I could not reproduce this bug anymore. But as this is not a final proof and as that bug was so serious I want to deal with it in a follow-up issue that should then be tested by several users/testers or developers. I also like your idea to apply the timeout on all platforms if it should still be necessary for Unix.

>Hmm, can't we rather use a .dropshadow-box selector along with the
>corresponding class? This could minimally improve performance.

I changed the implementation and use the class attribute now...

>Do we really need this in the interface? Sounds like an internal detail to
>me...
In this case I think we need this in the interface simply because the method is called from outside the class.

pushed that attached patch to comm-central

http://hg.mozilla.org/comm-central/rev/f282378ae9e9

->fixed.
Thank you for your careful review!
Attachment #349980 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
I checked thi sfix and found a new minor issue.

STR:
===

- switch to weekview
- drag an all day event to the all day section of the next day
- wait a moment that the drag shadow gets visible
- move and drop the event outside the all day section
- drag shadow is furthermore visible
reopening due to Andreas comment #16.
-At first I will have to remove all dropshadows when the mousepointer is dragged out of the calendar views in a drag session
But how do I deal with allday events that are dragged to the "normal day section" of the day or week view. I see three possibilities:
- raise the Event dialog, before the modified event is dropped.
- keep the all day flag of the event and just adjust the startDate and endDate. The modifed event would then again appear in the allday section of the calendar-view
- not allow this at all because that section is not reserved for all day events.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
It seems there are a couple of issues due to this patch (to verify when the patch is finished).

1) bug 469146 is probably caused by this patch because of onDropItem()
(http://hg.mozilla.org/comm-central/rev/f282378ae9e9#l2.236 for month-view and  http://hg.mozilla.org/comm-central/rev/f282378ae9e9#l3.138 for multiday-view).
In fact newItem.dueDate is always a null value after onDropItem() is called:
  
6.149 +         let newItem = this.onDropItem(item, session).clone();
6.150 +         let newStart = newItem.startDate || newItem.entryDate;
6.151 +         let newEnd = newItem.endDate || newItem.dueDate;

and this seems to cause bug 469246.

2) If you try to drag and drop a spanning days event, when you drop the event it becomes a one day event (his end date/time becomes the same as start date/time). This behavior would seem caused by onDropItem() too.

Moreover, in line 6.135 :
transfer.addDataFlavor("application/x-moz-cal-event"),
is x-moz-cal-event intentional?
> and this seems to cause bug 469246.

...bug 469146
Duplicate of this bug: 453052
Attachment #350759 - Attachment description: patch v. #5 → [checked in] patch v. #5
Inserted a dragend handler to remove all possibly remaining dropshadows. Notice: Dragging all day events into the "main" view is still not possible und ends in an error message. We should deal with this in another bug.
Attachment #358838 - Flags: review?(philipp)
Duplicate of this bug: 379264
Attachment #358838 - Flags: review?(philipp) → review+
Comment on attachment 358838 [details] [diff] [review]
[checked in] patch v. #6

> Inserted a dragend handler to remove all possibly remaining dropshadows.
> Notice: Dragging all day events into the "main" view is still not possible und
> ends in an error message. We should deal with this in another bug.

Are you going to take care of the other bug too? Please at least make sure there is no error message and the drop feedback is set so that it isn't allowed in case you don't have time to fix the other bug.

r=philipp
pushed to comm-central:

http://hg.mozilla.org/comm-central/rev/7ebc039a89cc

Philipp did a final review over the shoulder.

->fixed.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Attachment #358838 - Attachment description: patch v. #6 → [checked in] patch v. #6
Looks like this checkin regressed Bug 476219.
Depends on: 476219
Depends on: 467908
These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.