415247 - Support all free/busy time types (FBTYPE) defined in rfc2445

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, enhancement)

Description

[Simon Vaillancourt]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12pre) Gecko/20080120 Calendar/0.8pre

Currently, only 3 colors are used to display availability of attendees: "free", "busy" and "No information". 

Our internal deployment shows that many users would like to also see the other FBTYPEs supported by RFC2445, which are defined here: 

        fbtypeparam        = "FBTYPE" "=" ("FREE" / "BUSY"
                           / "BUSY-UNAVAILABLE" / "BUSY-TENTATIVE"
                           / x-name

Would it be possible to add 2 new colors for "BUSY-UNAVAILABLE" and "BUSY-TENTATIVE" ? 





Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Simon Vaillancourt]

Also note that in my opinion, the freebusy view should be applied the following order:

1) FREE
2) BUSY-TENTATIVE
3) BUSY-UNAVAILABLE
4) BUSY/x-name

What I mean is that, for example, if a time range has both BUSY and BUSY-TENTATIVE, BUSY should win over BUSY-TENTATIVE...

Comment 2

[Daniel Boelzle [:dbo]]

Makes sense. API is already prepared for that (http://mxr.mozilla.org/mozilla1.8/source/calendar/base/public/calIFreeBusyProvider.idl#83).

Comment 3

[Simon Vaillancourt]

Attachment: Add support for different FBTYPES

Please review carefully since this is my first patch with real code change. I would like to revisit the freebusy states represented as integers eventually, but I wanted to keep this patch small and easy to understand. 

I also changed the color of the "busy" state so it's similar with another popular calendar client:
free: invisible (not changed)
busy: dark blue (Changed from teal)
busy-tentative: Light blue (new)
busy-unavailable(our of office) : Purple (new)
unknown: red (not changed)

I will add a screenshot right after submitting this patch.

Comment 4

[Simon Vaillancourt]

Attachment: Screenshot of the attendee dialog with the new freebusy types

Comment 5

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

Comment on attachment 355912 [details] [diff] [review]
Add support for different FBTYPES

>diff --git a/calendar/base/content/dialogs/calendar-event-dialog-attendees.xul b/calendar/base/content/dialogs/calendar-event-dialog-attendees.xul
>--- a/calendar/base/content/dialogs/calendar-event-dialog-attendees.xul
>+++ b/calendar/base/content/dialogs/calendar-event-dialog-attendees.xul
>@@ -125,18 +125,26 @@
>     </box>
>     <box orient="horizontal">
>       <box orient="vertical" flex="1">
>         <box orient="horizontal" align="center">
>           <box class="legend" status="FREE"/>
>           <label value="&event.freebusy.legend.free;"/>
>         </box>
>         <box orient="horizontal" align="center">
>+          <box class="legend" status="BUSY_TENTATIVE"/>
>+          <label value="&event.freebusy.legend.busy_tentative;"/>
>+        </box>        
>+        <box orient="horizontal" align="center">
>           <box class="legend" status="BUSY"/>
>           <label value="&event.freebusy.legend.busy;"/>
>+        </box>
>+        <box orient="horizontal" align="center">
>+          <box class="legend" status="BUSY_UNAVAILABLE"/>
>+          <label value="&event.freebusy.legend.busy_unavailable;"/>
>         </box>
While you are at it, could you change all <box orient="horizontal"> to <hbox> and <box orient="vertical"> to <vbox> ?

Also, I'd probably prefer putting the last two legend items in a new column. Something like this:

[       ]  Free        [       ] Out of Office
[       ]  Tentative   [       ] No information
[       ]  Busy

Christian should decide on layout and colors though, requesting ui-review.





>               for each (var entry in aEntries) {
>                   var rangeStart = entry.interval.start.getInTimezone(kDefaultTimezone);
>                   var rangeEnd = entry.interval.end.getInTimezone(kDefaultTimezone);
>-                  var fbState = (entry.freeBusyType == Components.interfaces.calIFreeBusyInterval.UNKNOWN ? 0 : 2);
>+                  
>+                  // fbState values are:
>+                  // 0 = Unavailable
>+                  // 1 = Free
>+                  // 2 = Busy
>+                  // 3 = Busy-Tentative
>+                  // 4 = Busy-Unavailable
>+                  var fbState = 0;
>+                  switch(entry.freeBusyType) {
>+                    case Components.interfaces.calIFreeBusyInterval.BUSY:
>+                      fbState = 2;
>+                      break;
>+                    case Components.interfaces.calIFreeBusyInterval.BUSY_TENTATIVE:
>+                      fbState = 3;
>+                      break;
>+                    case Components.interfaces.calIFreeBusyInterval.BUSY_UNAVAILABLE:
>+                      fbState = 4;
>+                      break;
>+                  }

let is the new var :-) Please change that for all variables inside the for each() block. Please fix indentation of the switch block we use 4-space indentation here. If you like, you could use a map instead, i.e:

let fbMap = {
    Components.interfaces.calIFreeBusyInterval.BUSY: 2,
    Components.interfaces.calIFreeBusyInterval.BUSY_TENTATIVE: 3,
    Components.interfaces.calIFreeBusyInterval.BUSY_UNAVAILABLE: 4
};

let fbState = fbMap[entry.freeBusyType] || 0;



It would be really great if you could file a followup bug (with a patch ;-) to get rid of that integer stuff!

r=philipp, please attach a patch with comments fixed.
Next time, please request review from someone on the module owner page ( http://wiki.mozilla.org/Calendar:Module_Ownership )

Comment 6

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

Putting the legend items in one column also causes the dialog to be cut off for me. This can be fixed by

* changing the id of the attendees dialog (to reset persistance)
* removing width and height from the persist attribute on the window
* making sure the dialog is resizable (which it is)

I think we should do the above too to fix things in the long run. In case we keep the one-column layout, please do the above in your patch, otherwise its optional but appreciated.

Comment 7

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

Oh, and please do give yourself credit in the files you've changed.

Comment 8

[Simon Vaillancourt]

Attachment: Add support for different FBTYPES v2

Thanks for the review! Here is a new patch:
- Credited myself in the modified files
- Replaced box elements by vbox and hbox where appropriate
- Got rid of the "integer" stuff to keep the freebusy state, note that I could not define a map with the Components.interfaces.calIFreeBusyInterval.* values.
- Now using let instead of var in the functions I modified.
- Split the legend in 2 columns (See new Screenshot).
- Changed the id of the dialog (Note: I didn't find this pattern anywhere so I just added "-v2" at the end of the id, let me know if it's fine.
- The dialog will not be persisting the height and width anymore.

Comment 9

[Simon Vaillancourt]

Attachment: Screenshot of the attendee dialog with the new freebusy types v2

Comment 10

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

Comment on attachment 356039 [details] [diff] [review]
Add support for different FBTYPES v2

ui-r=christian, based on an email we received. His comment:

"The two column layout is much better than the single one. We need to take care that the layout works with languages such like German, French or Portuguese."


I went ahead and consolidated a further box (made the dialog orient="vertical" and removed the outermost vbox) and made the legend not be equally spaced but aligned left. Thanks for taking care!

r=philipp

Comment 11

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

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/8a0de13d1cec>

-> FIXED