Last Comment Bug 353070 - drag and drop for tasks do not work if due date is not set
: drag and drop for tasks do not work if due date is not set
Status: VERIFIED FIXED
:
Product: Calendar
Classification: Client Software
Component: Tasks (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Michael Büttner (no reviews TFN)
:
Mentors:
: 357281 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-17 12:56 PDT by Damian Szczepanik
Modified: 2008-04-01 03:15 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (2.71 KB, patch)
2007-03-23 03:41 PDT, Michael Büttner (no reviews TFN)
mvl: first‑review-
Details | Diff | Splinter Review
patch v2 (2.67 KB, patch)
2007-03-29 08:36 PDT, Michael Büttner (no reviews TFN)
mvl: first‑review+
Details | Diff | Splinter Review

Description Damian Szczepanik 2006-09-17 12:56:40 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060917 Calendar/0.3a2+

if task has specified start date and due date it's ok but if due date is not set drag and drop in main view is not supported - task disappears

Reproducible: Always

Steps to Reproduce:
1. switch to multiweek view
2. enable Tasks in View
3. create new task: start date = now, due date is disabled
4. change start date via drag & drop
Actual Results:  
task disappeared

Expected Results:  
task should be visible in new cell

if due date is set drag&drop works fine
Comment 1 Damian Szczepanik 2006-09-17 13:01:34 PDT
dmose: I have verfied two similar bugs for events - should we fix it before 0.3?
Comment 2 Dan Mosedale (:dmose) 2006-09-19 12:52:05 PDT
Does the task re-appear after a refresh?  Or is it gone forever?
Comment 3 Dan Mosedale (:dmose) 2006-09-19 12:55:48 PDT
sipaq tested, and this is, in fact, just a refresh issue.
Comment 4 Joey Minta 2006-09-19 12:58:05 PDT
This ought to be trivial to check for and work out ok.  I bet there's a really helpful error in the js-console too.
Comment 5 Simon Paquet [:sipaq] 2006-09-19 13:15:30 PDT
(In reply to comment #4)
> This ought to be trivial to check for and work out ok.  I bet there's a really
> helpful error in the js-console too.

Error: gXXXEvilHackSavedSelection has no properties
Source File: chrome://calendar/content/calendar.js
Line: 348
Comment 6 Stefan Sitter 2006-09-19 13:35:45 PDT
Using todays Sunbird/Lightning on Windows 2000 I see a different behavior: After Drag'n'Drop of the task I get the Edit Task dialog for this task. Pressing Ok shows the task at the old date-time, pressing Cancel makes the task disappear until view is refreshed. (And no error in console)
Comment 7 Omar Bajraszewski 2006-09-19 15:21:20 PDT
(In reply to comment #0)

> Steps to Reproduce:
> 1. switch to multiweek view
> 2. enable Tasks in View
> 3. create new task: start date = now, due date is disabled
> 4. change start date via drag & drop
> Actual Results:  
> task disappeared

I'm using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060918 Calendar/0.3a2+
and I can't see any error in console. But after 4th step I changed view to Month then back to multiweek and the task appeared untouched- I want to say that changing start date via drag&drop doesn't change the date. It only makes the task to disappear
Comment 8 Stefan Sitter 2006-10-19 10:41:34 PDT
*** Bug 357281 has been marked as a duplicate of this bug. ***
Comment 9 Omar Bajraszewski 2006-11-23 07:22:36 PST
Blocking 0.5?
Comment 10 Matthew (lilmatt) Willis 2007-03-12 13:32:10 PDT
We can block 0.5 on this for now.
Comment 11 Michael Büttner (no reviews TFN) 2007-03-15 07:11:00 PDT
Error: aDate has no properties
Source File: file:///C:/mozilla/branch18/build/release/thunderbird/dist/xpi-stage/lightning/js/calUtils.js
Line: 567

Shouldn't be such a big deal...
Comment 12 Martin Schröder [:mschroeder] 2007-03-15 07:56:41 PDT
The error message from comment#11 is related to bug 372855, which has a patch attached.
Comment 13 Michael Büttner (no reviews TFN) 2007-03-22 09:28:54 PDT
Bug 372855 has been landed, so I tried to reproduce this one. But as long as a task doesn't have both (entry- and due-Date) it won't be displayed in the week view, am I totally wrong? Dag'n'Drop in the month-view doesn't work either, so maybe the steps to reproduce this bug are wrong. Could please anybody shed some light in here?
Comment 14 Stefan Sitter 2007-03-22 11:54:24 PDT
(In reply to comment #13)
> But as long as a task doesn't have both (entry- and due-Date) it won't
> be displayed in the week view, am I totally wrong? 

This is correct. In day and view view the tasks only show up if both dates are set. Bug 372855 / Bug 353066 should have solved most of the issues regarding day/week view.

> Dag'n'Drop in the month-view doesn't work either, so maybe the steps
> to reproduce this bug are wrong. 

Do you see the issues reported with Comment #6? I think this issue still exists. 
Comment 15 Michael Büttner (no reviews TFN) 2007-03-23 01:02:12 PDT
(In reply to comment #14)
> Do you see the issues reported with Comment #6? I think this issue still
> exists. 
Right, this is exactly the current state of affairs. I'll have a look what's going on there... Thanks for the clarification...
Comment 16 Michael Büttner (no reviews TFN) 2007-03-23 03:41:39 PDT
Created attachment 259398 [details] [diff] [review]
patch v1

The problem is that modifyOccurrence() in calendar-views.js expects that (if the new time of the item to be modified is given) both times, start- as well as end-time, is to be passed to the function. In case of a task with just entry- or due-date this fails and errorneously opens the event dialog. The proposed solution is just a work-around for the 0.5 timeframe, since I think we should pass something like a generic object that could contain any sort of modification to modifyOccurrence(), but that's another story. For now, I think we should just make sure that start- and end-date exists, clone the other datetime-object if necessary, and put those objects into entry- or due-date, whatever already exists in the task-item. that way we circumvent the current problem.

Unfortunately this patch resolves the basic problem, but I found that something else doesn't work as expected with drag'n'drop in the month-view. Just drag'n'drop a task-item with entry-date but no due-date from one day to another.  the task jumps from one day to the other, as expected. after a view fresh the modification is gone, just as if nothing happened. this behavior can be seen with tasks only, events work as expected. tasks with both times (entry and due) also work as expected. furthermore, this happens only with a local storage calendar. plain ics-files, wcap, and probably other providers as well function as expected. onModifyItem() in the views gets called with the modification as if everything works as expected (that's why we see the view doing the operation until a view refresh happens). it strikes me as if the storage provider simply doesn't store the modification in the database. Could please somebody else (having more expertise than me in this particular field) have a look on this?
Comment 17 Michiel van Leeuwen (email: mvl+moz@) 2007-03-24 11:47:41 PDT
Comment on attachment 259398 [details] [diff] [review]
patch v1

Instead of this temporary assigning to variables, why not just fix the function?

143         if ((aNewStartTime && aNewEndTime) || aNewTitle) {
if (aNewStartTime || aNewEndTime || aNewTitle) {

156             if (aNewStartTime) { // we know we have aEndTime too then

Remove that assumption, and do assignment in to parts (start and end time), and check for existence of each part.
Comment 18 Michael Büttner (no reviews TFN) 2007-03-26 08:46:43 PDT
> Instead of this temporary assigning to variables, why not just fix the
> function?
This would be an obvious solution, but I decided to take the other route because it is a more failsafe solution. At least for tasks you need to query for existence of entry/dueDate, then call this function with proper dates set. in case we just want to set the date/s - without knowing if only one of them exists this patch is the right way to go, since it would put the dates to the objects that already exist, independent of where the dates have been passed as arguments. that's the (probably not obvious reason) behind this patch.

I don't have strong preference regarding one or the other way. I just felt that it's worth while to explain why I didn't just do it you proposed. Feel free to take the patch as it is or reject it. In the latter case I'll provide the patch with the other solution. Anyway, I still feel that the solution as it stands is reasonable. We should generally strive for a better architecture regarding the start/end vs entry/due datetime semantics. Please feel free to open a spin-off bug for this. I also started a newsgroup thread on this some time ago, see [1] for details.

[1] http://groups.google.com/group/mozilla.dev.apps.calendar/browse_thread/thread/ab540a510349bacb/42ebc980b6cfd778?lnk=gst&q=michael+buettner&rnum=13#42ebc980b6cfd778
Comment 19 Michiel van Leeuwen (email: mvl+moz@) 2007-03-26 09:49:14 PDT
I don't really understand what you mean. Either fix would make the function just as failsafe, the way i see it.
In my proposed fix, you always check before you access any of the date variables.
Comment 20 Michael Büttner (no reviews TFN) 2007-03-29 08:36:17 PDT
Created attachment 260018 [details] [diff] [review]
patch v2

This patch solves the problem the other way 'round. Just pick the one you like better. Just to clarify what I tried to explain - With this patch, the caller needs to know what times are valid (entry/due/both) *before calling the method* and pass them in the correct parameter. With the first patch I submitted, the caller can pass one of the dates in either argument and it will be put into the object(s) that actually exist. It doesn't make that much of a difference, on the other hand. Just pick one.
Comment 21 Michiel van Leeuwen (email: mvl+moz@) 2007-03-29 12:40:34 PDT
Maybe I'm missing something, but I would say that both patches have the same end result: callers don't need to make sure that all times passed in are valid. But you are saying that it isn't.
For example, if aNewEndTime is not set (null), it will never be accessed. All calls to use it are inside |if (aNewEndTime)| blocks.

Or is your point that if the item you are editing does not have a endDate, you can pass in a new start date in the wrong parameter? That doesn't sound it has any use, and might actually be very confusing. What if you do want to set a new end date, while it did not have one before?
Comment 22 Bas van den Bosch 2007-03-29 22:05:11 PDT
Messing things up, I know, but after some posts in the forum I would like to suggest tasks without start and due date should be regarded as all-day todo's so they show up on top of the day in the calendar-view.
http://forums.mozillazine.org/viewtopic.php?t=535194
http://forums.mozillazine.org/viewtopic.php?t=535195

benn600 states Ipaq uses the startdates and apart from appearing on top of the day this would seem to be the logical behaviour imho and we don't have to mess with start- and endtimes. I know this should be a seperate bug but this seems the moment to throw this in the discussion. I'll file a seperate bug if wanted...
Comment 23 Michiel van Leeuwen (email: mvl+moz@) 2007-03-29 23:07:21 PDT
If you know it should be a separate bug, there is no need to comment in this one. It has nothing to do with this bug... (and no, this is not the moment to throw this in some random other discussion)
Comment 24 Michiel van Leeuwen (email: mvl+moz@) 2007-04-01 11:48:41 PDT
Comment on attachment 259398 [details] [diff] [review]
patch v1

r- based on me liking the other patch better.
Also based on assigning to aSomething vars. Those are input variables, and as such not to be assigned to.
Comment 25 Michiel van Leeuwen (email: mvl+moz@) 2007-04-01 11:49:34 PDT
Comment on attachment 260018 [details] [diff] [review]
patch v2

r=mvl. Let's go for this one.
Comment 26 Michael Büttner (no reviews TFN) 2007-04-02 04:36:09 PDT
Checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Comment 27 Sebastian Schwieger 2007-04-03 09:17:02 PDT
Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4pre) Gecko/20070403 Calendar/0.5pre
Comment 28 Damian Szczepanik 2007-07-08 09:57:53 PDT
Litmus testcase 4438 created

Note You need to log in before you can comment on or make changes to this bug.