357112 - Drag and drop of multiday-event doesn't drop on days the shadow suggests

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Omar Bajraszewski]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv:1.8.1) Gecko/20061010 Firefox/2.0

Drag and drop of tasks/events doesn't work correctly.

Reproducible: Always

Steps to Reproduce:
1.Create an event/task that lasts for example a week (say from 16/10/2006 to 22/10/2006)
2.click on 2nd, 3rd,...,7th day of event and drag it to other week
3.Before releasing the mouse button notice what you see
4.Release the button

Actual Results:  
The'shadow' doesn't cover the result of the operation

Expected Results:  
When you click for example on 2nd day of task/event and drag it to next week you expect it will be a second day of the task/event not first.

Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv:1.9a1) Gecko/20061006 Sunbird/0.3

Comment 1

[Martin Schröder [:mschroeder]]

Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061027 Calendar/0.4a1.

Comment 2

[Omar Bajraszewski]

Shouldn't it be fixed before releasing calendar 0.5?

Comment 3

[Joey Minta]

Attachment: [checked in] include offset in calculation

For the life of me, I still can't understand the problem or the fix in bug 354633.  However, assuming that fix really is needed to fix some bug, this patch keeps it around and still includes the offset necessary for the calculations.

Comment 4

[cmtalbert]

Comment on attachment 248039 [details] [diff] [review]
[checked in] include offset in calculation

Reviewed and tested on windows. This solves the problem. The reason it works is it takes into account the offset of what the user clicked on in relation to the start/end time of the event. Looks good. ctalbert=r+

Comment 5

[Matthew (lilmatt) Willis]

jminta:
Does this just need checkin?

Comment 6

[Matthew (lilmatt) Willis]

jminta said in IRC that he thought it just needed checkin.

Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Comment 7

[Omar Bajraszewski]

Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3pre) Gecko/20070321 Calendar/0.5pre

Comment 8

[Omar Bajraszewski]

Problem is again present with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7pre) Gecko/20070831 Calendar/0.7pre and lightning 2007083103----> reopening

Comment 9

[Stefan Sitter [:ssitter]]

(In reply to comment #8)
I really wish people would file new bugs for new regressions and just refer to the old bugs instead of reopening the old bugs that were fixed and verified for previous releases.

Comment 10

[Omar Bajraszewski]

(In reply to comment #9)
> I really wish people would file new bugs for new regressions and just refer to
> the old bugs instead of reopening the old bugs that were fixed and verified for
> previous releases.
> 

That is why I asked on chat what I should do- I was told to reopen the bug. If I did wrong, sorry for that. Feel free to close it and open a new one for me, please.

Comment 11

[Simon Paquet [:sipaq]]

Setting wanted0.8- as the main Calendar developers will not devote any time to
this in the 0.8 timeframe. Patches are, of course, always welcome.

Comment 12

[Decathlon]

Attachment: Fix for Calendar 1.0 (again include offset in calculation)

This patch adds the code to figure out the offset of the dragged element for spanning days events and tasks in month-view and all-day events in week-view.

Instead of 'parentBox', the property 'parentNode' could be used, in this case it doesn't need to add the property inside binding calendar-header-container. Please tell me what's better.

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 397520 [details] [diff] [review]
Fix for Calendar 1.0 (again include offset in calculation)

>+         item = session.sourceNode.sourceObject;
Missing let.


Otherwise the code looks fine. Not sure how to best handle the parentNode/Box stuff. This makes the box depend on its parent. Maybe we can just pass the date instead?

Comment 14

[Decathlon]

(In reply to comment #13)
> (From update of attachment 397520 [details] [diff] [review])
> >+         item = session.sourceNode.sourceObject;
> Missing let.
>

original code defines the 'item' variable twice (see lines 450 and 469 in calendar-widgets.xml file), so I deleted the second 'let'.
http://mxr.mozilla.org/comm-central/source/calendar/base/content/widgets/calendar-widgets.xml#450ntral/source/calendar/base/content/widgets/calendar-widgets.xml#450

> 
> Otherwise the code looks fine. Not sure how to best handle the parentNode/Box
> stuff. This makes the box depend on its parent. Maybe we can just pass the 
> date instead?

I have problems in doing this in different ways, i.e. I haven't found another easy way to get the date of the starting point of the drag session other than the date of the box of the dragged element (session.sourceNode).
Is there a simple way instead?

Actually the property 'parentBox' should belong to event boxes instead of their box-container like 'calendar-header-container'.
I had thought to move the parentBox property inside 'calendar-editable-item' binding to make that property available for events boxes in month-view AND for all-day ones in week-view (because now event boxes for all-day events in the week-view are added with createXulElement() function and don't have a binding like 'calendar-month-day-box-item' where to add a property 'parentBox'). But I don't know if this would be a mistake i.e. 'calendar-editable-item' binding is a bad place for a 'parentBox' property (otherwise I've already tried a working patch in this way).

Comment 15

[Martin Schröder [:mschroeder]]

Comment on attachment 397520 [details] [diff] [review]
Fix for Calendar 1.0 (again include offset in calculation)

Decathlon, what is the state of this patch? It has review+, but it seems there are some rough edges Who needs to act next, and how can we help?

Comment 16

[Decathlon]

I'm waiting a review for the patch attached to Bug 476227 because that one would fix this one too (this issue is only a part of that other one).
I had to write a comment here, sorry.

Actually the problem related to the 'parentBox' mentioned in comment #13 is the same in the other patch because I've basically used the same code for the same patch's part.
I've mentioned this problem in bug 476227 comment #2. Maybe after the review there, I will need to ask the same question that I did here in * comment #14 * (I had written a mail to Philipp too, but he is too busy ;-). With a negative answer to this question I will try to do in a different way, but if I remember correctly (over three months have past) I will need some suggestion ;-).
Let's wait for Philipp's review on bug 476227.

Comment 17

[Decathlon]

This bug has been fixed with checkin for bug 476227.
Could someone confirm?

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Confirmed