Last Comment Bug 466451 - Improved rendering of iMIP information in message view
: Improved rendering of iMIP information in message view
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: E-mail based Scheduling (iTIP/iMIP) (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: 1.9
Assigned To: Laurent Jouanneau
:
Mentors:
Depends on: 815151
Blocks:
  Show dependency treegraph
 
Reported: 2008-11-24 04:23 PST by Daniel Boelzle [:dbo]
Modified: 2012-11-26 07:43 PST (History)
3 users (show)
dbo.moz: wanted‑calendar1.0+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
part 1 - Move some function to a new calRecurrenceUtils.jsm (45.29 KB, patch)
2012-05-14 09:19 PDT, Laurent Jouanneau
philipp: review+
Details | Diff | Splinter Review
part 2 - better header title and show recurrence date (7.50 KB, patch)
2012-05-14 09:28 PDT, Laurent Jouanneau
no flags Details | Diff | Splinter Review
part 2 - better header title and show recurrence date - v2 (7.74 KB, patch)
2012-05-15 02:07 PDT, Laurent Jouanneau
philipp: review-
Details | Diff | Splinter Review
Combined patch, debitrotted (51.65 KB, patch)
2012-06-21 13:38 PDT, Philipp Kewisch [:Fallen]
philipp: review+
Details | Diff | Splinter Review

Description Daniel Boelzle [:dbo] 2008-11-24 04:23:00 PST
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 2012-05-14 07:55:44 PDT
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 2012-05-14 09:19:14 PDT
Created attachment 623698 [details] [diff] [review]
part 1 - Move some function to a new calRecurrenceUtils.jsm
Comment 3 Laurent Jouanneau 2012-05-14 09:28:56 PDT
Created attachment 623701 [details] [diff] [review]
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 2012-05-15 02:07:13 PDT
Created attachment 623979 [details] [diff] [review]
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] 2012-06-21 13:25:11 PDT
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
Comment 6 Philipp Kewisch [:Fallen] 2012-06-21 13:27:57 PDT
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] 2012-06-21 13:38:56 PDT
Created attachment 635438 [details] [diff] [review]
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] 2012-06-21 13:43:25 PDT
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] 2012-08-22 15:54:08 PDT
I've taken care of the remaining review comments, push is imminent.
Comment 10 Philipp Kewisch [:Fallen] 2012-08-22 15:56:26 PDT
Pushed to comm-central changeset c1fc1371bc33

Note You need to log in before you can comment on or make changes to this bug.