Closed Bug 466451 Opened 11 years ago Closed 7 years ago

Improved rendering of iMIP information in message view

Categories

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

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dbo, Assigned: laurent)

References

Details

Attachments

(1 file, 3 obsolete files)

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
Flags: wanted-calendar1.0+
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.
Assignee: nobody → laurent
Attachment #623698 - Flags: review?(philipp)
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)
Attachment #623701 - Flags: review?(philipp)
Attachment #623698 - Attachment description: Move some function to a new calRecurrenceUtils.jsm → part 1 - Move some function to a new calRecurrenceUtils.jsm
Attachment #623701 - Flags: review?(philipp)
Depends on: 753305
No longer depends on: 753305
New version of part 2, adapted for latest changes on comm central.
Attachment #623701 - Attachment is obsolete: true
Attachment #623979 - Flags: review?
Attachment #623979 - Flags: review? → review?(philipp)
Status: NEW → ASSIGNED
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 <email@example.com> 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
Attachment #623979 - Flags: review?(philipp) → review-
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 ?
Attachment #623698 - Flags: review?(philipp) → review+
For your convenience, this patch combines your two previous patches, fixes some (but not all) review comments and applies to latest comm-central.
Attachment #623698 - Attachment is obsolete: true
Attachment #623979 - Attachment is obsolete: true
Attachment #635438 - Flags: review-
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.
I've taken care of the remaining review comments, push is imminent.
Attachment #635438 - Flags: review- → review+
Pushed to comm-central changeset c1fc1371bc33
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.9
You need to log in before you can comment on or make changes to this bug.