356833 - make the event/item dialog modeless

Status: VERIFIED FIXED

(Calendar :: Calendar Frontend, enhancement)

Description

[Michael Büttner (no reviews TFN)]

this bug is dedicated to make the event/task dialog modeless. please see http://groups.google.com/group/mozilla.dev.apps.calendar/browse_thread/thread/6ffc38d22ce21a83/d7cd70ac639cb3b4?hl=en#d7cd70ac639cb3b4 for further details on this discussion. if there are no objections to this issue i'll start working on the patch.

Comment 1

[Michael Büttner (no reviews TFN)]

actually making the dialog modeless is of course not a big deal. the problem is how we want to handle the necessary synchronization between the views and the dialog. currently i see the following options:

1) items which are currently edited will be locked in the views. no changes are allowed as long as the dialog is open. this could be visually indicated by some form of icon or whatever.

2) synchronize each and every change between views and dialog as soon as they are made. this would require a complete undo/redo for each and every possible modification.

3) close the dialog as soon as a modification in the view happens. in this case the dialog automatically stores any modifications made in the dialog.

4) synchronize changes from the views as possible without running into conflicts. lets say the user modifies the event title in the views which could be immediately reflected in the dialog as long as the user didn't already change the title in the dialog (in this case we could pop up a warning message).

we also need to further investigate how this synchronization should behave while editing a series, an occurrence or an exception. but for now these are my current thoughts on this topic. any comments on this are welcome...

Comment 2

[Christian Jansen]

Could you say something about the amount of work each solution would create?
I guess #2 would cause the most. From my point of view option #4 seems to be the best compromise.

The syncing would apply to:

- Title
- Location
- All day event
- Start, End

right?

Comment 3

[Omar Bajraszewski]

"Sunbird and Lightning should have an easy and intuitive user interface, allowing the user to get things done as effortlessly as possible."

I nominate it as blocking 0.5

Comment 4

[Omar Bajraszewski]

I have found two similar bugs: 216959 and 239748

Comment 5

[Michael Büttner (no reviews TFN)]

(In reply to comment #2)
> Could you say something about the amount of work each solution would create?
1) as i said, we'd need some indication that an item is locked. as this is not sorted out, i can't say how long it would take. the actual implementation would need around 1 day.

2) this is the most expensive solution. as long we're not having a proper undo/redo mechanism in place i'd strongly suggest to postpone this solution. besides that, it conflicts with the event dialog (the standard one as well as the prototypes dialog) since there wouldn't be the need for anything but a close-button.

3) this is the cheapest solution, less than a day...

4) i personally don't like this approach since we'd need warning dialogs. but the effective time to implement this would also be around 1 or 2 days...

i'd suggest that we go for approach 3 for now. as soon as we've a proper undo/redo in place we can reopen this issue and go for the 'immediately sync between dialog and views'-solution. does this sound reasonable?

Comment 6

[Michael Büttner (no reviews TFN)]

Attachment: patch v1

this patch implements approach 3, see my previous comments...

Comment 7

[Stefan Sitter [:ssitter]]

*** Bug 362486 has been marked as a duplicate of this bug. ***

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

There is an undo/redo function. What does it lack that you need?

Comment 9

[Michael Büttner (no reviews TFN)]

