411849 - Default date for add New Task is current day rather than the selected day

Status: VERIFIED FIXED

(Calendar :: Tasks, defect)

Description

[Steve]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12pre) Gecko/20080110 Calendar/0.8pre

When a new task is created the Start and Due Dates default to the current day rather than the selected day

Reproducible: Always

Steps to Reproduce:
1. Create a new task using any method desired.
2. Look at the default dates
Actual Results:  
The Start and End Dates are set to the current day instead of the day selected in the calendar.

Expected Results:  
At a minimum the dates should both default to the selected day in the calendar.

Comment 1

[Lars Wohlfahrt (thetux)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12pre) Gecko/20080110 Calendar/0.8pre ID:2008011005

I can confirm this behavior. Don't think this is a real bug (because start and due date are not activated by default), I would call it missing feature.

If this becomes offical, then the feature to select more than one day in calendar and get these values into the dialog for tasks and events is missing, too.

Comment 2

[Martin Schröder [:mschroeder]]

Cc'ing Christian for an UI expert opinion.

Comment 3

[Decathlon]

In my opinion this should be considered especially now that 0.9 has a good context menu. If you right click on any day in calendar view, and select "New event...", you get a start date for that day, but if you select "New task..." you get a start date that is always today. Seems an incoherent behavior.

Comment 5

[Decathlon]

Attachment: proposal patch

I've attached this proposal patch to force "Calendar team" to take a decision about this issue ;-)
I don't know if you've planned to consider this feature (I hope you'll do it of course), in this case, this patch *seems* working fine opening a new task (via context menu or toolbar buttons) or opening an existing task. In New Task dialog, the Start, Due and Completed Date datepickers always show the date of the selected day in calendar view.

To help in favour of a positive decision about the issue, I've added a preference with default value true: calendar.newtaskdialog.selectedDayDefaultDate (maybe a different name?) to disable the feature via configuration editor and turn back to the previous behaviour.
I've used getDefaultStartDate() function to generate both today date and selected day date because it returns a today date if his argument is null. It works but I don't know if something different should be used.

The patch modifies a behaviour about ToDo Status: normally in the New Task dialog, when status is "Completed" and the completed-date-picker is enabled with any date, if you change status (the date picker becomes disabled) and then select again "Completed" status, the date turn to today losing the previous value. With the patch, completed date is always the same until you change it or New Task dialog is closed. It seems to me better keeping the same date rather then lose it changing (inadvertently) the status.

If this feature won't be official, or the patch isn't fine, discard it without problems :)

Comment 6

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

Comment on attachment 357246 [details] [diff] [review]
proposal patch

I think we need to make this contextual. As you noted in comment 3, we now have a context menu on the views. creating a new task from there should open the new task dialog with that date, but in the toolbar we don't have the context of the views to base the date on. I'll ask christian on this issue.

I hope you are still interested in fixing this issue!

I'll give you a short code review anyway to gather ideas for future patches:

>+let gFirstTimeToDoStatus = false;
If possible I'd like to avoid a further global here. I'd go with setting a variable like "initialValue" on the datetimepicker node and when clicking the checkbox, check if the node has the initialValue property and if so, set the date from that property and set .initialValue = null.

Even better would be to find out if setting the value on the (initially disabled) datepicker has any negative consequences. If not, then you just need to set the date once and when the user enables the checkbox he can modify the date at will.

Also when testing, please also try to set a default alarm in the preferences and see if everything goes well. Setting a default alarm will enable the start date checkbox by default.


>+        window.selectedDayOnView = args.selectedDayOnView;
Rather than basing this on the selected day in the view (which is very context-specifc), you could call the variable args.initialStartDateValue. This way a caller can set this in any way needed in a context independent way.

>-          // if there isn't a completedDate, set it to now
>+          // if there isn't a completedDate, set it to the previous value
>+          // (it can be the old value, the selected day date or now)
>           if (!completedDate)
>-              completedDate = new Date();
>+              completedDate = oldCompletedDate;
While you are here, please add brackets to the if().


>+// default date in New Task dialog
>+pref("calendar.newtaskdialog.selectedDayDate", true);
Given we agree on context or not, we need a pref for this and the feature can be enabled by default.
For future patches: when adding a pref, it needs to be added to both lightning and sunbird's preferences file.



Thanks for looking into this issue, I really appreciate it. Next time you are planning to create a patch and the discussion on the bug doesn't have an obvious conclusion, go ahead and ask #calendar or me directly (i.e specifically asking me in the bug comment) then we can discuss whats best and you don't have to do double work. I'm happy to help :-)

Comment 7

[Christian Jansen]

