Closed Bug 317477 Opened 14 years ago Closed 14 years ago

Lightning: Canceling 'Edit Event' dialog opens 'New Event' dialog

Categories

(Calendar :: Lightning Only, defect)

x86
Windows 2000
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ssitter, Assigned: ssitter)

References

Details

Attachments

(1 file, 1 obsolete file)

Canceling 'Edit Event' dialog opens 'New Event' dialog.

Steps to reproduce:
1) double click on existing event in day/week view to open 'Edit Event' dialog
2) dismiss dialog with 'cancel'

Works with 20051121 Lightning build.
Fails with 20051122 Lightning build.
Regression from bug 309734.
Assigning to myself as I'm willing to work on it.
Assignee: jminta → ssitter
Attached patch patch (obsolete) — Splinter Review
The problem in this case is:

Double click on entry triggers the following events:

calendar-event-box: 1st 'click' event --> start single click timeout (350msec)

calendar-event-box: 2nd 'click' event --> last click within 350msec check
                                      --> double click --> Edit Event dialog

calendar-event-box: 'dblclick' event --> not handled
calendar-event-column: 'dblclick' event --> New Event dialog

I solved the problem by adding a 'dblclick' event handler to calendar-event-box and moving the double click check stuff from the 'click' to 'dblclick' event handler.

I found no regression during testing (only on windows); double clicking and inline editing still work after this changes.

Comments please.
Attachment #204582 - Flags: first-review?(jminta)
Comment on attachment 204582 [details] [diff] [review]
patch

Nice work!  Now for the annoying nits:
+        // stop 'single click edit' timeout
+        if (this.editingTimer) {
Is it possible to arrive here and have there not be an editingTimer?  If not, we should probably remove this check.


+            var self = this;
+            if (this.editingTimer) clearTimeout(this.editingTimer);
+            this.editingTimer = setTimeout(function () { self.startEditing(); }, 350);

I know you didn't write this, but if we're going to move it around, we should probably make it look proper.  (clearTimeout in {} on a new line)

I apologize for taking so long to get to this review.  I should be able to get to them faster starting next week.
Attachment #204582 - Flags: first-review?(jminta) → first-review-
Attached patch patch v2Splinter Review
(In reply to comment #3)
> +        // stop 'single click edit' timeout
> +        if (this.editingTimer) {
> Is it possible to arrive here and have there not be an editingTimer?  If not,
> we should probably remove this check.

There are several code places where this.editingTimer is set to null. Also I don't know for sure if the order of events is always guaranteed to be the same. To be on the safe side we should keep that check.

> +            var self = this;
> +            if (this.editingTimer) clearTimeout(this.editingTimer);
> +            this.editingTimer = setTimeout(function () { self.startEditing();
> }, 350);
> 
> I know you didn't write this, but if we're going to move it around, we should
> probably make it look proper.  (clearTimeout in {} on a new line)

Done.
Attachment #204582 - Attachment is obsolete: true
Attachment #205157 - Flags: first-review?(jminta)
Comment on attachment 205157 [details] [diff] [review]
patch v2

r=jminta
Attachment #205157 - Flags: first-review?(jminta) → first-review+
patch checked in
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.