(In reply to comment #8)
> There is an undo/redo function. What does it lack that you need?
I know that we have an undo/redo function in place, but I was referring to the fact that it's not integrated into the Thunderbird undo/redo stack (I know that this is a Lightning-only issue), which is why I don't consider this functionality to be finished. Of course, this wouldn't be a show-stopper regarding the modeless issue.

But since nobody expressed interest in either of the presented options I thought it might be a good idea to take the easy road. Of course we can decide to take the full-sync approach. But my impression was that the approach taken with this patch is enough to get this feature up and running. Besides that, both dialogs (standard/prototype) would need some modifications in order to play well with the full-sync approach, which I felt isn't worth the hassle.

Further comments on this?

Comment 10

[Matthew (lilmatt) Willis]

I came up with a dataloss scenario with #3, but I'm not sure how realistic it is.

I set my calendar to auto-reload every x minutes (assuming mvl's reload calendars every x minutes patch lands).

Someone changes something on an event shown on my calendar from their own machine.

I'm also editing the event simultaneously.

We reload, notice the changes, update the view, and therefore autosave and close the dialog. 

If I was editing the same field as what changed, I never saw what the other person updated, and I wasn't prompted about the overwrite.

Comment 11

[Michael Büttner (no reviews TFN)]

(In reply to comment #10)
> We reload, notice the changes, update the view, and therefore autosave and
> close the dialog. 
This is indeed a nasty case I didn't think of. My first thought was that a solution to this problem would be that the views need to remember which items are currently being possibly modified by the opened dialog and ignore updates for them. But this still leaves us with the conflict we initially tried to prevent. I'm making a local change to an event that already has been remotely modified.

On the other hand, this scenario could also happen with the modal dialog. The change could happen to the event on the remote storage while it is currently being edited by the modal dialog. generally, it strikes me that all of this leads us towards something that we need to address in terms of a general solution regarding conflict resolution. what's your opinion on that?

Comment 12

[Michael Büttner (no reviews TFN)]

Attachment: patch v2

I forgot to handle deletion of events in the previous patch, as well as some other minor bits and pieces. This is the final version of the patch. Matt, I'd suggest to go ahead with this approach and solve the conflict stuff in a follow-up bug, ok?

Comment 13

[Matthew (lilmatt) Willis]

Comment on attachment 247970 [details] [diff] [review]
patch v2

>+++ mozilla/calendar/base/content/calendar-event-dialog.js	2006-12-08 15:37:35.265625000 +0100

>+    if(args.job) {
Space between if and parens, and this entire if statement should use 4 space indenting.
>+
Delete this blank line ^^
>+      // keep this context...
>+      var self = this;
>+
>+      // store the 'finalize'-functor in the provided job-object.
>+      args.job.finalize = function() {
Please make this function unanonymous for easier debugging in Venkman later.


>+    var args = window.arguments[0];
>+    args.job.dispose();
>+
>+    window.calendarItem = item;
>+
>     return true;
> }
> 
> function onCancel()
> {
>-
>+    var args = window.arguments[0];
>+    args.job.dispose();
Let's make these two lines a helper function and call it twice rather than repeating ourselves.


>+++ mozilla/calendar/base/content/calendar-item-editing.js	2006-12-08 15:38:44.609375000 +0100
>+function modifyEventWithDialog(item,job)
Add a space between the command and job. Maybe you want to call it aJob?

>+function openEventDialog(calendarItem, calendar, mode, callback, job)
aJob instead of job?

>     // open the dialog modally
This comment doesn't apply anymore ^^

>     if (getPrefSafe("calendar.prototypes.wcap", false)) {
>       openDialog("chrome://calendar/content/sun-calendar-event-dialog.xul", "_blank",
>-                 "chrome,titlebar,modal,resizable", args);
>+                 "chrome,titlebar,resizable", args);
>     } else {
>       openDialog("chrome://calendar/content/calendar-event-dialog.xul", "_blank",
>-                 "chrome,titlebar,modal,resizable", args);
>+                 "chrome,titlebar,resizable", args);
>     }
This block of code bugs me due to the repetition.
Since we're touching it, could you refactor it like so:

     var url = "chrome://calendar/content/calendar-event-dialog.xul";

     if (getPrefSafe("calendar.prototypes.wcap", false)) {
         url = "chrome://calendar/content/sun-calendar-event-dialog.xul"
     }

     openDialog(url, "_blank", "chrome,titlebar,resizable", args);


>+++ mozilla/calendar/base/content/calendar-views.js	2006-12-08 15:37:35.312500000 +0100
>@@ -85,17 +85,65 @@ var calendarViewController = {
>             } else {
>                 date = currentView().selectedDay.clone();
>                 date.isDate = false;
>             }
>             createEventWithDialog(aCalendar, date, null);
>         }
>     },
> 
>+    pendingJobs: [],
>+
>+    createPendingModification: function (aOccurrence) {
>+
Please make this function unanonymous and remove the blank line.

>+      // finalize a (possibly) pending modification. this will notify
>+      // an open dialog to save any outstanding modifications.
>+      aOccurrence = this.finalizePendingModification(aOccurrence);
>+
>+      var perndingModification = {
Your var name has a typo.  pernding rather tha pending

>+        controller: this,
>+        item: aOccurrence,
>+        finalize: null,
>+        dispose: function() {
Please make this function unanonymous.
>+          var array = this.controller.pendingJobs;
>+          for(var i=0; i<array.length; i++) {
>+            if(array[i] == this) {
Add a space between the for/if and the parens.

>+              array.splice(i,1);
>+              break;
>+            }
>+          }
>+        }
>+      }
>+
>+      this.pendingJobs.push(perndingModification);
>+
>+      modifyEventWithDialog(aOccurrence,perndingModification);
>+    },
>+
>+    finalizePendingModification: function (aOccurrence) {
>+
Please make this function unanonymous and remove the blank line.
>+      for each(var job in this.pendingJobs) {
Add a space between the for each and the parens.
>+        var item = job.item;
>+        var parent = item.parent;
>+        if(item.hasSameIds(aOccurrence) ||
>+           item.parentItem.hasSameIds(aOccurrence) ||
>+           item.hasSameIds(aOccurrence.parentItem)) {
Add a space between the if and the parens and realign as appropriate.
>+          // terminate() will most probably create a modified item instance.
>+          aOccurrence = job.finalize();
>+          break;
>+        }
>+      }
>+
>+      return aOccurrence;
>+    },
>+
This whole addition uses 2 space indenting.  Please use 4 to match prevailing style.

>     modifyOccurrence: function (aOccurrence, aNewStartTime, aNewEndTime, aNewTitle) {
>+
>+        aOccurrence = this.finalizePendingModification(aOccurrence);
>+    
This line has trailing spaces ^^


>+++ mozilla/calendar/prototypes/wcap/sun-calendar-event-dialog.js	2006-12-08 15:37:35.375000000 +0100
>@@ -82,16 +82,36 @@ function getString(aBundleName, aStringN
> function onLoad()
> {
>   //gConsoleService = Components.classes["@mozilla.org/consoleservice;1"].getService(Components.interfaces.nsIConsoleService);
> 
>   // first of all retrieve the array of
>   // arguments this window has been called with.
>   var args = window.arguments[0];
>   
>+  if(args.job) {
>+
Formatting in this file is up to you I guess, but I'd remove the blank line and add a space between the if and parens.

>+    // keep this context...
>+    var self = this;
>+
>+    // store the 'finalize'-functor in the provided job-object.
>+    args.job.finalize = function() {
Please make this function unanonymous.

> function onAccept()
> {
>+  var args = window.arguments[0];
>+  args.job.dispose();
>+
>   onCommandSave();
>   return true;
> }
> 
>+function onCancel()
>+{
>+  var result = onCommandCancel();
>+  if(result == true) {
>+    var args = window.arguments[0];
>+    args.job.dispose();
>+  }
>+  return result;
>+}
I'd also use a helper function here for these 2 lines so as to not repeat myself.


Since this is almost entirely style nits, r=lilmatt with those fixed and a follow up filed for the dataloss scenario if no current ones about concurrent modifications seem to fit the bill.

Comment 14

[Michael Büttner (no reviews TFN)]

Attachment: patch v3

addressed style-nits from r1, requesting r2.

Comment 15

[Michael Büttner (no reviews TFN)]

Joey, I'd like to know if you have time for the review within a reasonable timeframe since the latest version of the patch has been submitted 2 weeks ago. Please let me know whether or not I should shift the review request to someone else.

Comment 16

[Joey Minta]

I really can't say when I'll have time to look at this, as my schedule is shifting pretty rapidly at this moment.  A couple of drive-by comments:
-This needs UI-review.
-Documentation about what |jobs| do would help both with the review and with extension authors.  Comments answering who is responsible for passing in jobs, what a dialog is responsible for doing when that happens, etc. would be the crucial bits.
-Won't making the event-dialog modeless with this approach break disastrously if the user closes the main window while the dialog is open? 

Comment 17

[Michiel van Leeuwen (email: mvl+moz@)]

From an UI standpoint, i think making the dialog modeless is a good idea. For example, copying information from an email isn't really a weird thing.
Does the path prevent you from opening multiple windows for one item (as noted by joey in the newsgroup thread, this is not what we want)

Comment 18

[Michael Büttner (no reviews TFN)]

(In reply to comment #17)
> Does the path prevent you from opening multiple windows for one item (as noted
> by joey in the newsgroup thread, this is not what we want)
Of course it does. In case a window is already open and the user tries to modify the event (e.g. uses inline editing in the views) the open window will be automatically closed (and all changes will be saved).

Comment 19

[Michael Büttner (no reviews TFN)]

Comment on attachment 252026 [details] [diff] [review]
patch v3

requesting ui-review and shifting r2 to mvl.

Comment 20

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 252026 [details] [diff] [review]
patch v3

UI looks good to me. ui-review=mvl

I do have some problems with the patch. It took some time for me to understand what was going on with the jobs and all. Can we add some documentation? Maybe just a comment in this bug, and cvs blame will do the rest. (because I don't see one obvious place to add the comment in the code itself)

Comment 21

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 252026 [details] [diff] [review]
patch v3

r2=mvl, but with some documentation added.

Comment 22

[Michael Büttner (no reviews TFN)]

Checked in on trunk and MOZILLA_1_8_BRANCH

Comment 24

[Martin Schröder [:mschroeder]]

Bug 373250 and bug 373251 are regressions because of the introduction of modeless item dialogs.

Comment 27

[Simon Paquet [:sipaq]]

The patch has been checked in. Removing blocking request.

Comment 29

[Damian Szczepanik]

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