317473 - For new events no calendar is chosen by default in the event-dialog

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Description

[Joey Minta]

Whenever the new-event dialog is brought up, no calendar is chosen by default in the drop-down list.  To begin with, this is bad from a user-experience standpoint, since they're always required to set that field, when many users only have 1 calendar to begin with.

This fact is also responsible for the fact that an hour-glass often remains as the mouse-cursor after cancelling the event.  This is caused by the fact that the dialog goes into an error with:
Error: document.getElementById("item-calendar").selectedItem has no properties
Source File: chrome://calendar/content/calendar-event-dialog.js
Line: 340

I haven't had time to check if someone is calling new-event on the composite, instead of on a specific calendar, or if the dialog simply fails to set this properly. (Sunbird does it correctly.)

Comment 1

[Stefan Sitter [:ssitter]]

If you double click on an existing event the following is passed to the dialog:

onLoad: args.calendarEvent=[xpconnect wrapped (nsISupports, calIItemBase,
                            calIEvent, calIInternalShallowCopy, nsIClassInfo)] 
onLoad: args.calendarEvent.calendar=[xpconnect wrapped calICalendar]

But if you double click on an empty space (to create a new event) the following is passed to the dialog:

onLoad: args.calendarEvent=[xpconnect wrapped (nsISupports, calIItemBase, 
                            calIEvent, calIInternalShallowCopy, nsIClassInfo)] 
onLoad: args.calendarEvent.calendar=null

As no calendar is passed to the 'New event' dialog the uri check is not performed and no item is selected in the calendar list by default.

There are two possible solutions:
1) easy: if args.calendarEvent.calendar==null always select first item in the calendar list
2) complex: ensure that there is always a calendar attached to the new items, e.g. the selected one

Comment 2

[Stefan Sitter [:ssitter]]

Attachment: select first entry in list by default

This would be the simple solution.

Comment 3

[Joey Minta]

Comment on attachment 204004 [details] [diff] [review]
select first entry in list by default

+        document.getElementById("item-calendar").selectedIndex = 0;

I don't think we always want the first calendar in the list.  In Sunbird, whatever calendar was the last one that was selected in the calendar-listbox is used.  This makes it easier to make multiple changes to say, the 3rd calendar in the list.

Comment 4

[Joey Minta]

(In reply to comment #1)
> There are two possible solutions:
> 2) complex: ensure that there is always a calendar attached to the new items,
> e.g. the selected one

This is what Sunbird does, and I think it's a good model to follow, because it allows for users to work around the fact that we currently don't allow the list to be re-ordered.

Comment 5

[Stefan Sitter [:ssitter]]

I think that in 
  function createEventWithDialog(calendar, startDate, endDate, summary) and
  function createTodoWithDialog(calendar, dueDate, summary)
there should be something like

    if (!calendar) {
        calendar = getTheSelectedCalendar();
    }
    event.calendar = calendar;

Joey: What do you think? Is there already a function that does the getTheSelectedCalendar() stuff?

Comment 6

[Joey Minta]

