466451 - Improved rendering of iMIP information in message view

Status: RESOLVED FIXED

(Calendar :: E-mail based Scheduling (iTIP/iMIP), enhancement)

Description

[Daniel Boelzle [:dbo]]

Currently only title, time and organizer are shown which makes it sometimes quite hard to judge whether to accept or decline a meeting, especially for recurring events. For the latter, we should show the recurring rule in clear text (might reuse calendar-dialog-utils.js' recurrenceRule2String).

Moreover I think invitation *updates* could be rendered differently, e.g. showing only what the organizer has actually changed, e.g.

! New Location: xyz
! New Cancellation for 12-12-2008

Comment 1

[Laurent Jouanneau]

I will have patchs:

- to move recurrenceRule2String(), splitRecurrenceRules(), checkRecurrenceRule() into a new JSM
- to add the recurring date and to have an explicit title header.

These patch will be made on top of patch for Bug 753305.

Comment 2

[Laurent Jouanneau]

Attachment: part 1 - Move some function to a new calRecurrenceUtils.jsm

Comment 3

[Laurent Jouanneau]

Attachment: part 2 - better header title and show recurrence date

I don't know yet the internal API. I'm not sure, for this new getItipHeader function, if we have informations in the event object or if we should really use the itipItem variable created in convertToHTML (getItipHeader comes from an old patch made by someone of our team)

Comment 4

[Laurent Jouanneau]

Attachment: part 2 - better header title and show recurrence date - v2

New version of part 2, adapted for latest changes on comm central.

Comment 5

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

Comment on attachment 623979 [details] [diff] [review]
part 2 - better header title and show recurrence date - v2

Review of attachment 623979 [details] [diff] [review]:
-----------------------------------------------------------------

r- to fix the below comments

::: calendar/lightning/components/lightningTextCalendarConverter.js
@@ +82,5 @@
> +    * @param       aItipItem  the event
> +    * @return string the header title
> +    */
> +    getItipHeader: function(aItipItem) {
> +       let header = "";

Given you are setting header each time, no need to initialize with an empty string.

@@ +103,5 @@
> +                                       "lightning");
> +                   break;
> +               case 'REPLY': {
> +                   // Generate proper body from my participation status
> +                   let sender = item.getAttendees({})[0];

Why is the sender always the first attendee? Shouldn't this be cal.getInvitedAttendee() ?

@@ +114,5 @@
> +               }
> +           }
> +       }
> +
> +       if (header == '')

Use brackets even for one-line if()s. Also, if you follow the above review comment, then just check if (!header) here.

@@ +152,5 @@
>  
>          // Simple fields
> +        let descr = doc.getElementById("imipHtml-header-descr");
> +        if (descr) {
> +            descr.textContent = this.getItipHeader(aNewItipItem);

With the current width of the imip detail (which I would not change), putting the string "Name Surname <[e-mail]> has invited you to Meeting about Ponies" will often break somewhere in the middle, and often in the middle of the Meeting title.

Given there is a bug open to overhaul the whole layout, I guess we can do it now, but its not good UX.

@@ +166,5 @@
> +            let startDate =  event.startDate;
> +            let endDate = event.endDate;
> +            startDate = startDate ? startDate.getInTimezone(kDefaultTimezone) : null;
> +            endDate = endDate ? endDate.getInTimezone(kDefaultTimezone) : null;
> +            let repeatString = recurrenceRule2String(recurrenceInfo, startDate, endDate, startDate.isDate);

You missed importing the module, also event.recurrenceInfo.

@@ +168,5 @@
> +            startDate = startDate ? startDate.getInTimezone(kDefaultTimezone) : null;
> +            endDate = endDate ? endDate.getInTimezone(kDefaultTimezone) : null;
> +            let repeatString = recurrenceRule2String(recurrenceInfo, startDate, endDate, startDate.isDate);
> +            if (repeatString) {
> +                dateString = repeatString;

If the rule is too complex for recurrenceRule2String to process, then it will return "Click here for details", which is not what you want. I'd suggest to make the function return null if it cannot process the rule and adjust the callers accordingly.

@@ +275,5 @@
>  
> +        // Create the HTML string for display
> +        let serializer = Components.classes["@mozilla.org/xmlextras/xmlserializer;1"]
> +                                   .createInstance(Components.interfaces.nsIDOMSerializer);
> +        return serializer.serializeToString(this.createHtml(event, itipItem));

I know you just moved this code, but you could use cal.xml.serializeDOM(this.createHtml(event, itipItem)); from calXMLUtils.jsm

Comment 6

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

Comment on attachment 623698 [details] [diff] [review]
part 1 - Move some function to a new calRecurrenceUtils.jsm

Review of attachment 623698 [details] [diff] [review]:
-----------------------------------------------------------------

This patch basically has r+, but probably needs changes due to the review comments on the other patch.

::: calendar/base/content/calendar-task-view.js
@@ +37,5 @@
>   * the terms of any one of the MPL, the GPL or the LGPL.
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
> +Components.utils.import("resource://calendar/modules/calRecurrenceUtils.jsm");

Why is this needed in calendar-task-view.js ?

Comment 7

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

Attachment: Combined patch, debitrotted

For your convenience, this patch combines your two previous patches, fixes some (but not all) review comments and applies to latest comm-central.

Comment 8

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

Also, try these STR:

1. create an event, invite someone
2. create an exception to the event

Result:

I got an email where the header says "has invited you to Test Weekly Recurring", without an organizer. I can imagine this has to do with how the sender is found in your header function.

Comment 9

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

I've taken care of the remaining review comments, push is imminent.

Comment 10

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

Pushed to comm-central changeset c1fc1371bc33