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)
Calendar
E-mail based Scheduling (iTIP/iMIP)
Tracking
(Not tracked)
RESOLVED
FIXED
4.4
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
Comment 1•14 years ago
|
||
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
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
OS: Windows XP → All
Hardware: x86 → All
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
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 6•9 years ago
|
||
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+
Assignee | ||
Comment 7•9 years ago
|
||
(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.
Reporter | ||
Comment 8•9 years ago
|
||
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)
Assignee | ||
Comment 9•9 years ago
|
||
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 10•9 years ago
|
||
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]
Assignee | ||
Comment 11•9 years ago
|
||
(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•9 years ago
|
||
(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?
Assignee | ||
Comment 13•9 years ago
|
||
Outlook uses orange for updated and gray for obsolete information. See attached screenshot.
Comment 14•9 years ago
|
||
Comment on attachment 8588545 [details]
highlight-updates.png
Okay, then nothing against your approach. :)
Comment 15•9 years ago
|
||
AFAIK the build error message just tells you that the entries in EXTRA_JS_MODULES must be sorted alphabetically.
Assignee | ||
Comment 16•9 years ago
|
||
(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.
Assignee | ||
Comment 17•9 years ago
|
||
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 18•9 years ago
|
||
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) ?
Assignee | ||
Comment 19•9 years ago
|
||
> 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•9 years ago
|
||
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+
Assignee | ||
Comment 21•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Attachment #8578995 -
Attachment is obsolete: false
Assignee | ||
Comment 22•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Attachment #8644358 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 23•9 years ago
|
||
https://hg.mozilla.org/comm-central/rev/e30a4f7f6a2f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 4.4
Comment 24•9 years ago
|
||
Hi MakeMyDay, it is possible that this check-in regressed Bug 1202901. Could you have a look?
Updated•9 years ago
|
Flags: needinfo?(makemyday)
Assignee | ||
Comment 25•9 years ago
|
||
Yes, this is a regression. A patch is uploaded for bug 1202901.
Flags: needinfo?(makemyday)
You need to log in
before you can comment on or make changes to this bug.
Description
•