361977 - [Proto] implementation of event summary dialog

Status: VERIFIED FIXED

(Calendar :: General, enhancement)

Description

[Michael Büttner (no reviews TFN)]

This bug is dedicated to the implementation of the event summary dialog. Please see the appropriate newsgroup thread http://groups.google.com/group/mozilla.dev.apps.calendar/browse_thread/thread/7ba1c65b7235f94c/322bf47b4415cb75#322bf47b4415cb75 and the specification http://wiki.mozilla.org/Calendar:SMB_Event_Summary for further details on this topic.

Comment 1

[Michael Büttner (no reviews TFN)]

Attachment: patch v1

draft version of the implementation, some final bits and pieces are missing.

Comment 2

[Michael Büttner (no reviews TFN)]

Attachment: patch v2

Final implementation of the event summary dialog. This dialog is invoked if a calendar is read-only or if the organizer differs from the calendar owner. This part is WCAP specific until we generally support a decent rights management. The user can change the participation status and/or set a reminder for the event. This functionality is only enabled in case of an invitation (calendar is not read-only but the organizer differs from the calendar owner).

Comment 3

[Michael Büttner (no reviews TFN)]

lilmatt: could you please push this one? I would like to get it checked in before i'm on vacation (which will be from next Thursday on).

Comment 4

[Joey Minta]

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

This seems like a large amount of code to be inserting into the core for a prototype.  Personally, I'd prefer to keep a stronger line between core and experimental code.

Comment 5

[Daniel Boelzle [:dbo]]

