Last Comment Bug 533265 - Show differences when receiving an event update via email
: Show differences when receiving an event update via email
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: E-mail based Scheduling (iTIP/iMIP) (show other bugs)
: unspecified
: All All
-- enhancement (vote)
: 4.4
Assigned To: [:MakeMyDay]
:
:
Mentors:
Depends on: 1202901 1268856
Blocks: 1174511
  Show dependency treegraph
 
Reported: 2009-12-07 04:30 PST by Alexander Stohr
Modified: 2016-05-22 09:09 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
DisplayChangesOnInvitationUpdates-V2.diff (37.45 KB, patch)
2015-03-17 15:43 PDT, [:MakeMyDay]
richard.marti: ui‑review+
philipp: feedback+
Details | Diff | Splinter Review
Preview (13.67 KB, image/png)
2015-03-17 15:44 PDT, [:MakeMyDay]
philipp: feedback+
Details
DisplayChangesOnInvitationUpdates-V3.diff (39.04 KB, patch)
2015-04-05 12:39 PDT, [:MakeMyDay]
no flags Details | Diff | Splinter Review
highlight-updates.png (3.14 KB, image/png)
2015-04-06 04:04 PDT, [:MakeMyDay]
no flags Details
DisplayChangesOnInvitationUpdates-V4.diff (39.32 KB, patch)
2015-04-06 12:54 PDT, [:MakeMyDay]
philipp: review+
Details | Diff | Splinter Review
DisplayChangesOnInvitationUpdates-V5.diff (46.37 KB, patch)
2015-08-06 07:29 PDT, [:MakeMyDay]
makemyday: review+
Details | Diff | Splinter Review

Description User image Alexander Stohr 2009-12-07 04:30:35 PST
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
Comment 1 User image Philipp Kewisch [:Fallen] 2010-03-07 07:58:15 PST
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.
Comment 2 User image [:MakeMyDay] 2015-03-17 15:43:11 PDT
Created attachment 8578992 [details] [diff] [review]
DisplayChangesOnInvitationUpdates-V2.diff

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.
Comment 3 User image [:MakeMyDay] 2015-03-17 15:44:51 PDT
Created attachment 8578995 [details]
Preview
Comment 4 User image Richard Marti (:Paenglab) 2015-03-18 11:34:05 PDT
Comment on attachment 8578992 [details] [diff] [review]
DisplayChangesOnInvitationUpdates-V2.diff

Based on the screenshot it looks good. ui-r+
Comment 5 User image Philipp Kewisch [:Fallen] 2015-03-19 04:01:32 PDT
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?
Comment 6 User image Philipp Kewisch [:Fallen] 2015-03-19 04:03:40 PDT
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?
Comment 7 User image [:MakeMyDay] 2015-03-30 13:18:36 PDT
(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.
Comment 8 User image Alexander Stohr 2015-03-31 00:13:35 PDT
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)
Comment 9 User image [:MakeMyDay] 2015-04-05 12:39:05 PDT
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)

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.
Comment 10 User image Richard Marti (:Paenglab) 2015-04-06 02:15:15 PDT
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]
Comment 11 User image [:MakeMyDay] 2015-04-06 03:25:55 PDT
(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.
Comment 12 User image Richard Marti (:Paenglab) 2015-04-06 03:43:47 PDT
(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?
Comment 13 User image [:MakeMyDay] 2015-04-06 04:04:39 PDT
Created attachment 8588545 [details]
highlight-updates.png

Outlook uses orange for updated and gray for obsolete information. See attached screenshot.
Comment 14 User image Richard Marti (:Paenglab) 2015-04-06 04:10:02 PDT
Comment on attachment 8588545 [details]
highlight-updates.png

Okay, then nothing against your approach. :)
Comment 15 User image Stefan Sitter 2015-04-06 04:28:42 PDT
AFAIK the build error message just tells you that the entries in EXTRA_JS_MODULES must be sorted alphabetically.
Comment 16 User image [:MakeMyDay] 2015-04-06 06:40:58 PDT
(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.
Comment 17 User image [:MakeMyDay] 2015-04-06 12:54:44 PDT
Created attachment 8588714 [details] [diff] [review]
DisplayChangesOnInvitationUpdates-V4.diff

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.
Comment 18 User image Philipp Kewisch [:Fallen] 2015-04-06 17:03:21 PDT
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) ?
Comment 19 User image [:MakeMyDay] 2015-04-06 23:04:44 PDT
> 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 20 User image Philipp Kewisch [:Fallen] 2015-04-29 07:23:14 PDT
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.
Comment 21 User image [:MakeMyDay] 2015-08-06 07:29:33 PDT
Created attachment 8644358 [details] [diff] [review]
DisplayChangesOnInvitationUpdates-V5.diff

Updated patch with comments considered. Additionally, I added a hidden pref to disable that feature by users choice - by default, this is enabled.
Comment 22 User image [:MakeMyDay] 2015-08-06 07:46:15 PDT
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
Comment 24 User image Stefan Sitter 2015-09-09 10:43:22 PDT
Hi MakeMyDay, it is possible that this check-in regressed Bug 1202901. Could you have a look?
Comment 25 User image [:MakeMyDay] 2015-09-09 12:54:21 PDT
Yes, this is a regression. A patch is uploaded for bug 1202901.

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