erroneous drag'n'drop with tasks in multiweek/month view

VERIFIED FIXED in 0.8

Status

defect
VERIFIED FIXED
12 years ago
6 years ago

People

(Reporter: sebo.moz, Assigned: jgrotepass)

Tracking

unspecified
Dependency tree / graph

Details

Attachments

(7 attachments, 7 obsolete attachments)

3.89 KB, patch
Details | Diff | Splinter Review
6.63 KB, text/plain
Details
799 bytes, text/plain
Details
799 bytes, text/plain
Details
779 bytes, text/plain
Details
795 bytes, text/plain
Details
5.25 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
Tasks that have only a start date set, are moved to a wrong day in special cases.

Steps to reproduce:
1. create task on 2008/02/07 22:00 (don't set a due date)
2. move to month view
3. show tasks in view
4. move task to 2008/01/31

result:
task is moved to 2008/01/02

expected:
should move to 2008/01/31

If dragged to 2008/01/30, the task is moved to 2008/01/01

All other date targets seem to be working correct.
ssitter confirmed this as a regression from 0.7 on IRC.
Flags: blocking-calendar0.8?
I tested again in Sunbird 0.7 and it's not a regression. Same behaviour in 0.7
Keywords: regression
Upon retest it fails in 0.7 too.

Problem seems related to the drag target date and the number of days in the drag origin month. The drag fails if target date > number of days in origin month.

Example: Feb-2008 has 29 days: dragging task from Feb-2008 to 01/29-Jan-2008 works. Dragging to 30/31-Jan-2008 fails.

Nov-2007 has 30 days: dragging task from Nov-2007 to 01/30-Oct-2007 works. Dragging to 31-Oct-2007 fails.

Sounds like some kind of 'overflow' in the day and month calculation.
The same issue happens when dragging the task into the future, e.g. dragging the task from 29-Feb-2008 to 30-Mar-2008 will move the task to 01-Mar-2008.
Looking into this I found that the calendar-month-view.xml contains a function "adjustDate". This is called when dropping the Task. 
I not have yet any idea how to trace that xml/js stuff. But there seems to be something wrong in this specific logic. Especially because only Tasks with No Duedate or only End-Dates are calling this function.

Jochen
This issue is really generated inside the adjustDate Function in calendar-month-view.xml. 
The statement newDate.day = boxDate.day seems to produce a weird result.
boxDate is correct filled. boxDate.day contains 31. After this statement, newDate.day however contains "2" if the drop is on 31-Jan-2008.
The only variable where the "2" is defined is in boxDate.endOfWeek.day.

Continue to research on this.
Jochen
OK, seems to find a solution. 
Instead of differentiate between Tasks with duedate and no-duedates, I modified the script to use the same logic for the start date and - if exist - the duedate.
In my opinion, there are some instable variants in the adjustDate logic. I was not able to figure out what it was. So adapting the already working logic made more sense to me.
One thing I realised during my tests:
When I have a task - as described here - without a duedate and the default time left at 00:00 and I move this to - see comment #3 - 31-March-2008 it falls back to 30-March 2008 after this patch. Seemed odd in the first moment.
Reason for this is the switch to Daylight Saving on 31-March. The logic for calculating the offset is then using only 23 hours which moved the task back one day. If there is a time placed in the task, this does not happen - it is just moved an hour back. 
I leave this as it is - even if it looks strange to me -.

Lets see, what the tests brings from others.

Jochen
Proposed patch
Attachment #302153 - Flags: review?
Did you try to move the lines 

>     if (duration.hours == 23) {
>         // entering DST
>         duration.hours++;
>     } else if (duration.hours == 1) {
>         // leaving DST
>         duration.hours--;
>     }

before the line

>     if (isEvent(item)) {   

so that both events and todos would benefit from the DT/DST correction?

Why not kill the function adjustDate() completely out of the code?

Hb, 
I made the modification to use the STANDARD/DST correction also in Todos now. Minor modification to the logic because it seems that addDuration is a bit instable on Information like 30D24H which is the result of the current logic.

Why not killing the adjustDate Function completely? Even if I checked the whole package if it used somewhere else, I am still not sure if there is any reference to it.
This could be done easily with some cleanups later..

Jochen
Modified patch - due to comment of HB - move the DST / STANDARD correction logic also for the ToDos. Slightly modified the logic to work correctly within DST correction.
Attachment #302153 - Attachment is obsolete: true
Attachment #302169 - Flags: review?(philipp)
Attachment #302153 - Flags: review?
well, a hard learning curve ...
Also removed the "adjustDate" Function - thanks HB. It is only used here.
Also found the way to create propper patches.
Posted patch removed adjustDate (obsolete) β€” β€” Splinter Review
Sorry for another patch. Now it seem to be in the propper format
Attachment #302169 - Attachment is obsolete: true
Attachment #302183 - Flags: review?(philipp)
Attachment #302169 - Flags: review?(philipp)
(In reply to comment #5)
> The statement newDate.day = boxDate.day seems to produce a weird result.
> boxDate is correct filled. boxDate.day contains 31. After this statement,
> newDate.day however contains "2" if the drop is on 31-Jan-2008.

Maybe this hint is far too late, but:
Did you try to simply swap the lines in order, to assign the correct year first, then month and last day?
It seems logical, that "31" is converted to "2", when newDate.month contains "2" - but I did not check. Maybe even newDate.month is modified to "3" then?

But if you have another working solution, don't care about my comment...
Jochen, would you be terribly upset if we checked this patch in after 0.8? We are already past our schedule w.r.t the 0.8 release schedule and for the sake of keeping the risk of regressions low it would be good if we could get the blocking-calendar0.8+ list down to zero before taking care of any blocking-calendar0.8? or other bugs.

Please don't get me wrong, we are very happy to have more developers contributing to the project, but we need to stay focused. I can review your patch before the release, but if you agree it would be great to check it in after 0.8.

Sven's comment should work out. This is due to the way jsDate works. If you set anything in a jsDate, the date is normalized. This way with:

2008-02-01

setting .day to 31 produces

2008-02-31

which automatically normalizes to

2008-03-02
Philipp, no problem at all. I thought during QA-Test that this issue could be fixed easily.
The idea of Sven make sense. My confusion is only when you set the day twice, than the day is inserted correctly. 
Anyhow, the other implementation is more streamlined in terms of code clearness (imho).
Jochen
(In reply to comment #15)
> The idea of Sven make sense. My confusion is only when you set the day twice,
> than the day is inserted correctly. 

Well, this only underlines my assumption:

[first run]
2008-02-01, day   = 31   --> 2008-02-31 --> 2008-03-02
2008-03-02, month = 01   --> 2008-01-02
2008-01-02, year  = 2008 --> 2008-01-02

[second run]
2008-01-02, day   = 31   --> 2008-01-31
2008-01-31, month = 01   --> 2008-01-31
2008-01-31, year  = 2008 --> 2008-01-31


and so the reverse order would also work in one run:
2008-02-01, year  = 2008 --> 2008-02-01
2008-02-01, month = 01   --> 2008-01-01
2008-01-01, day   = 31   --> 2008-01-31

(to set the year first would be necessary for leap years)
Oh, I forget one thing that has to be tested as well - to move an event from 2008-01-31 to 2008-02-01...

2008-01-31, year  = 2008 --> 2008-01-31
2008-01-31, month = 02   --> 2008-02-31 ?    or 2008-03-02 ?

Then always two runs would be required in this "workaround"...
This enables moving of tasks in every direction, but we should definitely focus on Jochen's patch after 0.8 is out.
Attachment #302774 - Flags: review?(philipp)
With the "debugger;" statement in the file and venkman I got some more insights  to this bug:

1. Jochens patch works. It considers movements over the DT/DST border correctly. 

2. I did not test Svens patch. I'm afraid that it does not consider the DT/DST border and will change the hour values if tasks are dragged over it.

3. The timezoneOffset propertys of the boxDates (3600 for MEZ and 7200 for MESZ) are calculated only for each row (= week) instead of each day in the month and multi week views. Daylight saving time starts on Sunday, 30. March 2008 at 02:00. This box has a boxDate with timezoneOffset = 3600. This behaviour does no harm. Dragging a task from Saturday, 29. 12:00 to Sunday 30. simply doesn't cross the DT/DST border. Dragging it in the next week will do.
(In reply to comment #19)
> 2. I did not test Svens patch. I'm afraid that it does not consider the DT/DST
> border and will change the hour values if tasks are dragged over it.

I think I have to clearify this:
In response to comment #14, I just wanted to provide a patch that makes it possible to use DnD in 0.8! I don't know, what behavoiur you expect - but I checked, that only the date is being modified with my patch.
So if I drag a task from 2008-03-29 to 2008-03-30, the time remains... Although I had general issues with times 02:00 and 03:00 on this Sunday, but that's another issue.
I don't see a need for using two ways to handle datetimes in dragged tasks. Currently these ways differ depending on the existence of the due date. The function adjustDate() is obsolete. With "boxDate" and "beginMove" we have two good sources to calculate the number of days to move the item.

>  var offset = this.monthView.mShadowIndex;
mShadowIndex gives a numerical string to show on which day a multi day item is gripped. This string was firstly used as solution for bug 357112. But currently that former solution gives a wrong result too when moving multi day events.

My favoured way for moving items is to calculate a duration and correct it if necessary for DT/DST or UI issues. This duration should then be applied only once to the items. Other ways like applying the move first, applying the DT/DST correction secondly and thirdly applying some UI drop shadow offsets will lead to side effects and unmanageable code. See bug 372839, bug 367163 and bug 354633 for additional information on this.

(In reply to comment #20)
> ... but I checked, that only the date is being modified with my patch.

Sorry for rating other patches. The situation after applying your patch which modifies only the adjustDate() sub function is:
a) Dragging tasks without due dates will work properly. Times remain save even if dragged over a DT/DST border.
b) Dragging tasks with due dates over a DT/DST border will change their times by an hour.

> I had general issues with times 02:00 and 03:00 on this Sunday, but that's
> another issue.

A way for this is setting the datetimes for this period in UTC, see bug 410931 to make this possible in UI.
Assignee: nobody → hb
Status: NEW → ASSIGNED
Attachment #302874 - Flags: review?
Attachment #302874 - Flags: review? → review?(philipp)
Blocks: 357112
OS: Windows XP → All
Hardware: PC → All
(In reply to comment #21)
> Sorry for rating other patches. The situation after applying your patch which
> modifies only the adjustDate() sub function is:
> a) Dragging tasks without due dates will work properly. Times remain save even
> if dragged over a DT/DST border.
> b) Dragging tasks with due dates over a DT/DST border will change their times
> by an hour.

You're totally right - and we don't need to discuss about checking-in half-baked solututions... I'm no friend of it at all, but I tried to provide a solution that is easily reviewed and brings a higher usability for 0.8 (according comment #14).
I also have to add that I only looked at these 3 lines, without the "whole view", when and how it is used...

But to support Philipp's review:
Your patch looks very similiar to Jochen's, with the advantage of removing unnecessary code as well (sorry Jochen).
What do you think (or may Jochen explain this) about his comment:

-           duration.hours++;
+           // seems to work better with Tasks than duration.hours++
+           duration.hours -= 23;
+           duration.days++;


@Philipp:
Apart from all the work that has to be done before 0.8 release, it should be considered one way, at least because of "blocking-calendar0.8?":

1. Leave everthing as is (bad UI behaviour because of "jumping" items in month view)

2. Use my (or any other) quick & dirty workaround (with the need to be cleaned up later)

3. Do the complete review for Hb's or Jochen's patch. I don't expect too many regressions, as this only uses approved code from Events for ToDos.
(In reply to comment #22)
> What do you think (or may Jochen explain this) about his comment:
> 
> -           duration.hours++;
> +           // seems to work better with Tasks than duration.hours++
> +           duration.hours -= 23;
> +           duration.days++;
> 
> 
Sven,
there are always millions ways to code and fix issues. I am not the Javascript Guru at all - just simple 25+ years of programming experience in nearly all languages on nearly all platforms (Basically I was a Mainframe System Programmer with IBM Assembler on MVS/OS390/zOS). So please appologize if this "normalization" stuff in Javascript slipped out of my view.

Now about my comment in the above snippet:
The old code simply added one hour to the object duration.hours. This ended in a wrong calculation of the distance. The object "duration.days" had a value (i.E. 13), the duration.hours had (after the increament) 24, the duration.icalString had "P13DT24H", the duration.seconds were correct.
I found that the Function "addDuration" simply did the calculation wrong. So modifying the calculation by decrease of 23 in hours and adding 1 day leaving the duration.icalString to the correct value of "P14D", the duration.days to "14" and the duration.hours with 0. 
Because I have learned to always live with - more or less stupid - workarounds will get issues solved far ways faster, I tended to use this approach. 

Finally my view to this discussion is pretty simple. As long as the bug is fixed, I really don't care much how and who provided the patch. We could start to discuss code clearness but I expect a more religous discussion than really better code. Neither of us is able to write Lightning from the start new and make everybody happy. 

I was on the QA-Channel during the last qa-test (it was my first occurence there) and found this a simple bug to fix the next morning. I also got the impression, that this bug might block 0.8 - which is more worse that writing perfect code (which will never exist - btw).

Cheers
Jochen
(In reply to comment #22)
> Your patch looks very similiar to Jochen's, with the advantage of removing
> unnecessary code as well (sorry Jochen).
> What do you think (or may Jochen explain this) about his comment:
> -           duration.hours++;
> +           // seems to work better with Tasks than duration.hours++
> +           duration.hours -= 23;
> +           duration.days++;

Jochen noticed some miracles when using "duration.hours++" for this. I did not notice unwanted behaviour from this in UI, but didn't traced it in the debugger. Jochens observations may appear for tasks only, because other files [1] to this calculation similar and have no problems. In doubt we should choose the safe way and take Jochens three lines here instead of "duration.hours++". An error in addDuration() is a big problem.
 
> @Philipp:
> Apart from all the work that has to be done before 0.8 release, it should be
> considered one way, at least because of "blocking-calendar0.8?":
> ...
> 3. Do the complete review for Hb's or Jochen's patch. I don't expect too many
> regressions, as this only uses approved code from Events for ToDos.

As newbie I don't see any regressions. The codes apply a behaviour which is now used for events only to tasks with and without due dates. My patch additionally fixes bug 357112. Please try and move events which last three days or so.


[1]
http://mxr.mozilla.org/mozilla/source/calendar/base/content/calendar-multiday-view.xml#1489
Jochen, are you sure that addDuration() works wrong?


After trying some manipulation with the duration I see the confusing behaviour you described too in the icalString. But the property inSeconds has always the correct duration. 

Please don't overweight the ical string. Having an icalString like "P6DTH24" and applying duration.inSeconds += 0; changes the string to "P1W".

Hb, I was sure - just tested the logic again and found that it now "seems" to work correct. I like those kind of miracles in programming languages that creates different results for some reason in different tests ;-)

Ok, leave the duration.hours++

Jochen
I think what you are observing here is auto normalization of durations. P6DTH24 is P1W, nothing strange about that.
Jochen, if you are sure you are experiencing wrong calculations with addDuration() then please create a test scenario and file that. Anything else (especially workarounds) don't help us, but makes the code base unmaintainable.
During my last revisions of the calDatetime object (especially removing the need for manual normalization and the modified timezone handling), I've removed quite lots of those hacks. So after all, if you figure out something's not documented enough resp. leaves space for interpretation, then please file bugs for that (and CC me if you like). Thanks!
Comment on attachment 302774 [details] [diff] [review]
Quick hack to work-around this problem (without running twice).

Don't like quick fixes - they make me sick (quote Beatstakes)

I'll see if I can review one of the other solutions soon :)
Attachment #302774 - Flags: review?(philipp) → review-
Comment on attachment 302774 [details] [diff] [review]
Quick hack to work-around this problem (without running twice).

(In reply to comment #28)
> (From update of attachment 302774 [details] [diff] [review])
> Don't like quick fixes - they make me sick (quote Beatstakes)

I do totally agree!!!
Attachment #302774 - Attachment is obsolete: true
Attachment #302183 - Attachment is obsolete: true
Attachment #302874 - Attachment is obsolete: true
Attachment #303233 - Flags: review?(jgrotepass)
Attachment #302183 - Flags: review?(philipp)
Attachment #302874 - Flags: review?(philipp)
Comment on attachment 302183 [details] [diff] [review]
removed adjustDate 

Due to the little time left for 0.8, I decided to post my review comments as a next-version patch. Jochen, since this is your bug could you take a look and see if everything is as you wanted it to be?

>+         if (duration.hours == 23) {
>+           // entering DST
>+           // seems to work better with Tasks than duration.hours++
>+           duration.hours -= 23;
>+           duration.days++;
duration.hours++ seems to works fine for me. In any case, it *should* work. If it does not, it should be fixed elsewhere.

>          if (newStart && offset) {
>            newStart.day -= offset;
>          }
>          if (newEnd && offset) {
>            newEnd.day -= offset;
>          }
Removed, since its not needed.

Also, the indentation of that function is now corrected.


What I forgot to put in the patch but would like to fix before checkin is to get rid of an exception while dragging:

@@ -601,19 +601,21 @@
       <handler event="dblclick"><![CDATA[
         event.stopPropagation();
         this.monthView.controller.createNewEvent();
       ]]></handler>
       <handler event="dragenter"><![CDATA[
         var dragService = Components.classes["@mozilla.org/widget/dragservice;1"].
                           getService(Components.interfaces.nsIDragService);
         var session = dragService.getCurrentSession();
-        session.canDrop = true;
-        this.removeDropShadows();
-        this.addDropShadows();
+        if (session) {
+            session.canDrop = true;
+            this.removeDropShadows();
+            this.addDropShadows();
+        }
       ]]></handler>
 
       <handler event="dragover"><![CDATA[
         var dragService = Components.classes["@mozilla.org/widget/dragservice;1"].
                           getService(Components.interfaces.nsIDragService);
         var session = dragService.getCurrentSession();
         session.canDrop = true;
       ]]></handler>
(In reply to comment #31)
> >          if (newStart && offset) {
> >            newStart.day -= offset;
> >          }
> >          if (newEnd && offset) {
> >            newEnd.day -= offset;
> >          }
> Removed, since its not needed.

Please remember to also remove the unneeded variable declaration before check-in, pointed out by Hb:

>          var offset = this.monthView.mShadowIndex;
Philipp, I tested the modification - with the extension in Comment #31.
It seems, as if today is a day where the "normalization" that I saw during my first tests in the DST adjustment create the "old" behavior - means  it is not working correct.
As Daniel requested in Comment #27 I tested this very extensively and see the problem again.

I created a Task without due-date on 28-Feb-2008. Than I dragged that to 1-Apr-2008. The Task was dropped on 31-March-2008.
The attached file shows the details of the different fields. Maybe this helps somebody to find the bug behind that.

I really have no idea why this sometimes work and the next day not. 

Jochen
Start of Testcase for issues with DST drag'n-Drop of Tasks.
This is a fresh created Task - no duedate - in an CalDav Calendar (DAViCal) -.

Part 1 of 4 ....
The Task after Draging it to 1-Apr-2008 (!).
 Part 2 of 4
A Task in a local Calendar created on 29-Feb-2008 - no duedate.

Part 3 of 4
The Task from the local Calendar after dragging it to 1-Apr-2008.

Part 4 of 4

To me it seems as if there are differences in the DTSTART. Also in the X-MOZ-GENERATION Tag. 

I guess that there are different handlers for the calculation - or the results are handled differently.

Jochen
Assignee: hb → jgrotepass
Status: ASSIGNED → NEW
Philipp, I tested the modification and found - except the AllDay issue - no ther issues. Also I implemented your proposed modification on the session issue.

This one now should - hopefully - be ok.
Jochen
Attachment #303323 - Flags: review?(philipp)
Comment on attachment 303323 [details] [diff] [review]
Patch Combination from Philipp, Hb and - still - some pieces from me

r=philipp Looks good. I'll check in this evening or tomorrow.
Attachment #303323 - Flags: review?(philipp) → review+
I just found bug 416322 which addresses the "AllDay" issue that I have. With Hb we  found that this only occurs under Lightning.
So this one here is really solved for me.
Jochen
Philipp, could you please hold this for a moment. Looking at Bug 416322 shows the same issue with the all-day event/task issue. When I modify the duration in the case that DST/DT border is crossed with either "duration.normalize()" or "duration.inSeconds +=0" (thanks Hb) both issues are fixed.

Jochen
OK, after getting the information that this would be another workaround, I the patch is OK.
Not going to block on this, but we'll of course take the patch.
Flags: blocking-calendar0.8? → blocking-calendar0.8-
Sorry Philipp for the confusion. Talking on IRC with mvl, hb and ctalbert we agreed to get the duration.normalized into the code to fix 416322. 
it seems that duration is not (yet) auto-normalized.

Jochen
Attachment #303323 - Attachment is obsolete: true
Attachment #303372 - Flags: review?(philipp)
Made some minor comment fixes to clearify DT/DST - DST/DT logic.
Attachment #303372 - Attachment is obsolete: true
Attachment #303377 - Flags: review?(philipp)
Attachment #303372 - Flags: review?(philipp)
Blocks: 416322
Comment on attachment 303377 [details] [diff] [review]
minor comment changes finally - fixes 416322 too

r=philipp
Attachment #303377 - Flags: review?(philipp) → review+
Attachment #303233 - Flags: review?(jgrotepass)
Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Flags: in-litmus?
Verfied with Lightning 2008022620 on Windows XP.
Status: RESOLVED → VERIFIED
Flags: blocking-calendar0.8-
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.