Closed Bug 343049 Opened 18 years ago Closed 18 years ago

Lightning should display iMIP/iTIP related actions for meeting requests

Categories

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

Product:
Calendar
Component:
E-mail based Scheduling (iTIP/iMIP)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
Lightning 0.3

People

(Reporter: cmtalbert, Assigned: cmtalbert)

References

(Depends on 1 open bug)

Details

(Whiteboard: [cal-ui-review+])

Attachments

(7 files, 11 obsolete files)

HTML cleanup
1.43 KB, application/octet-stream
Details
icon for the winstripe iMIP bar - calendar/lightning/themes/winstripe/icon16.png
756 bytes, image/png
Details
A picture of the new iMIP bar (non-prototype) in action
157.82 KB, image/jpeg
Details
Simple notification of after clicking the "Add to calendar" button
156.24 KB, image/jpeg
Details
The Pinstripe icon
1.00 KB, image/png
Details
Patch to address the race condition issues
18.49 KB, patch
cmtalbert
: first-review+
dmosedale
: second-review+
Details | Diff | Splinter Review
patch, v9
19.39 KB, patch
dmosedale
: first-review+
dmosedale
: second-review+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4 Build Identifier: version 3 alpha 1 (20060619) (Thunderbird) Thunderbird needs to be able to understand when it receives an iMIP request and pass along enough information to the Lightning Text/Calendar MIME converter so that a proper determination can be made about how to display the email. For example, there are several iTIP states that can be set, and each state implies different actions that are available to the user in response to the encapsulated event. Upon receipt of the iMIP event email, the program must understand who sent the event, the METHOD (i.e. state) of that event and display proper options to the user regarding their logical next steps. Reproducible: Always Steps to Reproduce: 1. Send yourself an REQUEST iMIP/iTIP meeting request from another email client (Outlook, KOrganizer etc) 2. Open this email in Lightning+Thunderbird. 3. You will note that there is no indication of the state of the email (i.e. REQUEST), or your possible next steps: "ACCEPT | TENTATIVELY ACCEPT | DECLINE | COUNTER" 1. Send yourself a PUBLISH request from KOrganizer (it is easier in KOrganizer than Outlook) 2. Open this email in Lightning+Thunderbird 3. Note that the only next step which should be displayed is "ADD TO CALENDAR" Actual Results: The Lightning MIME converter will always only display the "Accept Meeting" and "Decline meeting" regardless of the actual iTIP state and the proper next set of actions. Expected Results: Lightning should allow the user an easy way to interact with meeting requests that allows the user the full functionality of iMIP/iTIP handling in the UI. As gleaned from the iTIP RFC 2446, these are the proper set of METHODs and the options available to the user from each one: -- METHOD -- -- NEXT STATES -- PUBLISH ADD REQUEST REPLY | REQUEST | COUNTER CANCEL (no state) -- update calendar) DECLINECOUNTER (no state) -- notify user REPLY REQUEST | CANCEL ADD REFRESH COUNTER REQUEST | DECLINECOUNTER REFRESH REQUEST Note that not all of these next states will be demonstrated to the user as such, for example, in the decline counter, the user just receives a notice that their attempt to change the event was rejected by the Organizer.
Depends on: 343664
Depends on: 343677
No longer depends on: 343664
Status: NEW → ASSIGNED
Depends on: 343664
Assignee: nobody → cmtalbert
Status: ASSIGNED → NEW
Whiteboard: [high risk]
Attached patch Lightning UI Components (obsolete) — — Splinter Review
This is a subset of the entire iTIP/iMIP patch file. It was broken out in order to be more manageable. The full iTIP/iMIP patch file resides in Bug 334685. Note that the patch to Bug 287550 (attendee list) had to be applied first; therefore, in the resulting iTIP/iMIP patch there are some residual changes from that first patch, particularly in the EventDialog.js section.
Attachment #233693 - Flags: first-review?(jminta)
Status: NEW → ASSIGNED
Depends on: 334685
Comment on attachment 233693 [details] [diff] [review] Lightning UI Components Some high-level comments from a first-pass. 1.) What made you choose to create a separate imip-bar-overlay.xul, rather than simply sticking that small bit of xul into messanger-overlay-sidebar.xul? I can think of a couple of possible reasons, but I'm curious what your motivation was. 2.) Something's wrong with this patch syntax, or something's wrong with bugzilla, because it's not showing up nicely in diff view. +# HTML event display in message +imipHTML.headerSentence=Event sent from <a href='%1$S'> %2$S </a> +imipHTML.header=<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd"><html><head><meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"><title>Untitled Document</title></head><body link="#000000" vlink="#FF0000"><center><table border="3"><tr><td><table bgcolor="#f0ece4" border="0" cellpadding="4" cellspacing="0" width=633><tr><td colspan="3" align="Left" bgcolor="#0a6cd0"><p style="color:#FFFFFF; font-size:16px; font-weight:bold;">&nbsp;Event sent from <a href='%1$S'> %2$S </a></p></td></tr> +imipHTML.header.no.organizer=<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd"><html><head><meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"><title>Untitled Document</title></head><body link="#000000" vlink="#FF0000"><center><table border="3"><tr><td><table bgcolor="#f0ece4" border="0" cellpadding="4" cellspacing="0" width=633><tr><td colspan="3" align="Left" bgcolor="#0a6cd0"><p style="color:#FFFFFF; font-size:16px; font-weight:bold;">&nbsp;Event Invitation</p></td></tr> This is going to be nearly impossible for l10n teams to handle. :-( I'd suggest we start having fun with e4x here. Let's connect over IRC about the best way to move forward with this review.
(In reply to comment #2) > (From update of attachment 233693 [details] [diff] [review] [edit]) > Some high-level comments from a first-pass. > > 1.) What made you choose to create a separate imip-bar-overlay.xul, rather than > simply sticking that small bit of xul into messanger-overlay-sidebar.xul? I > can think of a couple of possible reasons, but I'm curious what your motivation > was. > Mostly, the reason was that I did not know how involved the iMIP bar was going to be, originally. I also wasn't certain how much the other Lightning UI components would change over the course of my development, and so I didn't want to try to keep up with a moving target. I agree it's probably best to merge this into the larger XUL file. > 2.) Something's wrong with this patch syntax, or something's wrong with > bugzilla, because it's not showing up nicely in diff view. Could be the patch syntax. I cut the patch into pieces in order to make it easier to review. It's quite possible that I messed it up in the process. ..snip.. > This is going to be nearly impossible for l10n teams to handle. :-( I'd > suggest we start having fun with e4x here. > I don't know what e4x is. The way we handled this in Simdesk Organizer was to check the entire HTML file into the locale directory, then load the file from there. That makes an HTML file that has far better formatting. But, doing it this way allowed me to build it on the fly. So, the HTML you see is customized for what is in the ICS file. I'm open to other ideas. > Let's connect over IRC about the best way to move forward with this review. > Certainly.
Attached file HTML cleanup —
Clint, I took the liberty to cleanup the HTML code in your patch a little bit. Here's what i did: 1. Remove transitional HTML code and move the HTML code to a Strict DOCTYPE. 2. Move styling information into a separate CSS file The resulting combination should look nearly the same, but should better cope with various screen sizes. I took the liberty to remove the all-enclosing <table>, which seemed to be there just for a nice border-effect. The attached zip-archive contains three files: 1. imip_before.html (your old code with the "No Organizer"-header) 2. imip_after.html (HTML code with cleanup done) 3. imip.css (CSS file for imip_after.html) One question: If we move this code to XHTML (which can be done in about three minutes from the HTML4 Strict file), shouldn't it be possible to include DTD files, which we can then use for l10n purposes? Or is this not possible with XHTML?
(In reply to comment #4) > If we move this code to XHTML (which can be done in about three minutes from > the HTML4 Strict file), shouldn't it be possible to include DTD files, which we > can then use for l10n purposes? Or is this not possible with XHTML? Replying to myself here: It is possible to use DTDs to localize an XHTML file. On IRC Boris Zbarsky pointed me to a good example: http://lxr.mozilla.org/seamonkey/source/docshell/resources/content/netError.xhtml I believe that all strings from the HTML file should be moved into an imip.dtd file. This still begs the question what we should do with the two possible headers (organizer, no-organizer) but perhaps we can avoid having two files with some JS magic, that I know nothing about.
As we discussed, I've simplified the patch, and I've gotten rid of the imip-bar.xul. This file contains all the necessary changes to XUL and javascript and langauge files for the iMIP/iTIP UI interaction. This includes the iMIP bar and the sending of event notifications from the event dialog.
Attachment #233693 - Attachment is obsolete: true
Attachment #235043 - Flags: first-review?(jminta)
Attachment #233693 - Flags: first-review?(jminta)
Whiteboard: [high risk] → [high risk] [cal-ui-review needed]
Screen shot of the iMIP bar in action.
(In reply to comment #6) > As we discussed, I've simplified the patch, and I've gotten rid of the > imip-bar.xul. This file contains all the necessary changes to XUL and > javascript and langauge files for the iMIP/iTIP UI interaction. > This includes the iMIP bar and the sending of event notifications from the > event dialog. The first thing that springs to my eye are the long lines in the patch. One of the reviewers will probably also point it out, but while I'm here I can also say it. Please use a Mozilla-like coding style with line breaks at around 80 characters per line.
Comment on attachment 235043 [details] [diff] [review] All Xul and JS components for iTIP iMIP First pass. I didn't get all the way through it, but I wanted to get these comments up at least... + /* Send inviations */ + if (attendeeListBox.attendees.length > 0) { + sendInvitations(item); + } Are we sure we should always send invites? Or does the prompt also give the user a "Don't send" option? + // We no need to ensure that the organizer is set; however. s/no/do + var iTipSender = Components.classes["@mozilla.org/calendar/itip-sender;1"].createInstance(Components.interfaces.calIITipSender); + var organizer = Components.classes["@mozilla.org/calendar/attendee;1"].createInstance(Components.interfaces.calIAttendee); + if (iTipSender && organizer) { If these calls to XPCOM don't succeed, we have bigger problems than the error we'll throw in the next lines. You can drop the if. I'll also echo sipaq's comment about line wrapping. The normal style is: var organizer = Components.classes["@mozilla.org/calendar/attendee;1"] .createInstance(Components.interfaces.calIAttendee); Lastly, it feels to me that the iTipSender should be a service, not an instance? + item.setProperty("METHOD", "REQUEST"); This doesn't seem quite right. Won't this also make *my* copy of the item have METHOD:REQUEST too? I don't know the spec very well, but that seems counter-intuitive. +/* ***** BEGIN LICENSE BLOCK ***** + * Version: MPL 1.1/GPL 2.0/LGPL 2.1 This doesn't look like quite the right license block. (It's missing original author and copyright date lines.) +// iMIP Security warnings +const INCORRECT_METHOD_FOR_SIGNER=0; +const MESSAGE_SIGNER_NOT_IN_ICS=1; +const SIGNER_NOT_VALID=2; + How about defining these as constants on an interface? + var iTipItem = subject.QueryInterface(Components.interfaces.calIITipItem); + if (iTipItem) { + iTipItem.isSend = false; + iTipItem.autoResponse = true; If iTipItem doesn't implement calIItipItem, the the QueryInterface will throw, not return null. So if you're concerned that iTipItem might show up and not be what you want, you should try/catch, rather than using if. +function imipOnLoad() { + var listener = {}; + listener.onStartHeaders = oniMIPStartHeaders; Hmm, indenting looks a bit off here. You've been using 4 space everywhere else. + dump ("\n\n Adding IMIP listners now\n\n"); + gMessageListeners.push(listener); Where is gMessageListeners defined to be an array? + // Should we clear gITIPItem here? +} + Comments like this should include XXX in them, for easy searching +// This should be common code somewhere +// Function will return the string label if anything goes wrong +function getLtnString(stringlabel) http://lxr.mozilla.org/mozilla/source/calendar/lightning/content/lightning-utils.js#103 + } + catch(e){} + return result; Just a nit: I like to at least dump(e), so that devs can track down problems more easily. + bt1.setAttribute("hidden", false); + bt2.setAttribute("hidden", false); + bt3.setAttribute("hidden", false); Toolkit can be annoying here sometimes. There are certain cases where setting this to false isn't enough. You should do bt1.removeAttribute("hidden");
Addresses some of the early UI commentary and the comments from jminta's first look at the patch -- only addresses issues in imipbar.js and the resulting XUL for the UI update. Does not address event-dialog yet.
Attachment #235043 - Attachment is obsolete: true
Attachment #235043 - Flags: first-review?(jminta)
Attached image A problematic rendition of the UI Review (obsolete) —
This is the UI that dmose and shaver and I agreed on. However, I'm not liking the look of it at all. The "Reply:" tag makes it look like part of the mail header. And I couldn't figure out what is wrong with the menu-button. I'm sure some style sheet guru can make this UI work. I'm just not sure I'm that style-sheet guru.
So I'm thinking that we'd do well to make this stylistically more similar to the Junk or Images Blocked bars: icon on the left, some explantory text, then any relevant buttons. In particular, since we're not actually generating any iTIP replies just yet, we probably should use different verbiage for the toolbar for now to make clear exactly what's going on. Perhaps: (Calendar Icon) This message contains an invitation to a calendar event. (Add to my calendar) Maybe a different colored background as well. Light blue?
Whiteboard: [high risk] [cal-ui-review needed] → [high risk] [patch needed]
We're already past the "high-risk" deadline on this and several other bugs. I don't think this bug is really all that high-risk, however, since the functionality that it's replacing doesn't current work. I think it's still possible that we would accept some of this for 0.3. In particular, if a patch were to appear that implements the UI I described in comment 12, that seems like it could be a good candidate. I'd suggest we separate the S/MIME security stuff out into a separate bug to cut down on review and code complexity here. Also, I didn't notice any code for constructing the new-and-improved HTML in this patch, should that be here also?
(In reply to comment #13) > We're already past the "high-risk" deadline on this and several other bugs. I > don't think this bug is really all that high-risk, however, since the > functionality that it's replacing doesn't current work. I tend to agree with you. It is a very small amount of functionality that is only called if there is a text/calendar part of the email message. I think it's still > possible that we would accept some of this for 0.3. In particular, if a patch > were to appear that implements the UI I described in comment 12, that seems > like it could be a good candidate. I'd suggest we separate the S/MIME security > stuff out into a separate bug to cut down on review and code complexity here. Sure, I can do that. Do you care if I leave the code in the file and just comment it out? > Also, I didn't notice any code for constructing the new-and-improved HTML in > this patch, should that be here also? That code doesn't go here. That code lives in the Lightning Mime Handler which is addressed in bug 334468. It is unreleated to the iMIP Bar, and we decided early on to address them in separate bugs in order to limit complexity. However, I can of course check the patch for 334468 into this bug, if you prefer.
Attached image Picture of new UI for iMIP Bar (obsolete) —
UI sample from dmose's comment. Assuming that the bar goes all the way across the area, this is what you want, right?
(In reply to comment #14) > Sure, I can do that. Do you care if I leave the code in the file and just > comment it out? I'd prefer to not leave it commented out, but to actually have it not in this patch. > However, I can of course check the patch for 334468 into this bug, if you > prefer. Nah, I was confused. I think leaving it where it is is the right thing to do. The new screenshot looks great! It'd be nice to shrink the icon a bit so that the bar is the same height as the other bars. [cal-ui-review+] with the bar width fix you mentioned.
Whiteboard: [high risk] [patch needed] → [high risk] [patch needed][cal-ui-review+]
Attached patch Patch for new UI of imip bar (obsolete) — — Splinter Review
Here is another patch for the iMIP bar - it is publish only, it doesn't have any of the iMIP security. I've replaced sections of code that will not be used in 0.3 with simple "Bug xxxx" placeholders so that we know where the future functionality will need to go.
Attachment #235520 - Attachment is obsolete: true
Attachment #236263 - Flags: first-review?(dmose)
This is the icon used in the iMIP bar UI. Please add this to calendar/lightning/themes/__THEME__/icon16.png The 16 stands for 16X16 pixels, which is the size of the icon.
Attachment #235113 - Attachment is obsolete: true
Attachment #235521 - Attachment is obsolete: true
Attachment #236108 - Attachment is obsolete: true
I took the liberty of adding this small change to the UI. It became very obvious to me after adding several events to my calendar that we really need to indicate a change in state after clicking the "Add to Calendar" button. This state will not be saved (at this time), but while the user is looking at this event, at least they will understand that something did occur. If the user were to click off this message and back onto this message, the iMIP code would reset and the "Add to Calendar" button would be available again. In the future, we would automatically check to see if the event was in the calendar and would display proper information and actions for that state. There is a placeholder in the code for that check.
Attached image The Pinstripe icon —
This is the icon for calendar/lightning/themes/pinstripe/icon16.png
Attachment #236265 - Attachment description: icon for the iMIP UI bar. → icon for the winstripe iMIP bar - calendar/lightning/themes/winstripe/icon16.png
blocking0.3+ per dmose We want to improve the iMIP/iTIP story in 0.3
Flags: blocking0.3+
Whiteboard: [high risk] [patch needed][cal-ui-review+] → [high risk][cal-ui-review+][l10n impact]
Target Milestone: --- → Lightning 0.3
Whiteboard: [high risk][cal-ui-review+][l10n impact] → [high risk][cal-ui-review+][l10n impact][needs review dmose]
Comment on attachment 236263 [details] [diff] [review] Patch for new UI of imip bar >Index: lightning/jar.mn >=================================================================== >+ content/lightning/imip-bar.js (content/imip-bar.js) Nit: Add one more space before the ( for this line ^^^. Then the ( is at 50 chars. > classic.jar: > #expand skin/classic/lightning/lightning.css (themes/__THEME__/lightning.css) >+#expand skin/classic/lightning/imip.css (themes/__THEME__/imip.css) >+#expand skin/classic/lightning/icon16.png (themes/__THEME__/icon16.png) Lose one space before the ( on the imip.css line, and align the icon16.png line to match. Let's also rename that icon to cal-icon16.png so we have collisions with Tb or other extensions >Index: lightning/content/imip-bar.js >=================================================================== >+/* -*- Mode: javascript; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ Change this to 20 so tabs are obvious. >+ * The Initial Developer of the Original Code is Mozilla Corporation >+ * Portions created by the Initial Developer are Copyright (C) 2005 >+ * the Initial Developer. All Rights Reserved. >+ * >+ * Contributor(s): >+ * Clint Talbert <cmtalbert@myfastmail.com> No, it's originally by you, (c) 2006 >+//imip-bar.js We know what file we're in. Lose this. >+var gITIPItem; Capitalization style. "imipFoo" and "onImipFoo" please. >+// Sets up iTIP creation event listener >+const onITipItem = { >+ observe: function observe(subject, topic, state) { >+ if (topic == "onITipItemCreation") { >+ dump("\n\n ON ITIP ITEM CREATION!!\n\n"); Lose the dumps >+ try { Lotsa trailing spaces ^^^ >+ var iTipItem = subject.QueryInterface(Components.interfaces.calIITipItem); >+ iTipItem.isSend = false; >+ iTipItem.autoResponse = true; >+ iTipItem.targetCalendar = getTargetCalendar(); >+ var imipMethod = getMsgIMipMethod(); >+ if (imipMethod != "") { >+ iTipItem.receivedMethod = imipMethod; >+ } >+ gITIPItem = iTipItem; Yeah. This is what we want to avoid. Capitalization style. "imipFoo" and "onImipFoo" please. >+ } >+ catch(e) { } catch (e) { please >+addEventListener('messagepane-loaded', imipOnLoad, true); I don't see a removeEventListener anywhere. >+ >+// attempt to add self to gMessageListeners defined in msgHdrViewOverlay.js >+function imipOnLoad() { >+ var listener = {}; >+ listener.onStartHeaders = oniMIPStartHeaders; >+ listener.onEndHeaders = oniMIPEndHeaders; Capitalization style. "imipFoo" and "onImipFoo" please. >+ dump ("\n\n Adding IMIP listners now\n\n"); >+ gMessageListeners.push(listener); >+ ^^^ Extra spaces on that blank line >+ // Set up our observers >+ var observerService = Components.classes["@mozilla.org/observer-service;1"] >+ .getService(Components.interfaces.nsIObserverService); Please fix alignment. I'd also declare and use Cc and Ci in this file. >+ observerService.addObserver(onITipItem, "onITipItemCreation", false); >+ observerService.addObserver(signedObserver, "onSmimeSignedStatus", false); These two lines have trailing spaces. ^^^ I also don't see a removeObserver anywhere. We don't want to leak. >+function oniMIPStartHeaders() { Capitalization style. "imipFoo" and "onImipFoo" please. >+ dump("iMIP::onstartHeaders\n"); Lose dump >+ var imipbar = document.getElementById("imip-bar"); >+ imipbar.setAttribute("collapsed", "true"); imipBar please. >+ // New Message is starting, clear our iMIP/iTIP stuff so that we don't set it by accident Line length ^^^ >+ imipMethod = ""; >+ gITIPItem = null; Trailing spaces ^^^ >+function oniMIPEndHeaders() { Capitalization style. "imipFoo" and "onImipFoo" please. >+ dump("iMIP::onendHeaders\n"); Lose dump >+ var imipMethod = getMsgIMipMethod(); >+ spaces on blank line ^^^ >+ if (imipMethod != "") { >+ if (imipMethod != "nomethod") { >+ setupBar(imipMethod); >+ } >+ else { } else { please >+ setupBar("defaultLayout"); >+ } >+ // Bug xxx: no security yet >+ // handleiMIPSecurity(imipMethod); Include bug number. >+ } >+ else if (gITIPItem) { >+ // Handle the Tbird 1.5 case -- here we cannot get the iMIP method >+ // from the headers, so we must use the iTIPItem's method >+ setupBar(gITIPItem.receivedMethod); >+ // Bug xxx: no security yet >+ // handleiMIPSecurity(gITIPItem.receivedMethod); include bug number. >+function setupBar(imipMethod) >+{ >+ // Bug 348666 -xxx- Currently we only do PUBLISH requests >+ // In the future this function will set up the proper actions >+ // and attributes for the buttons as based on the iMIP Method >+ >+ var imipbar = document.getElementById("imip-bar"); >+ imipbar.setAttribute("collapsed","false"); imipBar please, and trailing spaces on this line ^^^ >+ var description = document.getElementById("imip-description"); >+ // Bug xxxx here is where we would check if this event was already added to >+ // calendar or not and display correct information here include bug number. >+ if (description.firstChild.data) { >+ description.firstChild.data = ltnGetString("lightning","imipStd_Desc"); Lose the underscores in entity names. imipBarText or something maybe? >+ // Since there is only a PUBLISH, this is easy >+ var button1 = document.getElementById("imip-btn1"); >+ button1.removeAttribute("hidden"); >+ button1.setAttribute("label", ltnGetString("lightning","imipAdd_To_Calendar.label")); Lose the underscores in entity names. >+ button1.setAttribute("oncommand", "setAttendeeResponse('PUBLISH', 'CONFIRMED');"); Trailing spaces ^^^ >+function getMsgIMipMethod() Capitalization and trailing spaces ^^^ >+{ Pick a braces style. You're using both in this file. In calendar, I mostly see function foo() { >+ var imipmethod = ""; imipMethod please. You're using it that way below. >+ try { >+ var msgURI = GetLoadedMessage(); >+ var msgHdr; >+ if (!(/type=application\/x-message-display/.test(msgURI))) Um, for us non-mail-news folk, can you include a one-liner about what's going on here ^^^ >+ msgHdr = messenger.messageServiceFromURI(msgURI).messageURIToMsgHdr(msgURI); Braces around this. >+ imipMethod = msgHdr.getStringProperty("imip_method"); trailing spaces ^^^ >+ } >+ catch(err) { } catch (e) { please. >+ dump("ERROR GETTING MESSAGE HEADER: " + err + "\n"); >+ } >+ >+ return imipMethod; >+} >+ >+function getMsgRecipient() trailing space >+{ >+ var imipRecipient = ""; >+ try { >+ var msgURI = GetLoadedMessage(); >+ var msgHdr; >+ if (!(/type=application\/x-message-display/.test(msgURI))) Include that comment here as well. >+ msgHdr = messenger.messageServiceFromURI(msgURI).messageURIToMsgHdr(msgURI); Braces around this. >+ } >+ catch(err) { } catch (e) { please. >+// Bug xxxx call calendar picker here include bug number and remove trailing spaces. >+function getTargetCalendar() trailing space. >+{ >+ var calMgr = Components.classes["@mozilla.org/calendar/manager;1"] >+ .getService(Components.interfaces.calICalendarManager); Cc and Ci and align >+//Type is type of response Add a space after // >+// event_status is an optional directive to set the Event STATUS property >+function setAttendeeResponse(type, eventStatus) >+{ >+ var myAddress = getMsgRecipient(); >+ if(type && gITIPItem) { >+ switch (type) {// We set the attendee status appropriately Add spaces before the // >+ case "ACCEPTED": Trailing spaces. >+ case "TENTATIVE": >+ case "DECLINED": >+ gITIPItem.setAttendeeStatus(myAddress, type); >+ doResponse(); >+ break; >+ case "REPLY": >+ case "PUBLISH": >+ doResponse(eventStatus); >+ break; >+ default: >+ // Nothing -- if given nothing then the attendee wishes to disregard trailing space. move "disregard" to next line so it's < 80 chars. >+// doResponse performs the iTIP action for the current iTIPItem that we >+// parsed from the email. >+// Takes an optional parameter to set the event STATUS property >+function doResponse(eventStatus) >+{ >+/** >+*** Removing this until the iTipResponder is done and landed >+ var iTipResponder = Components.classes["@mozilla.org/calendar/itip-responder;1"] >+ .createInstance(Components.interfaces.calIITipResponder); >+ // So, next we have to determine the state of this iTIP request and what the next step might be >+ // to recommend to the XUL window what to display >+ var returnObject = new Object(); >+ returnObject.value = gITIPItem; >+ >+ iTipResponder.recommendNextITipState(returnObject); >+ gITIPItem = returnObject.value; >+ iTipResponder.handleITipAction(gITIPItem, "TRUE"); >+ >+ gITIPItem = ""; >+*** >+**/ I thought dmose wanted this removed, not commented out? >+ // For now, just add the item to the calendar trailing spaces >+ var cal = getTargetCalendar(); >+ var item = gITIPItem.getFirstItem(); >+ while (item != null) { >+ item.status = eventStatus; >+ cal.addItem(item, null); >+ item = gITIPItem.getNextItem(); trailing spaces >+ } >+} >+ >+// Bug xxxx this gives the user an indication that the Action occurred include bug number >+// In the future we want to store the status of invites that you have added trailing spaces >+// to your calendar and provide the ability to request updates from the >+// organizer. This function will be responsible for setting up and maintaining >+// the proper lists of events and informing the user as to the state of the trailing spaces >+// iTIP action >+function finishITipAction(type, eventStatus) >+{ >+ // For now, we just do something very simple >+ var description = document.getElementById("imip-description"); >+ if (description.firstChild.data) { >+ description.firstChild.data = ltnGetString("lightning","imipAdded_Item_To_Cal"); No underscores in entity names. >Index: lightning/content/messenger-overlay-sidebar.xul >=================================================================== >+<!-- This is used so that we can reuse the thunderbird Notification bar style for our iMIP warning --> fix capitalization >+ >+<vbox id="messagepanebox"> >+ <hbox id="imip-bar" collapsed="true" insertbefore="msgHeaderView"> trailing spaces. >+ <hbox id="imip-notificationbar" flex="1" class="iMipNotificationBar" align="center"> Wrap the properties on this line so it fits. >+ <image id="lightningImage" src="chrome://lightning/skin/icon16.png"/> >+ <description id="imip-description" flex="1" class="msgNotificationBarText"> >+ &lightning.imipbar.description; >+ </description> >+ <spacer flex="1"/> >+ <button id="imip-btn1" align="right"/> The previous 6 lines have tabs. Please remove. Fix the emacs footer with tab=20 if that'll help. >+ </hbox> >+ </hbox> trailing spaces >+</vbox> >+ > </overlay> > > <!-- -*- Mode: xml; sgml-ident-step: 2; indent-tabs-mode: nil; -*- --> >Index: locales/en-US/chrome/lightning/lightning.properties >=================================================================== >+ >+imipAdd_To_Calendar.label=Add To Calendar >+imipAdded_Item_To_Cal=Event Added to Calendar Pick a capitalization for "to". I think it should be lowercase for both. >+imipStd_Desc=This message contains an invitation to a calendar event. Lose the underscores. You're using CamelHumps. That should be enough. >\ No newline at end of file Add one Otherwise, r1=lilmatt once all the cleanup happens.
Attachment #236263 - Flags: second-review?(dmose)
Attachment #236263 - Flags: first-review?(dmose)
Attachment #236263 - Flags: first-review+
Whiteboard: [high risk][cal-ui-review+][l10n impact][needs review dmose] → [high risk][cal-ui-review+][l10n impact][needs updated patch]
Comment on attachment 236263 [details] [diff] [review] Patch for new UI of imip bar Since there are going to be a large number of changes in the next update of the patch, I'd like to wait for that version before reviewing.
Attachment #236263 - Flags: second-review?(dmose)
Whiteboard: [high risk][cal-ui-review+][l10n impact][needs updated patch] → [cal-ui-review+][l10n impact][needs updated patch]
Attached patch patch addresses many nits (obsolete) — — Splinter Review
Remaining nits to be addressed by ctalbert: >Index: lightning/content/imip-bar.js >=================================================================== >+addEventListener('messagepane-loaded', imipOnLoad, true); I don't see a removeEventListener anywhere. >+ observerService.addObserver(onITipItem, "onITipItemCreation", false); >+ observerService.addObserver(signedObserver, "onSmimeSignedStatus", false); I don't see a removeObserver anywhere. We don't want to leak. >+ // Bug xxx: no security yet >+ // handleiMIPSecurity(imipMethod); Include bug number. >+ // Bug xxx: no security yet >+ // handleiMIPSecurity(gITIPItem.receivedMethod); include bug number. >+ var description = document.getElementById("imip-description"); >+ // Bug xxxx here is where we would check if this event was already added to >+ // calendar or not and display correct information here include bug number. >+ try { >+ var msgURI = GetLoadedMessage(); >+ var msgHdr; >+ if (!(/type=application\/x-message-display/.test(msgURI))) Um, for us non-mail-news folk, can you include a one-liner about what's going on here ^^^ >+ if (!(/type=application\/x-message-display/.test(msgURI))) Include that comment here as well. >+// Bug xxxx call calendar picker here include bug number >+// Bug xxxx this gives the user an indication that the Action occurred include bug number
Attachment #236263 - Attachment is obsolete: true
Comment on attachment 236985 [details] [diff] [review] patch addresses many nits There's also a dump I forgot to remove in imipOnLoad(). Please do
Whiteboard: [cal-ui-review+][l10n impact][needs updated patch] → [cal-ui-review+][l10n impact][needs review dmose]
Comment on attachment 236985 [details] [diff] [review] patch addresses many nits >+var gItipItem; A comment here about what global this is actually living in (and therefore it's expected lifetime) would be helpful. >+ >+// Sets up iTIP creation event listener A little more documentation here about how the architecture works would be a good thing, I think. >+const onItipItem = { >+ observe: function observe(subject, topic, state) { >+ if (topic == "onItipItemCreation") { >+ try { >+ var itipItem = subject.QueryInterface(Components.interfaces.calIItipItem); >+ itipItem.isSend = false; >+ itipItem.autoResponse = true; Comments on the above two lines about why those are the appropriate settings would be good too. >+function onImipEndHeaders() >+{ >+ var imipMethod = getMsgImipMethod(); >+ >+ if (imipMethod != "") { The general convention for this in JS is |if (stringObject.length)|. >+ if (imipMethod != "nomethod") { >+ setupBar(imipMethod); >+ } else { >+ setupBar("defaultLayout"); >+ } What's the advantage to switching from a magic constant named "nomethod" to a magic constant named "defaultLayout"? Given that the argument name in setupBar is "imipMethod", the former constant name seems to make more sense. >+ // Bug xxx: no security yet >+ // handleImipSecurity(imipMethod); >+ } else if (gItipItem) { >+ // Handle the Tbird 1.5 case -- here we cannot get the iMIP method >+ // from the headers, so we must use the ItipItem's method >+ setupBar(gItipItem.receivedMethod); This wants an XXX bug comment also, I bet. >+function getMsgImipMethod() >+function getMsgRecipient() These seem like cases where errors should cause an exception to propagate to the caller. Callers might want to do a try/catch, however. Also, no need to use all caps in reportErrors. >+/** >+ * Bug xxxx - Call calendar picker here >+ */ >+function getTargetCalendar() >+{ >+ var calMgr = Components.classes["@mozilla.org/calendar/manager;1"] >+ .getService(Components.interfaces.calICalendarManager); >+ var cals = calMgr.getCalendars({}); >+ return cals[0]; >+} Is this really worth filing a separate bug for? I mean, isn't it just a one line fix? >+function setAttendeeResponse(type, eventStatus) >+{ >+ var myAddress = getMsgRecipient(); >+ if (type && gItipItem) { >+ switch (type) { // We set the attendee status appropriately >+ case "ACCEPTED": >+ case "TENTATIVE": >+ case "DECLINED": >+ gItipItem.setAttendeeStatus(myAddress, type); >+ doResponse(); >+ break; >+ case "REPLY": >+ case "PUBLISH": >+ doResponse(eventStatus); >+ break; >+ default: >+ // Nothing -- if given nothing then the attendee wishes to >+ // disregard the mail, so no further action required >+ break; >+ } >+ } >+ >+ finishItipAction(type, eventStatus); This isn't really the right place for this call; in the default case, it should indeed be called right away, but in any of the cases that call doResponse, it shouldn't be called until after the last callback from calendar.addItem() has returned (i.e. in that listener). >+/** >+ * doResponse performs the iTIP action for the current iTIPItem that we >+ * parsed from the email. >+ * Takes an optional parameter to set the event STATUS property >+ */ >+function doResponse(eventStatus) >+{ >+ // XXX For now, just add the item to the calendar >+ var cal = getTargetCalendar(); >+ var item = gItipItem.getFirstItem(); >+ while (item != null) { Do we really want to add all of the items in the message, rather than just the first? >+ item.status = eventStatus; I bet this will produce a JS string warning for the case when doResponse is called without a parameter. >+imipBarText=This message contains an invitation to a calendar event. I'd say that the word "calendar" is redundant here.
Attachment #236985 - Flags: second-review-
Whiteboard: [cal-ui-review+][l10n impact][needs review dmose] → [cal-ui-review+][l10n impact][needs updated patch]
Carried forward Matt's review, since this addresses his remaining comments.
Attachment #236985 - Attachment is obsolete: true
Attachment #237268 - Flags: second-review?(dmose)
Attachment #237268 - Flags: first-review+
Attached patch patch v.next (obsolete) — — Splinter Review
This patch fixes an errant initialize call and a DTD entity capitalization error, but it's still not actually causing the IMIP bar to pop up for me.
Attachment #237268 - Attachment is obsolete: true
Attachment #237268 - Flags: second-review?(dmose)
I've landed the strings here so that we can declare string freeze. Still working out some logistical kinks in the remaining code.
Whiteboard: [cal-ui-review+][l10n impact][needs updated patch] → [cal-ui-review+][needs updated patch]
Attached patch patch, v.n+1 (obsolete) — — Splinter Review
Patch with some capitalization tweaks and added vertical whitespace.
Attachment #237341 - Attachment is obsolete: true
Attachment #237367 - Attachment is obsolete: true
Attachment #237404 - Flags: second-review?(dmose)
Attachment #237404 - Flags: first-review+
Comment on attachment 237404 [details] [diff] [review] Patch to address the race condition issues I've added some vertical whitespace in a few places to make things easier to read. >+*/ >+const onItipItem = { >+ observe: function observe(subject, topic, state) { I've changed this line to give a more unique local function name: observe: function onItipItem_observe(... This way, if that function name shows up in a stack, it's possible to do a search with lxr and figure out exactly where to look. I also wrapped a few lines at 80 columns and made the tweaks we discussed in IRC. In general, looks great; nice work on the comments in particular. r2=dmose
Attachment #237404 - Flags: second-review?(dmose) → second-review+
Attached patch patch, v9 — — Splinter Review
Patch with tweaks, ready for landing.
Attachment #237421 - Flags: second-review+
Attachment #237421 - Flags: first-review+
Checked in to the trunk and branch with cross-commit. Thanks for the patch, Clint!
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Whiteboard: [cal-ui-review+][needs updated patch] → [cal-ui-review+]
Blocks: 59630
Component: Lightning Only → E-mail based Scheduling (iTIP/iMIP)
QA Contact: lightning → email-scheduling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: