Closed Bug 533265 Opened 15 years ago Closed 9 years ago

Show differences when receiving an event update via email

Categories

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

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: alexander.stohr, Assigned: MakeMyDay)

References

Details

Attachments

(3 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5 (.NET CLR 3.5.30729)
Build Identifier: 0.9

if someone changes some calendar entry there is no hint whats the differenc

Reproducible: Always

Steps to Reproduce:
1. create an entry and send notifications
2. change som value of the entry and send again
3. receive the results via e-mail
Actual Results:  
just a compact panel inside the e-mail window is displayed

Expected Results:  
provide a way to show what has changed (e.g. location, date/time, participants, agenda)

this might be in the form of a button "whats changed" that then either highlights the differences between the already stored entry and the newly arrived one. it might alternatively trigger some "diff" tool,
e.g. opening that in a simple text editor and showing the canged raw lines.

for unchanged entries a matching note might help.

for new entries a note about this beeing a novelty entry (not an update) should be added
Confirming, sounds reasonable. Might not be an easy task though, we'll need to create some sort of mechanism thats fast and can compare events, probably an addition to calIItemBase.

This would also be good for bugs like 274967.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: notification on updated entrys dont expose whats updated → Show differences when receiving an event update via email
This patch is to display modification in received updates on previously accepted email invitations. Newly added props are displayed italic, while removed props are striked through.
Assignee: nobody → makemyday
Status: NEW → ASSIGNED
Attachment #8578992 - Flags: ui-review?(richard.marti)
Attachment #8578992 - Flags: feedback?(philipp)
Attached image Preview
OS: Windows XP → All
Hardware: x86 → All
Comment on attachment 8578992 [details] [diff] [review]
DisplayChangesOnInvitationUpdates-V2.diff

Based on the screenshot it looks good. ui-r+
Attachment #8578992 - Flags: ui-review?(richard.marti) → ui-review+
Comment on attachment 8578992 [details] [diff] [review]
DisplayChangesOnInvitationUpdates-V2.diff

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

::: calendar/lightning/modules/ltnInvitationUtils.jsm
@@ +8,5 @@
> +
> +/**
> + * Scheduling and iTIP helper code
> + */
> +EXPORTED_SYMBOLS = ["ltn"];

this.EXPORTED_SYMBOLS = ["ltn"];

Also, if we eventually also have a core ltnUtils.jsm, then we should probably move this to ltn.invitations or ltn.invite.

@@ +25,5 @@
> +        // extended to only linkify urls that have an internal protocol handler, or
> +        // have an external protocol handler that has an app assigned. The same
> +        // could be done for mailto links which are not handled here either.
> +
> +        // XXX Ideally use mozITXTToHTMLConv here, but last time I tried it didn't work.

Totally optional, but maybe you can try mozITXTToHTMLConv again?

@@ +103,5 @@
> +            }
> +        }
> +
> +        if (!header) {
> +            header = cal.calGetString("lightning", "imipHtml.header", null, "lightning");

We should move this to ltnGetString(), or if we go with ltnUtils then ltn.getString()

@@ +348,5 @@
> +                        let attendees = _getAttendees(doc, aElement);
> +                        let oldAttendees = _getAttendees(aOldDoc, aElement);
> +                        // decorate newly added attendees
> +                        for (let att of Object.keys(attendees)) {
> +                            if (Object.keys(oldAttendees).indexOf(att) == -1) {

Can't you just check if (att in oldAttendees) ?

::: calendar/lightning/modules/moz.build
@@ +3,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +EXTRA_JS_MODULES += [
> +    'ltnInvitationUtils.jsm',

We also register resource://lightning/modules/ somewhere (or was it just chrome://lightning/ ?), maybe it makes sense to put this module there?
Attachment #8578992 - Flags: feedback?(philipp) → feedback+
Comment on attachment 8578995 [details]
Preview

Maybe you could put the crossed out date first? 

I'm not sure if it would look better, but what about making newly added attendees bold and those that have changed their attendee status italic?
Attachment #8578995 - Flags: feedback+
(In reply to Philipp Kewisch [:Fallen] from comment #6)
> Maybe you could put the crossed out date first? 
To read the relevant content before the outdated one seems more intuitive to me.

> I'm not sure if it would look better, but what about making newly added
> attendees bold and those that have changed their attendee status italic?
Bold is eventually to much, but maybe we can use additional colors to indicate differences.
highlighting colors will typically not work for:
- disability modes
- b/w & gray scale displays (e.g. e-paper based devices, still present non-color printers)
Updated patch considering the previous comments.

I have added colors (red/normal for new, red/italic for modified, gray/striked-through for removed and black for unchanged) for the regular mode and an alternate decoration for accessibility mode (bold for new, italic for modified, striked-through for removed and normal for unchanged, all black). Modified is only relevant for attendees, but now considers changes of partStat. (Therefore, I'm re-requesting UI review)

Also I added ltnUtils.jsm to host getString. Linkify is replaced by use of mozITXTToHTMLConv. We don't need a separate registration for Lightning modules - they end up in the modules folder, which is already registered in calendar/jar.mn.
Attachment #8578992 - Attachment is obsolete: true
Attachment #8578995 - Attachment is obsolete: true
Attachment #8588458 - Flags: ui-review?(richard.marti)
Attachment #8588458 - Flags: review?(philipp)
Comment on attachment 8588458 [details] [diff] [review]
DisplayChangesOnInvitationUpdates-V3.diff

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

I get this, when I try to build with your patch:
0:14.58 Reticulating splines...
0:14.58 Traceback (most recent call last):
0:14.58   File "config.status", line 951, in <module>
0:14.58     config_status(**args)
0:14.59   File "z:\Mozilla\comm-central\mozilla\python\mozbuild\mozbuild\config_status.py", line 149, in config_status
0:14.59     summary = the_backend.consume(definitions)
0:14.59   File "z:\Mozilla\comm-central\mozilla\python\mozbuild\mozbuild\backend\base.py", line 180, in consume
0:14.59     for obj in objs:
0:14.60   File "z:\Mozilla\comm-central\mozilla\python\mozbuild\mozbuild\frontend\emitter.py", line 140, in emit
0:14.60     for out in output:
0:14.60   File "z:\Mozilla\comm-central\mozilla\python\mozbuild\mozbuild\frontend\reader.py", line 975, in read_mozbuil

0:14.61     raise bre
0:14.61 mozbuild.frontend.reader.BuildReaderError:
0:14.61 ==============================
0:14.61 ERROR PROCESSING MOZBUILD FILE
0:14.62 ==============================
0:14.62
0:14.62 The error occurred while processing the following file:
0:14.62
0:14.62     z:/Mozilla/comm-central/calendar/lightning/modules/moz.build
0:14.63
0:14.63 The error was triggered on line 8 of this file:
0:14.63
0:14.64     'ltnInvitationUtils.jsm',
0:14.64
0:14.64 An error was encountered as part of executing the file itself. The error appears to be the fault of the script.

0:14.64
0:14.65 The error as reported by Python is:
0:14.65
0:14.65     ['UnsortedError: An attempt was made to add an unsorted sequence to a list. The incoming list is unsorted s
arting at element 0. We expected "ltnInvitationUtils.jsm" but got "ltnUtils.jsm"\n']
0:14.66
0:14.66 Makefile:94: recipe for target 'backend.RecursiveMakeBackend' failed
0:14.66 mozmake.EXE[1]: *** [backend.RecursiveMakeBackend] Error 1
0:14.66 client.mk:404: recipe for target 'build' failed
0:14.67 mozmake.EXE: *** [build] Error 2
0:14.71 639 compiler warnings present.

(In reply to MakeMyDay from comment #9)
> Created attachment 8588458 [details] [diff] [review]
> DisplayChangesOnInvitationUpdates-V3.diff
> 
> Updated patch considering the previous comments.
> 
> I have added colors (red/normal for new, red/italic for modified,
> gray/striked-through for removed and black for unchanged) for the regular
> mode and an alternate decoration for accessibility mode (bold for new,
> italic for modified, striked-through for removed and normal for unchanged,
> all black). Modified is only relevant for attendees, but now considers
> changes of partStat. (Therefore, I'm re-requesting UI review)

What about green for the new, now valid values and red for the old, no more valid values?

::: calendar/lightning/themes/common/imip.css
@@ +91,5 @@
> +.added {
> +       color: rgb(255, 0, 0);
> +}
> +.added[systemcolors] {
> +       color: rgb(0, 0, 0);

We don't use systemcolors in message pane until now and I think, it's not needed only on the text. Also using hard coded black isn't systemcolor friendly for example when the user has a dark background. Maybe WindowText or -moz-DialogText should fit better.

@@ +100,5 @@
> +       font-style: italic;
> +}
> +.modified[systemcolors] {
> +       color: rgb(0, 0, 0);
> +       font-style: italic;

font-style: italic; isn't needed as it is already defined in .modified.

The same applies for .removed[systemcolors]
(In reply to Richard Marti (:Paenglab) from comment #10)
> 0:14.65     ['UnsortedError: An attempt was made to add an unsorted sequence
> to a list. The incoming list is unsorted s
> arting at element 0. We expected "ltnInvitationUtils.jsm" but got
> "ltnUtils.jsm"\n']

Sorry Richard. I'll have a look at the compiler issue. I should have made another try-build...

> What about green for the new, now valid values and red for the old, no more
> valid values?

Isn't a red(or orange) font too prominent for something that was removed? I was inspired by the Outlook approach for the choosen colors, but I'm open for changes.

> We don't use systemcolors in message pane until now and I think, it's not
> needed only on the text.

I just added this for color blind users, who probably use that mode.

> Also using hard coded black isn't systemcolor
> friendly for example when the user has a dark background. Maybe WindowText
> or -moz-DialogText should fit better. 

I'll exchange that.

> font-style: italic; isn't needed as it is already defined in .modified.
> 
> The same applies for .removed[systemcolors]

I'll remove that.
(In reply to MakeMyDay from comment #11)
> (In reply to Richard Marti (:Paenglab) from comment #10)
> 
> > What about green for the new, now valid values and red for the old, no more
> > valid values?
> 
> Isn't a red(or orange) font too prominent for something that was removed? I
> was inspired by the Outlook approach for the choosen colors, but I'm open
> for changes.

On second thought, yes, red is too prominent for a removed entry. I haven't the Outlook approach in mind now, but are new values red or green there?
Attached image highlight-updates.png
Outlook uses orange for updated and gray for obsolete information. See attached screenshot.
Comment on attachment 8588545 [details]
highlight-updates.png

Okay, then nothing against your approach. :)
AFAIK the build error message just tells you that the entries in EXTRA_JS_MODULES must be sorted alphabetically.
(In reply to Stefan Sitter from comment #15)
> AFAIK the build error message just tells you that the entries in
> EXTRA_JS_MODULES must be sorted alphabetically.

Yes, thanks, this is it. I triggered a try build with an adjusted order in EXTRA_JS_MODULES and that ran successfully.

Philipp: I will upload an updated patch later today.
Updated patch with comments considered, build issue fixed and some modifications to improve partStat modification detection and handling for invitations, where the recipient is not in the attendee list.
Attachment #8588458 - Attachment is obsolete: true
Attachment #8588458 - Flags: ui-review?(richard.marti)
Attachment #8588458 - Flags: review?(philipp)
Attachment #8588714 - Flags: review?(philipp)
Comment on attachment 8588714 [details] [diff] [review]
DisplayChangesOnInvitationUpdates-V4.diff

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

Some nits before I fall asleep, I'll see if I can test this soon so I can r+.

::: calendar/lightning/content/imip-bar.js
@@ +93,5 @@
>              let imipBar = document.getElementById("imip-bar");
>              imipBar.setAttribute("collapsed", "false");
>              imipBar.setAttribute("label",  cal.itip.getMethodText(itipItem.receivedMethod));
> +            
> +            ltnImipBar.msgOverlay = msgOverlay;

Whitespaces snuck in here.

::: calendar/lightning/modules/ltnInvitationUtils.jsm
@@ +127,5 @@
> +            let modifiedOccurrences = [];
> +            function dateComptor(a,b) a.startDate.compare(b.startDate);
> +
> +            // Show removed instances
> +            for each (let exc in event.recurrenceInfo.getRecurrenceItems({})) {

Can you change this to for..of ? At least two more occurrences of for each..in found in this function.

@@ +217,5 @@
> +    /**
> +     * Expects and return a serialized DOM - use cal.xml.serializeDOM(aDOM)
> +     * @param  String aOldDoc
> +     * @param  String aNewDoc
> +     * @param  String aIgnoreId

For completeness, could you document what these parameters do?

Regarding the type name, we haven't been doing this elsewhere, but if you want to add it I think the accepted format is:

@param {String} aOldDoc         The reference document to compare.

@@ +251,5 @@
> +        /**
> +         * Extracts attendees from the given document
> +         * @param   Node   aDoc     document to search in
> +         * @param   String aElement element name as used in _compareElement()
> +         * @returns Array           named array: arr[attendee] = Node 

Trailing whitespace

@@ +254,5 @@
> +         * @param   String aElement element name as used in _compareElement()
> +         * @returns Array           named array: arr[attendee] = Node 
> +         */
> +        function _getAttendees(aDoc, aElement) {
> +            let attendees = [];

This should be a dict

@@ +269,5 @@
> +         * @param String aElement
> +         */
> +        function _compareElement(aElement) {
> +            // We need to distinguish attendee here, because I currently cannot change the name
> +            // in createInvitationOverlay due to string freeze - this should be cleaned up later

Sorry if this was already mentioned, but it looks like you'd like to take this for 38?

@@ +303,5 @@
> +                    }
> +                } else {
> +                    content = doc.getElementById(aElement + '-table');
> +                    oldContent = aOldDoc.getElementById(aElement + '-table');
> +                    let excludeAddress = aIgnoreId.split('mailto:').join('');

Will this gracefully fail for attendees with urn: scheme?

@@ +318,5 @@
> +                        // decorate removed attendees
> +                        for (let att of Object.keys(oldAttendees)) {
> +                            // if att is the user his/herself, who accepted an invitation he/she was
> +                            // not invited to, we must exclude him/her here
> +                            if (!(att in attendees) && att.split(excludeAddress).join('') == att) {

Why not use !att.contains(excludeAddress) ?
> Sorry if this was already mentioned, but it looks like you'd like to take this for 38?

Not neccessaryly - I've started this at the beginning of string freeze and wasn't sure when this would be ready. I leave it up to you whether or not this should be in 38.
Comment on attachment 8588714 [details] [diff] [review]
DisplayChangesOnInvitationUpdates-V4.diff

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

Ok, r+. I couldn't find an invitation in my inbox that triggers the behavior, but given you have screenshots that show it I trust it will work correctly.

About including in 38, I think it might be too risky. As much as I'd like to see more features in 38 I think we should stay on the safe side.
Attachment #8588714 - Flags: review?(philipp) → review+
Updated patch with comments considered. Additionally, I added a hidden pref to disable that feature by users choice - by default, this is enabled.
Attachment #8588714 - Attachment is obsolete: true
Attachment #8578995 - Attachment is obsolete: false
Try builds for Daily (42) are available for Mac and Windows (for these builds, you would have to enable the calendar.itip.displayInvitationChanges as I switched the default value after initiating the builds)[1].

To see the effect, you would need to have an invitation already accepted. Once you receive an update on the event by mail, the differences should be highlighted in the message overlay. This should look like on the already attached preview image [2].

[1] https://ftp-ssl.mozilla.org/pub/mozilla.org/calendar/lightning/try-builds/makemyday@gmx-topmail.de-f4c37c2a873f/
[2] https://bugzilla.mozilla.org/attachment.cgi?id=8578995
Attachment #8644358 - Flags: review+
Keywords: checkin-needed
Blocks: 1174511
https://hg.mozilla.org/comm-central/rev/e30a4f7f6a2f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 4.4
Hi MakeMyDay, it is possible that this check-in regressed Bug 1202901. Could you have a look?
Flags: needinfo?(makemyday)
Yes, this is a regression. A patch is uploaded for bug 1202901.
Flags: needinfo?(makemyday)
Depends on: 1202901
Depends on: 1268856
Depends on: 1360155
You need to log in before you can comment on or make changes to this bug.