Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Improved rendering of iMIP information in message view

RESOLVED FIXED in 1.9

Status

Calendar
E-mail based Scheduling (iTIP/iMIP)
--
enhancement
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: dbo, Assigned: Laurent Jouanneau)

Tracking

Bug Flags:
wanted-calendar1.0 +

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

9 years ago
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+
(Assignee)

Comment 1

5 years ago
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
(Assignee)

Comment 2

5 years ago
Created attachment 623698 [details] [diff] [review]
part 1 - Move some function to a new calRecurrenceUtils.jsm
Attachment #623698 - Flags: review?(philipp)
(Assignee)

Comment 3

5 years ago
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)
Attachment #623701 - Flags: review?(philipp)
(Assignee)

Updated

5 years ago
Attachment #623698 - Attachment description: Move some function to a new calRecurrenceUtils.jsm → part 1 - Move some function to a new calRecurrenceUtils.jsm
(Assignee)

Updated

5 years ago
Attachment #623701 - Flags: review?(philipp)
(Assignee)

Updated

5 years ago
Depends on: 753305

Updated

5 years ago
No longer depends on: 753305
(Assignee)

Comment 4

5 years ago
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.
Attachment #623701 - Attachment is obsolete: true
Attachment #623979 - Flags: review?
(Assignee)

Updated

5 years ago
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+
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.
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.9
Depends on: 815151
You need to log in before you can comment on or make changes to this bug.