309426 - Double clicking on agenda/calendar items should open edit-dialogs

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Description

[Joey Minta]

I like double-clicking, a lot.  Generally, I double-click on items in order to
edit them or find out more information about them.  Right now, double clicking
on items in the agenda view or on calendars does nothing.

Comment 1

[Joey Minta]

Attachment: add double-click support

This patch adds double-click editing to events in the agenda and to calendars
in the listbox.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 196884 [details] [diff] [review]
add double-click support

In general, I'd suggest commenting each clause of code you add, even if the
comments are very brief.  This makes the code easier to read as well as
maintain.

Also, when adding code in Lightning, please always uses braces around (eg)
if/then/else clauses in order to prevent future problems where someone inserts
a piece of code at the same level of indent but forgets to add braces.

>Index: mozilla/calendar/lightning/content/agenda-tree.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/lightning/content/agenda-tree.js,v
>retrieving revision 1.9
>diff -p -U8 -r1.9 agenda-tree.js
>--- mozilla/calendar/lightning/content/agenda-tree.js	17 Sep 2005 23:37:56 -0000	1.9
>+++ mozilla/calendar/lightning/content/agenda-tree.js	21 Sep 2005 06:50:42 -0000
>
> [...]
>
>+agendaTreeView.modifyItem =
>+function agendaModifyItem(event)

Modifying an item is only one of several possible things this function to do. 
Can we call this onAgendaDoubleClick instead?

>+{
>+    if (event.button != 0) 
>+        return;
>+    var tree = document.getElementById("agenda-tree");
>+    var row = tree.treeBoxObject.getRowAt(event.clientX, event.clientY);
>+    var calendar = getCalendars()[0];

I think you really want to use getCompositeCalendar() here, so that the event
gets added to the default calendar in the composite, which may or may not be
the one in the first slot.

>+    if (!row)
>+        createEventWithDialog(calendar, today(), today());

After createEventWithDialog finishes, do we want to return, rather than
continuing on?

>+    var event = this.events[row];
>+    if (!this.isContainer(row))
>+        modifyEventWithDialog(event);

Same question.	

>+    else { //Clicked on a container
>+        if (event == this.today)
>+            createEventWithDialog(calendar, today(), today());
>+        else  {
>+            var tom = today().clone();
>+            var offset = event == this.tomorrow ? 1 : 2;

Parens around the == clause would make this a bit easier to read.

>+            tom.day += offset;
>+            tom.normalize()
>+            createEventWithDialog(calendar, tom, tom);

I assume we want to return here as well?
>+        }
>+    }
>+}

Comment 3

[Joey Minta]

Attachment: add double-click support v2

Version 2 includes additional comments about the code.	It also adds a return
after the first case in onDoubleClick (renamed).  The others you asked about
are properly nested in if/else, so return isn't needed.  

I feel that using the composite calendar as the target is wrong.  When the
event dialog opens up in this case, the calendar listbox has no selected item,
since compositeCalendar isn't an option.  The problem remains that sometimes
the first calendar isn't selected.  This can be fixed with nicely with a
getDefaultCalendar() function, but that feels like a separate bug to me.  If
you want, though, I'll do it here.  

Finally, this version includes a much improved version of calendar
doubleclicking, covering the case where a calendar isn't clicked on, but rather
empty space.

Comment 4

[Joey Minta]

Comment on attachment 197138 [details] [diff] [review]
add double-click support v2

OK, I just found the getCompositeCalendar().defaultCalendar is a valid
property, so ignore the whole thing about the target.  I'll use that.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 197138 [details] [diff] [review]
add double-click support v2

Sounds like there will be a new patch forthcoming; removing r? from this
version.

Comment 6

[Joey Minta]

Attachment: add double-click support v3

What we really want here is to get the selected calendar, since we're going to open up the dialog.  Adding an event to that calendar will filter up through the composite calendar, so everyone still gets notified.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 203882 [details] [diff] [review]
add double-click support v3

General comment: if you could be a bit more generous with vertical whitespace, it'd make the code easier to read.

>+agendaTreeView.onDoubleClick =
>+function agendaDoubleClick(event)
>+{
>+    // We only care about left-clicks
>+    if (event.button != 0) 
>+        return;
>+    // Find the row clicked on, and the corresponding event
>+    var tree = document.getElementById("agenda-tree");
>+    var row = tree.treeBoxObject.getRowAt(event.clientX, event.clientY);
>+    var calendar = ltnSelectedCalendar();
>+    var event = this.events[row];

Given that one of the function params is already called "event", can we call this something else?

r=dmose with review nits addressed

Comment 8

[Joey Minta]

patch checked in.