373251 - Can open multiple windows of a single event/task from Agenda, Task lists and Unifinder

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[lithis]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a3pre) Gecko/20070307 Calendar/0.6a1

If an event is opened from the event list, it opens a new window regardless of whether that event was already open.

Reproducible: Always

Steps to Reproduce:
1. Double-click on an event in the event list.
2. Leave the event window open and return to the main calendar window.
3. Double-click on the same event again.
Actual Results:  
A second, identical "Edit Event" dialog is opened.

Expected Results:  
Only one "Edit Event" dialog should appear.

Comment 1

[Omar Bajraszewski]

It's the same story like in bug 373250. These bugs are very similar. I agree that only one edit event dialog should appear.

Comment 2

[Bas van den Bosch]

Apart from the behaviour described in bug 373250, I can't confirm the behaviour of two windows opening. So WFM for this bug in:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3pre) Gecko/20070305 Calendar/0.5pre

Can someone confirm the behaviour in 0.6a1?

Comment 3

[Martin Schröder [:mschroeder]]

Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3pre) Gecko/20070308 Calendar/0.5pre.

Bas, have you really tried to open the same event multiple times from the unifinder (event list)?

Comment 4

[Bas van den Bosch]

I stand corrected, when opening by doubleclicking on the unifinder-list it opens two windows, when doubleclicking on the calendar-view, the "edit Event" title changes to "New Event" like in previously mentioned bug. There should be a check on the open dialogs to see wether the event is already open. 

Comment 5

[Martin Schröder [:mschroeder]]

This is also true for Agenda and Todo panel in Sunbird & Lightning.

Comment 6

[lithis]

would this be considered a dataloss bug?

steps to reproduce:
1. an event is opened.
2. the same event is opened again; a second window opens.
3. changes are made to the event in one of the windows.
4. the ok button in the window with the changes is clicked.
5. the ok button in the other window is clicked.

actual results:
the changes would be overwritten when the unchanged window is closed.

expected results:
only one window should exist.

i would also like to point out that although double-clicking on the calendar view only opens one instance of the "edit event" window, it is a different instance than the windows that open from the event list (unifinder).

steps to reproduce:
1. open an event from the event list; an "edit event" window opens.
2. open the same event from the calendar view.

actual results:
a second "edit event" window opens.

expected results:
again, only one window should exist.

Comment 7

[Michael Büttner (no reviews TFN)]

When we made the event dialog window modeless I also added code that remembers which items are currently "open for editing" and taking care if further modification requests should come along. The part that should handle this can be found at [1]. Probably it has something to do with finding the "already open modification" in the list.

[1] http://lxr.mozilla.org/seamonkey/source/calendar/base/content/calendar-views.js#121

Comment 8

[lithis]