I'm a bit undecided...., but on the other hand there is no real difference between calling the task dialog from the tool bar, menu, or context menu.

From an users perspective it makes sense to offer a straight forward solution which assures that the behavior is in all three cases the same. This would fit at least to users expectations (-> same things should behave same).

So, in the end I think it is the best to set the Start and End Date to the selected day instead of the current day, regardless if the user creates a task from menu, tool bar, or context menu.

Does this make sense?

Comment 8

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

(In reply to comment #7)
> Does this make sense?

Christian, what about totally out of context things like the today pane in mail mode? I would probably expect a newly created task to have the date currently shown in the today pane instead of whats selected in the views.

Decathlon, I'd say we use more general names and let the caller pass in a context date. This could default to using the current view's date when called through the command. What do you think?

Comment 9

[Decathlon]

(In reply to comment #8)
> (In reply to comment #7)
> > Does this make sense?
> 
> Christian, what about totally out of context things like the today pane in mail
> mode? I would probably expect a newly created task to have the date currently
> shown in the today pane instead of whats selected in the views.

The problem would be the same if you have only tasks section in today pane while you are in mail mode: neither visual confirmation of selected day nor of today nor the selected day in today pane, but new event menu command generates an event with the date of the day that you previously selected in calendar view.

On the other hand, now (and with Ligtning 0.9 too), if you create a new event with menu command (or toolbar button with LG 1.0pre) while you are in mail mode without today pane (again without visual confirmation of the date), new event is always created with the date of the day you previously selected on calendar view.
Moreover, in mail mode with today-pane on (only Ligthning 0.9 for now), a new event created via menu has, again, the date of the selected day on calendar view and not the date on today-pane.

> Decathlon, I'd say we use more general names and let the caller pass in a
> context date. This could default to using the current view's date when called
> through the command. What do you think?

IMO if present behaviour about dates for new event is right, I think the same should be for new tasks (with dates disabled of course), and this means the need of the selected day for all cases except that one of new task from contextual menu in today pane with events and tasks sections both enabled, that requires a context date (miniday's or minimonth's date), so at the end, I think it's better let the caller pass in a context date (although I wouldn't know how to do it) :(

Comment 10

[Christian Jansen]

(In reply to comment #8)
> (In reply to comment #7)
> > Does this make sense?
> 
> Christian, what about totally out of context things like the today pane in mail
> mode? I would probably expect a newly created task to have the date currently
> shown in the today pane instead of whats selected in the views.


This is true with regards to the today pane it makes sense to stick to the current date.
> 
> Decathlon, I'd say we use more general names and let the caller pass in a
> context date. This could default to using the current view's date when called
> through the command. What do you think?

Comment 11

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

Comment on attachment 357246 [details] [diff] [review]
proposal patch


> >+// default date in New Task dialog
> >+pref("calendar.newtaskdialog.selectedDayDate", true);
> Given we agree on context or not, we need a pref for this and the feature can
> be enabled by default.
I of course meant "given we agree on context or not, we _don't_ need a pref for this and the feature can be enabled by default.


ui-r=christian based on his last two comments.


Given his comments I'd like to see a patch
* Without any preferences, feature integrated byb default
* Allowing the caller to pass in a context date to the event dialog
* Having the today pane pass in the currently selected date.
* Having all other callers pass in the current view's selected date.
* With other review comments considered.


r- for now to get an updated patch.

Comment 12

[Decathlon]

Attachment: patch with a context date

This patch satisfies every requirement from comment #11, though something needs to be changed.

Apart from possible errors, I have basically the following doubts:

- for the default alarm in the preferences, I added a second parameter to the function cal_alarm_setDefaultValues in calAlarmUtils.jsm file. Don't know if there is a better way.

- Task view and task section in todaypane are the same and have the same context menu, so, to understand if a context menu command comes from todaypane I've checked focused element (function getDateIfTodaypane()). This way doesn't work if you generate a new task command from another element (e.g. the File->New->Task... menu) after the command from context menu of todaypane because the todo-tab-panel in todaypane keeps focused status. As a bad workaround, I've changed the focused element, with advanceFocus() method, every time a new task command comes from context menu of todaypane. It works but I don't like this solution, could you suggest me something better?

- Do variables' names need any change?

Comment 13

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

(In reply to comment #12)
> Created an attachment (id=362570) [details]
> patch with a context date
> 
> This patch satisfies every requirement from comment #11, though something needs
> to be changed.
> 
> Apart from possible errors, I have basically the following doubts:
> 
> - for the default alarm in the preferences, I added a second parameter to the
> function cal_alarm_setDefaultValues in calAlarmUtils.jsm file. Don't know if
> there is a better way. 
I'd rather not have setDefaultValues change much more than actual alarm properties. I think we should rather set the entry date before setDefaultValues is called.


> - Task view and task section in todaypane are the same and have the same
> context menu, so, to understand if a context menu command comes from todaypane
> I've checked focused element (function getDateIfTodaypane()). This way doesn't
> work if you generate a new task command from another element (e.g. the
> File->New->Task... menu) after the command from context menu of todaypane
> because the todo-tab-panel in todaypane keeps focused status. As a bad
> workaround, I've changed the focused element, with advanceFocus() method, every
> time a new task command comes from context menu of todaypane. It works but I
> don't like this solution, could you suggest me something better?
I agree we don't want a focus-specific behavior. This type of problem has become quite common with the command handler stuff. I think we might want to at least partially move away from pure <command>s.

If the command is used only once in a certain context (i.e there is only one button that creates a new event from the today pane), you can make that button use oncommand="blablaNewEvent(dateParamFromSource)" directly and have the other, context-free, parts use the original <command>.

If the context-sensitive command is used more than once, it makes sense to split the commands into two (i.e calendar_new_event_from_todaypane_command and calendar_new_event_from_view_command) and use accordingly.

> - Do variables' names need any change?

If you use initialDate(), then I'd prefer something like this to make it more clear in the calling context.

var initialDate = {
  mInitialDate: null,
  get: function get_initialDate() {
    return this.mInitialDate || currentView().selectedDay;
  },
  set: function set_initalDate(val) {
    return (this.mInitialDate = val);
  }
};

On the other hand, I'm not sure we should store the initial date like this. This might be obsolete anyway due to the command restructure above, so if possible I'd prefer passing the initial date directly from where ever it came from instead of saving it somewhere temporary.

Also, I'm not quite satisfied with the parentNode.parentNode... stuff in calendar-task-tree.xml. What about extending the task tree by a function to retrieve the date, i.e add a method getInitialEventDate() to calendar-task-tree.xml with a default implementation and then create an extra binding for the today pane that does nothing more than extend the original binding and overwrite getInitialEventDate()? There may be a similar but more elegant way though, you get what I mean though?

Sorry for the delay, I wasn't quite sure how to solve the issues at first and then other obligations got me carried away.

Comment 14

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

Comment on attachment 362570 [details] [diff] [review]
patch with a context date

See previous comment

Comment 15

[Decathlon]

Attachment: patch_v1

Patch with all modifies requested in comment #13.
Although your mail, I'm still not sure everything is done in the right way.

Since new events have an initial date with *time* set to the next hour and 0 minutes from the current time, I've thought to set the same for new task too.
At the moment when you create a new tasks, time-pickers are set to the current time (hour and minutes of the task creation) as initial time. The change makes new events and new tasks equals. It seems to me more coherent, but I don't know if there is a particular reason for an initial time different for new tasks and new events.

After an update of every file to the later in the repository, now seems that currentView() method works fine even from task view. With older files I wasn't able to make it work.

Comment 16

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

Comment on attachment 387953 [details] [diff] [review]
patch_v1


>     document.getElementById("calendar_new_todo_command").removeAttribute("disabled");
>+    document.getElementById("calendar_new_todo_todaypane_command").removeAttribute("disabled");
Interesting, I wonder why we unconditionally remove the disabled state here. Could you file a followup bug so we can investigate?

Otherwise this patch looks fine, I'll check it in soon. r=philipp

Comment 17

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

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/3dcbdb5905b8>

-> FIXED

Comment 18

[Stefan Sitter [:ssitter]]

This checkin regressed Bug 508295.

Comment 19

[Martin Schröder [:mschroeder]]

I think we can mark this bug fixed again since the regression is tracked in bug 508295.

Comment 20

[Martin Schröder [:mschroeder]]

(In reply to comment #16)
> (From update of attachment 387953 [details] [diff] [review])
> 
> >     document.getElementById("calendar_new_todo_command").removeAttribute("disabled");
> >+    document.getElementById("calendar_new_todo_todaypane_command").removeAttribute("disabled");
> Interesting, I wonder why we unconditionally remove the disabled state here.
> Could you file a followup bug so we can investigate?

Filed as bug 512436.

Comment 21

[Decathlon]

(In reply to comment #16)

> Interesting, I wonder why we unconditionally remove the disabled state here.
> Could you file a followup bug so we can investigate?

Completely forgot this. Sorry.

Comment 22

[Damian Szczepanik]

verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.9pre) Gecko/20100225 Calendar/1.0b2pre

Comment 23

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

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".