(In reply to comment #4)
> (From update of attachment 246785 [details] [diff] [review] [edit])
> This seems like a large amount of code to be inserting into the core for a
> prototype.  Personally, I'd prefer to keep a stronger line between core and
> experimental code.
> 

???
The vast majority of the patch goes into the prototypes folder and that little code that needs to go into the core is inside the 

     if (getPrefSafe("calendar.prototypes.wcap", false)) {

block.
I don't see any risk here for non-prototypes users.

Comment 6

[Joey Minta]

(In reply to comment #5)
> 
>      if (getPrefSafe("calendar.prototypes.wcap", false)) {
> 
> block.
> I don't see any risk here for non-prototypes users.
> 
This is indeed the block I'm concerned about.  It's not risk here that concerns me, but rather the fact that we're losing a clear definition of what 'core' is, as a substantial amount of logic related only to experimental code is inserted in the core.

Comment 7

[Daniel Boelzle [:dbo]]

(In reply to comment #4)
> (From update of attachment 246785 [details] [diff] [review] [edit])
> This seems like a large amount of code to be inserting into the core for a
> prototype.  Personally, I'd prefer to keep a stronger line between core and
> experimental code.
> 

IMO the core reason that lead to that code in calendar-item-editing.js and the "I am invited vs. I own an event/have write access" separation (leading to different dialogs) is that Lightning has no support for which user owns the displayed calendar, meaning Lightning is user agnostic in this sense.

Comment 8

[Daniel Boelzle [:dbo]]

(In reply to comment #6)
> me, but rather the fact that we're losing a clear definition of what 'core' is,
> as a substantial amount of logic related only to experimental code is inserted
> in the core.
> 

I think sooner or later, Lightning has to have support for the logic I have mentioned, i.e. providers need to provide a user-id to which the calendar belongs. Even in core code, we IMO need to have the ability to experiment with new features, always in the secure scope of prototyped code. But I agree that we should do this with care, and should streamline that code ASAP when a core API has been established.

Comment 9

[Matthew (lilmatt) Willis]

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

(In reply to comment #8)
> Even in core code, we IMO need to have the ability to experiment with
> new features, always in the secure scope of prototyped code. But I agree that
> we should do this with care, and should streamline that code ASAP when a core
> API has been established.

Not to speak for him, but what I think what jminta is objecting to is adding the equivalent of #ifdef IS_A_WCAP_CALENDAR in the middle of core code.

A different and arguably more modular solution would be to add a principal/owner/whatever-name-you-like attribute to the provider IDL and use the if(!WCAP) section of your code to provide that attribute for ICS/WebDV/storage and use the WCAP section for WCAP (obviously.)

Another possibility which is perhaps less technically and architecturally correct, but that might work would be to add a function to the core code to find the owner using the if(!WCAP) section, and override that function in the WCAP code.  I like the previous option better however.

Comment 10

[Daniel Boelzle [:dbo]]

(In reply to comment #9)
> A different and arguably more modular solution would be to add a
> principal/owner/whatever-name-you-like attribute to the provider IDL and use
> the if(!WCAP) section of your code to provide that attribute for
> ICS/WebDV/storage and use the WCAP section for WCAP (obviously.)

I would favor this solution too (having a stable calICalendar API). But IMO this ownerID attribute is yet immature. I am not sure whether it will last. My plan is to dump all collected WCAP interface additions and discuss them broadly once the whole story (prototype UI + WCAP provider) is in good shape, and that additional IDL has proven to be good and working.

Comment 11

[Michael Büttner (no reviews TFN)]

Attachment: patch v3

This is an updated version of the patch. The main concern with the previous version of the patch was the amount of code to be inserted into the core. First of all, the code that decides about which dialog should be invoked has been reduced to a single line of code. The necessary API changes have been proposed, please refer to bug #370150 for the details on this topic. Second, it would just be nice to have a decent summary of an event in case we're not allowed to modify its details (read-only, etc.). This is a feature we'll most probably want to have in the long term, I hope we can agree on this.

Comment 12

[Matthew (lilmatt) Willis]

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

asking mvl for ui-r

Comment 13

[Simon Paquet [:sipaq]]

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

2nd review is not necessary anymore, only first review by a peer.

mvl, could Christian do the UI-review since, you are already doing the code review?

Comment 14

[Michael Büttner (no reviews TFN)]

I'm a bit confused about who will do which review now? Please could somebody shed some light in here? mvl, who should be responsible for ui-review and who will do the code review? In case you do both, could you please give this a shot, since I'm waiting for review for more than 3 months now... ;-)

Comment 15

[Simon Paquet [:sipaq]]

(In reply to comment #14)
> I'm a bit confused about who will do which review now?

As of now, mvl is still the only doing UI reviews. Sorry for the confusion.

Comment 16

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

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

removing my review request. I don't see myself getting to do it any time soon.

Comment 17

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

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

Most comments are just style nits, I'm sorry about that ;) Since I'd like this to work for Sunbird too and I think some functions can be put into a utility file, I'll give r- for now. If you decide to do that in an extra bug, just make sure this dialog won't show in sunbird for now. 

I'll see if I can provide you with an updated review script to make it easier to fix the style.

>+
>+      // this will be called if file->new has been selected from within the dialog
>+      args.onNewEvent = function(calendar) {
>+        createEventWithDialog(calendar, today(), today());
This file has 4 space indentation. I believe calendar style has 4 spaces in general, please use this also for new files. To make it easier, I will create a script to automatically adjust spaces soon.

>diff -a -x CVS -U 8 -pN -rN mozilla_ref/calendar/lightning/jar.mn mozilla/calendar/lightning/jar.mn
>+++ mozilla/calendar/lightning/jar.mn	2007-02-19 12:42:23.000000000 +0100
In spirit of bug 371917, please also add the files to base/jar.mn and make the dialog work for sunbird.


>+.text-link:focus
>+{
>+  color : blue;
>+  border : 1px solid transparent;
>+}
Please remove extra spaces here and below.

>+  var start = isEvent(item) ? item.startDate : item.EntryDate;
This should be item.entryDate. If you want you can make this line shorter:
var start = item.startDate || item.entryDate;
Also, isn't it possible to have a task without an entryDate? Then start would be null and the line that follows would throw an error. You could do something like:
if (start) {
  document.getElementById("item-datetime").value = formatter.formatDateLong(start.getInTimezone(kDefaultTimezone);
}


>+  var organizer = item.organizer;
>+  if(organizer) {
>+    if(organizer.commonName && organizer.commonName.length) {
>+      document.getElementById("item-organizer").value = organizer.commonName;
>+    } else if(organizer.id && organizer.id.length) {
>+      var email = organizer.id;
>+      if (email.indexOf("mailto:") == 0)
>+        email = email.split("mailto:")[1]
>+      document.getElementById("item-organizer").value = email;
>+    }
>+  }
...
>+function sendMailTo()

As the class of this label is text-link, it should be enough to setAttribute("href", organizer.id); for both cases, this should open the new mail dialog. Specifically, this should also make it work for sunbird, which obviously does not have nsIMsgComposeService.

>+
>+  if(item.hasProperty("DESCRIPTION")) {
>+    var description = item.getProperty("DESCRIPTION");
>+    if(description && description != "") {
I prefer if (description && description.length) as you did above. Same goes for the other fields. Which ever you choose, just be consistent.

>+  opener.setCursor("auto");
Is this not window.opener ?

>+function checkRecurrenceRule(rule,array)
>+function splitRecurrenceRules(recurrenceInfo)
>+function editReminder()
>+function updateReminderDetails()
These are used in both the dialog and the readonly dialog. I would prefer putting these in a utility file.

>+function loadReminder(item)
>+function saveReminder(item)
>+function updateReminder()
>+function browseDocument()
These functions slightly differ. Can they be modified so they can also be extracted to a utility file?

>+  var recurrenceInfo = item.recurrenceInfo;
>+  if (recurrenceInfo) {
>+    recurrenceInfo = recurrenceInfo.clone();
>+    var rrules = splitRecurrenceRules(recurrenceInfo);
>+    if (rrules[0].length == 1) {
To keep down the indentation level, I would prefer using statements like if (rrules[0].length != 1) { return; }

>+          var sbs = Components.classes["@mozilla.org/intl/stringbundle;1"]
>+                              .getService(Components.interfaces.nsIStringBundleService);
>+          var props = sbs.createBundle("chrome://calendar/locale/sun-calendar-event-dialog.properties");
calGetString() here and elsewhere, or maybe
function calEventDialogString(aName, aArgs) {
  calGetString("sun-calendar-event-dialog", aName, aArgs);
}

>+
>+          var abs = function(v) {
>+            return v >= 0 ? v : -v;
>+          }
Math.abs()

>+          var ruleString = "???";
why the question marks? Will this be displayed?

>+              var ordinal_string = props.GetStringFromName('repeatDetailsOrdinal'+day_position(byday));
>+              var day_string = props.GetStringFromName('repeatDetailsDay'+day_of_week(byday));
Blanks before and after operators, here and possibly elsewhere. (I don't think my review script covers this). Also, please check for blanks after commas. I'll try to put this in a new version of the review script.

>+              if(bymonth.length == 1) {
>+                if(byday.length == 1) {
This can be put together to one if() condition.

>+          var kDefaultTimezone = calendarDefaultTimezone();
>+          var startDate = null;
>+          var endDate = null;
>+          if (isEvent(item)) {
>+            startDate = item.startDate;
>+            endDate = item.endDate;
>+          } else {
>+            var hasEntryDate = (item.entryDate != null);
>+            if (hasEntryDate)
>+              startDate = item.entryDate;
>+            var hasDueDate = (item.dueDate != null);
>+            if (hasDueDate)
>+              endDate = item.dueDate;
>+          }
var startDate =  item.startDate || item.entryDate;
var endDate = item.endDate || item.dueDate;


>+            while(repeatDetails.childNodes.length > lines.length)
>+              repeatDetails.removeChild(repeatDetails.lastChild);
Add {}

>+      if(!(name && name.length)) {
if (!name) { return; } if (name.length) { ... } else { ... }
would save an indentation level, but I'd be ok with how it is.

>+//////////////////////////////////////////////////////////////////////////////
>+// End of file
>+//////////////////////////////////////////////////////////////////////////////
Do we really need a comment for this? ;)

>+  <box id="item-general-box" orient="vertical">
>+    <box orient="horizontal" align="center">
why not vbox and hbox?

>+      <label id="item-link"
>+             class="text-link"
>+             crop="right" 
>+             onclick="browseDocument()"/>
As far as I understood, it is enough to set class="text-link" and href="(the url)". Maybe we don't need browseDocument()?

Comment 18

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

Also I have the feeling the dialog is a bit incomplete for readOnly calendars?

Some fields like "Location" show up, but if many of them are empty, it just seems like something is missing. I guess this is left to christian's ui-review though. Since the main prototype dialog is all colorful, it seems a bit strange to have a gray-black dialog :)

Comment 19

[Christian Jansen]

Generally the UI looks ok.

Some comments:
- In case "Location" and/or "Organizer" has not been set these fields
  should be hidden.
- The description field shouldn't be grayed-out for RO-Calendards.
- If the Event Summary Dialog is open, but minimized it should be possible to
  redesiplay the dialog by double-clicking the event a second time.

I've updated the specification (removed Delete Button)
http://wiki.mozilla.org/Calendar:SMB_Event_Summary

Comment 20

[Michael Büttner (no reviews TFN)]

Attachment: patch v4

This is the current version of the patch with all the above issues addressed. The largest modification concerns the factorization of the shared code between the main dialog and the summary dialog. I moved a number of function to /c/b/content/calendar-dialog-utils.js. We could also introduce a new file for the shared code but I didn't feel that it buys us anything. The following is a list of functions that are now being shared between the main dialog and the summary dialog:

_updateRepeatDetails
splitRecurrenceRules
checkRecurrenceRule
dispose
editReminder
updateReminderDetails
loadReminder
saveReminder
_updateReminder

I also took the liberty to move the files that implement the summary dialog to c/b/content/calendar-summary-dialog.js/xul. Furthermore, this patch moves the necessary css rules to c/b/themes/?instripe/calendar-event-dialog.css.

Comment 21

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

Comment on attachment 275591 [details] [diff] [review]
patch v4

All Comments about using calUtils are totally optional. Please make sure the dialog has an icon and preferably also a title. Maybe we should also introduce a minimum height, so that the dialog doesnt look so small My Suggestion:

-------------------------------------------
|II  New Event  (Read Only)          _[]X |
-------------------------------------------
| General ------------------------------- |
|   Title: New Event                      |
|   Date: Sunday, June 17, 2007           |
|                                         |
|                                         |
|                                         |
|                                         |
|                                         |
|                                         |
|                      [ OK ]  [ Cancel ] |
------------------------------------------

I also think the wait cursor is wrong. This dialog doesnt block the main window, therefore it can still be used. The wait cursor is there as long as the window is open, which looks kind of strange since you can still do things. You might get away with using the Arrow-And-Busy icon, but I think it should just not be there. Aside from that, the wait cursor is still visible after closing the dialog.

If you find a nice way to do so, it would also be good to not open more than one dialog per event (i.e reuse the dialog). This can be a spinoff bug though.


Some of the functions you put in calendar-dialog-utils.js seem to be very specific to the event dialog. I guess Im fine with keeping them there, but we should try keep calendar-dialog-utils.js a file that has functions that are used in more than one dialog.

Please run the style checker script I gave you, there are still some open issues. Also, doublecheck spaces after commas, there were some places where they are missing.

Im not quite clear which part of this dialog is going into base (mainly because I have to leave 10 minutes ago). If (new) parts are going into base, please also move all icons that belong to that dialog into base and give them a filename without sun-. I think its enough to have Sun in the licence block.

This is what I found, otherwise it looks fine. r- for now to fix these issues.


>+function _updateRepeatDetails(recurrenceInfo,startDate,endDate,allDay) {
Spaces after commas

>+    // first of all collapse the details text. if we fail to
I know I slack on spelling and grammar a lot too, but if you like at least capitalize correctly. I feel it looks more professional.

>+    // Retrieve a valid recurrence rule from the currently
>+    // set recurrence info. Bail out if there's more
>+    // than a single rule or something other than a rule.
>+    recurrenceInfo = recurrenceInfo.clone();
I am getting a has no properties error in this line when the item is not recurring. Also, the 'Repeat' label is still visible.

>+            var sbs = Cc["@mozilla.org/intl/stringbundle;1"]
>+                      .getService(Ci.nsIStringBundleService);
>+            var props =
>+                sbs.createBundle(
>+                    "chrome://calendar/locale/sun-calendar-event-dialog.properties");

Use of calGetString() will make some of the other lines a bit shorter.

>+function _updateReminder() {
Do these functions need to have a _ ?


>+function updateReminder() {
>+    _updateReminder();
>+}
A function to call the same function with a different name? Really needed?

>+function sendMailTo() {
>+    const kSUNBIRD_ID = "{718e30fb-e89b-41dd-9da7-e25a45638b28}";
>+    var appInfo = Cc["@mozilla.org/xre/app-info;1"]
>+                  .getService(Ci.nsIXULAppInfo);
>+    if (appInfo.ID == kSUNBIRD_ID) {
>+        return;
>+    }
File followup bug to launch mail url externally.


> url("chrome://calendar/skin/sun-calendar-event-dialog-attendees.png");
If this is going into base, rename icon and move into base if applicable.

Comment 22

[Michael Büttner (no reviews TFN)]

Attachment: summary dialog - full fledged

Comment 23

[Michael Büttner (no reviews TFN)]

Attachment: summary dialog - simple repeating event

Comment 24

[Michael Büttner (no reviews TFN)]

Attachment: summary dialog - minimum case

Comment 25

[Michael Büttner (no reviews TFN)]

Christian, please find some screenshots showing the summary dialog with varying level of content. I hope this helps judging whether or not we need any further adjustments...

Comment 26

[Christian Jansen]

Thanks for providing the screen shots.

Summary dialog - general
please add an icon to the dialog

summary dialog - minimum case, simple repeating event

- In case users can't modify anything the dialog should not come with
[0Ok] [Cancel]. [Ok] only would be sufficient enough.

**** summary dialog - minimum case

- Please add a Title to the dialog, like shown in the "simple repeating event" & "the full fledged" one

- Please Remove "Repeat"

Thanks. ui-r=christian

Comment 27

[Michael Büttner (no reviews TFN)]

Attachment: patch v5

> Use of calGetString() will make some of the other lines a bit shorter.
calGetString() is not used throughout the code. I didn't use your calEventDialogString() function since "sun-calendar-event-dialog" will someday be merged with calendar.dtd.

> Please make sure the dialog has an icon
None if our dialogs has an icon, I don't know why the summary dialog should be an exception. Furthermore, I simply don't have an icon and we didn't decide about how it should look like. It's extremely easy to add the icon, as we just need an icon-file with the same name as the id of the dialog, e voila. I strongly suggest that we handle this in a follow-up bug that introduces icons for all our dialogs.

> I also think the wait cursor is wrong.
Errm, unfortunately for non-repeating items the dialog threw an exception, that's why the cursor was simply a left-over.

> it would also be good to not open more than one dialog per event
This should just be the case, as it already is with the event dialog. This features has been introduced with making the dialog modeless. -> Bug 356833

> Some of the functions you put in calendar-dialog-utils.js seem to be very
> specific to the event dialog. I guess Im fine with keeping them there, but we
> should try keep calendar-dialog-utils.js a file that has functions that are
> used in more than one dialog.
First of all, you asked me to factorize the shared code in a separate file. I found calendar-dialog-utils.js a good candidate. I you insist, I'm fine with introducing a new file for this purpose. Although, the functions I moved into calendar-dialog-utils.js are used by the main event dialog as well as the summary dialog, that's why there have been moved there.

> Please run the style checker script I gave you, there are still some open
> issues. Also, doublecheck spaces after commas, there were some places where
> they are missing.
I manually double checked everything, I hope I didn't miss anything.

> If (new) parts are going into base, please
> also move all icons that belong to that dialog into base and give them a
> filename without sun-.
There is nothing new (at least no new images, that is), so I'll keep the files in /prototypes until the event dialog moves to /base.

> I am getting a has no properties error in this line when the item is not
> recurring. Also, the 'Repeat' label is still visible.
Fixed.

> A function to call the same function with a different name? Really needed?
The function with the underscore is the one coming from ...-utils.js. The one you're looking at is the one from the dialog itself. It just forwards to the utility function. I find it good to know just in case we need some extra processing here some time. But I also don't mind removing it.

> Do these functions need to have a _ ?
The underscore is there to distinguish between the 'local' utility files and the globals from the dialog. If you have some other preference, just tell me and I change it.

> File followup bug to launch mail url externally.
This is handled in bug 373761. I suggest we go ahead with this patch as it stands and address the remaining bits and pieces in the other one.

I carried over ui-review+ from Christian.

Comment 28

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

Comment on attachment 277732 [details] [diff] [review]
patch v5

(In reply to comment #27)
> None if our dialogs has an icon, I don't know why the summary dialog should be
> an exception.
Yes, you are totally right. I'd also say we need a followup bug to give both event dialog types an icon.

> ... calendar-dialog-utils.js ...
Maybe I misunderstood that files purpose. Go ahead and keep that code as is.


> I manually double checked everything, I hope I didn't miss anything.
4x Missing blank before '(', 7x Trailing whitespace

> The underscore is there to distinguish between the 'local' utility files and
> the globals from the dialog. If you have some other preference, just tell me
> and I change it.
Maybe a more verbose name would be good. Add a prefix like "summary" or "common"  etc.


> 
> > File followup bug to launch mail url externally.
> This is handled in bug 373761. I suggest we go ahead with this patch as it
> stands and address the remaining bits and pieces in the other one.
Since 373761 is fixed and checked in, do whatever you need to do here.


I also noticed, the description field still looks like a textbox. This should probably look more like a label/description element. Also, what about things like privacy,transparency,timezone (i.e all things from the menu). Shouldn't they be shown somewhere in the dialog too?

r- for now since I think we should have the missing info somewhere in the dialog. Since that is the only reason (apart from the comments above), feel free to change to r+ if Christian thinks the info is not needed.

Comment 29

[Michael Büttner (no reviews TFN)]

Christian, as far as I can see the only issues before this can finally land are the questions from Philipp (see previous comment). Please clarify if we this information should be included or if I should go ahead and let it land...

Comment 30

[Christian Jansen]

(In reply to comment #28)
[...]
> I also noticed, the description field still looks like a textbox. This should
> probably look more like a label/description element.


The box indicates that users can select text e.g. for copying it. We would loose that if we remove the box. Additionally the text box scrollbars, which occur if a long description is displayed would be placed "somewhere" on the dialog.

> Also, what about things
> like privacy,transparency,timezone (i.e all things from the menu). Shouldn't
> they be shown somewhere in the dialog too?

We should file spin-off bugs for

- Importance
- Privacy
- Timezone

 
> r- for now since I think we should have the missing info somewhere in the
> dialog. Since that is the only reason (apart from the comments above), feel
> free to change to r+ if Christian thinks the info is not needed.

IMHO the dialog works pretty good. We should go with that version, all the other additional features can be added later, or should be displayed in the event box itself.

See the very early (uncommented) draft:
http://wiki.mozilla.org/Calendar:Improved_Events_and_Tasks

Comment 31

[Michael Büttner (no reviews TFN)]

Comment on attachment 277732 [details] [diff] [review]
patch v5

> Feel
free to change to r+ if Christian thinks the info is not needed.
Changing to r+ per previous comment.

Comment 32

[Michael Büttner (no reviews TFN)]

patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED

Comment 33

[Martin Schröder [:mschroeder]]

Don't forget to file spin-off and follow-up bugs.

Comment 34

[Omar Bajraszewski]

Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7pre)
Gecko/20070829 Calendar/0.7pre and lightning 2007082905

Comment 35

[Omar Bajraszewski]

I've created new bugs that IMO are caused by this patch:
Bug 394169 and Bug 394174