(In reply to comment #7)
> When we made the event dialog window modeless I also added code that remembers
> which items are currently "open for editing" and taking care if further
> modification requests should come along.

are you talking about inline edits in the calendar view? (if not, i'm sorry; i know nothing about the mozilla source code.) this bug has nothing to do with that; it's about multiple concurrent windows for the same event.

Comment 9

[Michael Büttner (no reviews TFN)]

(In reply to comment #8)
> are you talking about inline edits in the calendar view? (if not, i'm sorry; i
> know nothing about the mozilla source code.) this bug has nothing to do with
> that; it's about multiple concurrent windows for the same event.
no, i'm not talking about inline editing, we're indeed talking about the very same feature :-) don't mind, i just wanted to point to the relevant source code location.

Comment 10

[Simon Paquet [:sipaq]]

Setting wanted0.8- as the main Calendar developers will not devote any time to
this in the 0.8 timeframe. Patches are, of course, always welcome.

Comment 15

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

This could be fixed together with bug 390293 by creating a service that takes care of opening event dialogs and also the actual item modification.

Comment 16

[PM]

Attachment: Patch that search for already open item window

This patch avoids opening multiple editor windows for an item. 

It works when the user opens an item from the list of events or tasks, or from the calendar/task view s(context command or double-click). 

calFindItemWindow (calUtils.js) function allows you to find the window of an element from its ID. 
If a window exists, it is highlighted. 

Without this patch, double-clicking twice on the calendar view closes existing window and saves item without notification. A new instance is created (see caldav log in console).

Comment 17

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

Comment on attachment 552065 [details] [diff] [review]
Patch that search for already open item window

Nice, much simpler than I had in mind! I was working on making a dialog service that put everything into an independent xpcom component, but this is much easier to handle. r=philipp with some style stuff cleaned up, I'll take care of that before checkin.

Comment 18

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

Comment on attachment 552065 [details] [diff] [review]
Patch that search for already open item window


>     modifyOccurrence: function (aOccurrence, aNewStartTime, aNewEndTime, aNewTitle) {
> 
>+        var dlg=calFindItemWindow(aOccurrence);
>+        if (null!=dlg){
>+          dlg.focus();
>+          return;
>+        }
>+        
>         aOccurrence = this.finalizePendingModification(aOccurrence);

Hmm actually I'm not sure if this is good here, but I do see why you did it. If left in, then it will be impossible to move an event in the view via click'n'drag when an event dialog with that item is still open. If taken out, then clicking on the same event more than once will cause a modification failed message due to the way finalizePendingModification works.

Putting back on my review queue so I don't forget this.

Comment 19

[Bas van den Bosch]

@18: "If left in, then it will be impossible to move an event in the view via click'n'drag when an event dialog with that item is still open."

This is a good thing, dragging is also modifying an event, we don't want events being modified when a dialog is open.

Comment 20

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

Yes, but don't you think it might confuse the user that he can't drag the event?

Comment 21

[Bas van den Bosch]

I don't think so. If, with an open dialog, an event is dragged and the data in the dialog is saved after that, the event will be moved back (assuming no altered times in the dialog) which will be more confusing imho. You could throw a warning when trying to move an event which is already open.

Comment 22

[Daniel]

Does this patch also help when you open a Task from within a Reminder and closing the changed Reminder before the changed Task? 

(In reply to PMARTINAK from comment #16)
> Created attachment 552065 [details] [diff] [review]
> Patch that search for already open item window
> 
> This patch avoids opening multiple editor windows for an item. 
> 
> It works when the user opens an item from the list of events or tasks, or
> from the calendar/task view s(context command or double-click). 
> 
> calFindItemWindow (calUtils.js) function allows you to find the window of an
> element from its ID. 
> If a window exists, it is highlighted. 
> 
> Without this patch, double-clicking twice on the calendar view closes
> existing window and saves item without notification. A new instance is
> created (see caldav log in console).

Comment 23

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

(In reply to Daniel from comment #22)
> Does this patch also help when you open a Task from within a Reminder and
> closing the changed Reminder before the changed Task? 

Good question, I haven't tried that. I don't think it will though, since the reminders don't use the standard mechanisms. If we solve that using this patch, then you won't be able to dismiss a reminder before you close the event dialog, which seems wrong to me.

Maybe we should solve this the other way around: If the item gets changed from outside the event dialog, the event dialog should show a bar telling the user that the item was changed while viewing it. He would then get the option to either reload the event and discard changes, or to ignore and write anyway. The latter option would then update the old item so that the write succeeds.

Comment 24

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

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/31a247f6426c>
-> FIXED

Comment 25

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

Backported to comm-aurora <http://hg.mozilla.org/releases/comm-aurora/rev/a7f24c946128>

Comment 26

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

Backported to comm-beta <http://hg.mozilla.org/releases/comm-beta/rev/d030179bc5a4>

Comment 27

[Martin Schröder [:mschroeder]]

Checked in without r+?

Comment 28

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

Comment on attachment 552065 [details] [diff] [review]
Patch that search for already open item window

Thanks, r=philipp

Comment 29

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

Oh right, reasoning: I think for 1.0b7 and maybe even 1.0 we should take this patch. Then later on we fix bug 687286 to turn around the logic.