(In reply to comment #5)
> I think that in 
>   function createEventWithDialog(calendar, startDate, endDate, summary) and
>   function createTodoWithDialog(calendar, dueDate, summary)
> there should be something like
> 
>     if (!calendar) {
>         calendar = getTheSelectedCalendar();
>     }
>     event.calendar = calendar;
> 
> Joey: What do you think? Is there already a function that does the
> getTheSelectedCalendar() stuff?
> 

In Lightning there is ltnSelectedCalendar().  In Sunbird there is getSelectedCalendar().  The tricky part is that calendar-item-editing ought to remain neutral between Lightning and Sunbird so it can be used in both.  Since createEventWithDialog allows for a calendar argument, I think you're going to need to do a null-check earlier.

Comment 7

[Stefan Sitter [:ssitter]]

(In reply to comment #6)
> Since createEventWithDialog allows for a calendar argument, I think you're
> going to need to do a null-check earlier.

The comment for createEventWithDialog says: "all params are optional". So this seems to be best place to do the null-check.
I have an idea how to solve that 'tricky part'. I'll try when I come home.

Comment 8

[Joey Minta]

(In reply to comment #6)
> In Lightning there is ltnSelectedCalendar().  In Sunbird there is
> getSelectedCalendar().  

Sorry, Sunbird's function is actually getSelectedCalendarOrNull()

(In reply to comment #7)
> The comment for createEventWithDialog says: "all params are optional". So this
> seems to be best place to do the null-check.
> I have an idea how to solve that 'tricky part'. I'll try when I come home.

I'm curious to see this, but I'm wondering why it's not simpler to make the 'calendar' argument required?  This isn't even an IDL, and lxr says there are currently only 7 consumers (4 of which I added last night, and they do provide a calendar).

Comment 9

[Stefan Sitter [:ssitter]]

Assigning to myself as I'm willing to work on it.

Comment 10

[Stefan Sitter [:ssitter]]

Sorry for bugspam

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

new-event is _supposed_ to be called on the composite calendar, which will then add the event to its default calendar.

Comment 12

[Stefan Sitter [:ssitter]]

Attachment: patch, v2

The changes:

calendar-management.js: Ensure that a calendar is selected after startup. This is the same what Sunbird does.

calendar-item-editing.js: If calendar is omitted use the currently selected calendar.

Comment 13

[Joey Minta]

Comment on attachment 204220 [details] [diff] [review]
patch, v2

+    if (!calendar && this.getSelectedCalendarOrNull) {
+        calendar = getSelectedCalendarOrNull();
+    }
+    event.calendar = calendar;
This seems bad if getSelectedCalendarOrNull actually returns null.

Right now, nothing in base/ cares whether it is used in Sunbird, Lightning or jminta-calendar-app.  This patch eliminates the possibility of using calendar-item-editing.js in any other application and I don't want to introduce that restriction unless absolutely necessary.

Instead, I think I might prefer a model that leaves the calendar argument optional, but selects the first item in the dropdown if none is specified.  It should then be the responsibility of the callers to substitute a more appropriate calendar default, if desired, BEFORE calling creatEventWithDialog.

Comment 14

[Stefan Sitter [:ssitter]]

Attachment: patch, v3

Ok, third try:

calendar-management.js
Ensure that a calendar is selected after Thunderbird/Lightning startup. Otherwise ltnSelectedCalendar() would fail until the user selected one calendar manually. This is pretty straight the same solution as in Sunbird.

calendar-item-editing.js
Assign passed calendar in createEventWithDialog() and createTodoWithDialog() to new item. Item.calendar is evaluated in event dialog to select the calendar in dropdown list.

calendar-event-dialog.js
If event dialog was called witout calendar we select the first one in the dropdown list. (Should not happen after this patch)

messenger-overlay-toolbar.xul and todo-list.js
Fix all calls of createEventWithDialog() and createTodoWithDialog() to pass calendar.

calendar-month-view.xml and calendar-multiday-view.xml:
In case of double click createNewEvent() (calls createEventWithDialog()) is called without calendar. I put the call to ltnSelectedCalendar() into createNewEvent() to avoid calling Lightning only functions from base files.

Comment 15

[Joey Minta]

Comment on attachment 204242 [details] [diff] [review]
patch, v3

Now this one I like.  Just a couple ot nits:
+    <menuitem id="ltnNewEvent" label="&event.title.new;" oncommand="createEventWithDialog(ltnSelectedCalendar());"/>
This line is now more than 80 chars long, which means it should be broken up.  Bugzilla/LXR-diff don't deal well with very long lines.

+    <toolbarbutton id="calendar-new-event-button" class="cal-toolbarbutton-1" label="&calendar.newevent.button.label;" tooltiptext="&calendar.newevent.button.tooltip;" oncommand="createEventWithDialog(ltnSelectedCalendar())"/>
+    <toolbarbutton id="calendar-new-task-button" class="cal-toolbarbutton-1" label="&calendar.newtask.button.label;" tooltiptext="&calendar.newtask.button.tooltip;" oncommand="createTodoWithDialog(ltnSelectedCalendar())"/>
These two aren't your fault for being > 80 chars, but while we're here, let's fix these too.

Thanks for the hard-work on this one!

Comment 16

[Stefan Sitter [:ssitter]]

Attachment: patch, v4

Same content as patch v3 but with more line breaks. 
I also wrapped some lines that did not change in content.
I hope this is my last patch on this bug Joey! ;-)

Comment 17

[Joey Minta]

Comment on attachment 204247 [details] [diff] [review]
patch, v4

Sweet! r=jminta

Comment 18

[Joey Minta]

Patch checked in. :-)