To implement more itip features (reply with comments, counter, delegation,...), we will need the imip toolbar to support more actions. To not extend the number of buttons too much, we need also differnt types of buttons (similar to the TB message pane buttons just below the imip bar) to provide more than one action. I'm currently working on a patch, that will provide three types of buttons: - a simple button as we have today - a dropdown button, which unfolds a menu list of options - a smart button combining a clickable button and the dropdown feature The attached screenshots illustrate what I currently have (button text and icons are dummies). The style simply follows the TB button style of the applied theme by referring to the TB css definitions - only the buttons icons are defined separately. Calendar's today implementation creates the button dynamically based on a basic pre-defined XUL element. Do want want keep this or is a fully XUL implementation preferred? I also thought about moving the buttons from the imiptoolbar to the TB message header pane (adding to and/or replacing of existing, mail specific buttons). Is it worth to investigate further into this? Or maybe in a second step? I would appreciate some feedback on this and incorporate it before submitting the patch.

I think a button type="menu-button" could work well for "Accept" and "Decline". Then it's easy to press only the button for this two actions and all is done. For Tentative and "...with Comment" I think placing them into buttons menu is also valid as they need more thinking about the reply and a second click to access the needed action doesn't hamper the user a lot. The more button should be only a type="menu" button because there is no default action on it. And please don't use a ellipsis on More because the user is aware (thanks to the dropdown-arrow on the right) of something opening by clicking the button.

I agree with Richard that a button with type="menu-button" works for Accept/Decline and a button type="menu" should work for the "More" button. I don't think we should move the buttons into the message header toolbar because they are context only for certain types of emails. This would mean either buttons that become invisible for non-invitation mails (ui jumping), or buttons that are disabled but visible in most cases (clutter). Another thing, Thunderbird recently switched to a new notification bar in bug 562048. I don't know if its worth it, but maybe we could switch to using this bar? This might not be so great for my next proposal, but who knows when we will have time for it: I would love to see a more elaborate scheduling view for invitations. It could have a number of cool features, for example notifications if the event overlaps with existing events, or even a full scheduling view that shows you where the request would show in your calendar and allowing you to COUNTER by moving the event around. The scheduling view could show over the email text when the "More" button is clicked. This is a major change though and might be too much at once. It might also require some special casing, i.e when extensions like Thunderbird Conversations is used. It should also be in chrome, not content, to avoid security issues.

Thank you for your comments. I will come back with a first patch soon and also take a look at the new notificationbar. I like Philipp's proposal to implement an extended invitation view, but I suggest to create a solution that would work provider independed and can be used as an overlay either on e-mail and caldav inviations. I guess we can use bug 458578 to discuss this further.

I knew I had a bug filed for that, thanks for the reference.

The attached patch provides the additional support for the menu and menu-button type buttons and is intended to collect a first feedback before submitting the final patch for review - so any comments are welcome. The entire set of buttons is definied in XUL, while showing the required ones is handled in JS. See also the comment inside the XUL overlay file. In edge cases, where dropdown style buttons don't make sense because there are no or only duplicating menitems inside, the button type will be automatically hidden or set back to simple buttons. The patch actually contains already buttons and strings that will be required for upcoming features (like counter, delegate, comment on reply, save a copy, different handling for recurring events), although these are currently not effective. If preferred, these may be removed and added to the respective feature patches later on. The patch has still one issue with the button style (buttons are vertically stretched to the full height of the imip-bar - all this XUL stuff is a little bit tricky...). While already defined icons for accept, decline and tentative buttons are used, some other buttons might deserve icons as well. I would like this to be handled in a separate bug, though.

The first view looks good. When you add labelalign="end" to imip-view-toolbox, then the icons and text are horizontally aligned.

Comment on attachment 8402205 [details] [diff] [review] ReworkImipBarButtons-V1.diff Review of attachment 8402205 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to MakeMyDay from comment #5) > The patch actually contains already buttons and strings that will be > required for upcoming features (like counter, delegate, comment on reply, > save a copy, different handling for recurring events), although these are > currently not effective. If preferred, these may be removed and added to the > respective feature patches later on. I'd prefer if these could be added in the respective feature patch. I saw a few buttons that are put into the disabled state, does this cover all of them? Do you think it would make more sense to hide anything that is not possible for the current invitation? After all, this is not something that will change without selecting a new email and the display changing substantially anyway. > > The patch has still one issue with the button style (buttons are vertically > stretched to the full height of the imip-bar - all this XUL stuff is a > little bit tricky...). This is normal, it should be fine when you set labelalign=end as Richard suggested. Or did you want the bar to look differently? Usually the flex attribute is responsible for this, you might get away with setting margins if you want to make any further changes. > While already defined icons for accept, decline and tentative buttons are > used, some other buttons might deserve icons as well. I would like this to > be handled in a separate bug, though. Sounds fine to me.

The updated patch fixes the button stretching issue as proposed and has removed the parts for the not yet implemented features (XUL, css, strings). The distinction respective recurring events is kept as well as the two ways to accept tentatively (together with the structure of the imipMoreButton as a guideline for the upcoming implementations). > I saw a few buttons that are put into the disabled state, does this cover all of > them? Do you think it would make more sense to hide anything that is not possible > for the current invitation? In fact, these are hidden and not disabled. The naming of the var was misleading here. >After all, this is not something that will change without selecting a new email >and the display changing substantially anyway. The way to deal with removing the imipbar if not needed has basically not changed from the current implementation. Only the reset of the menuitems inside a button was added. To address this, some function/var names are changed to be more precise what data/functionality they provide. I will file a follow up bug for the icons once this is checked in. Is there anybody who I can point to that bug then? Personally, I'm absolutely free of talent in graphic design ;-) btw: Is there any reason why the lightning theme stuff for Windows is in base and for Mac/Linux in lightning?

(In reply to MakeMyDay from comment #8) > > btw: Is there any reason why the lightning theme stuff for Windows is in > base and for Mac/Linux in lightning? It's because for Windows there are actually two themes (XP and Aero) with shared Windows specific styles (which are still at the correct place). And to not make it more complex like it is now to split in Calendar- and Lightning directories in manifest file I decided to package them together.

Comment on attachment 8402301 [details] [diff] [review] ReworkImipBarButtons-V2.diff Review of attachment 8402301 [details] [diff] [review]: ----------------------------------------------------------------- Note that comm-central is on version 31, which means that whatever we push until the next merge (April 29th I think) will be part of the next major release. l10n strings are always frozen on -aurora and -beta, but we can still do bugfixes down to beta, and of course chemspill 3.3.x releases. I'm a little reluctant to add this before the merge because there are many edge cases in invitations and we have next to no tests that cover these cases. I don't want to hold off this functionality longer than needed, but if we do push this we need to make dead sure that it works in all cases. Postponing this to after the merge on the other hand means that it will not be part of TB31's Lightning release, which means the next major release this is in will be TB38. It will be available in all beta releases though. We have applications for a GSoC Project that will hopefully provide us with a good set of tests for invitations, and this would give us a lot of time to fill in features like delegation. What do you think? I'm setting r- for now to ensure we've discussed this enough and because there are review comments anyway. ::: calendar/base/themes/windows/win-aero/lightning.css @@ +61,5 @@ > list-style-image: url(chrome://lightning/skin/imip-aero.png); > -moz-image-region: rect(0px 48px 16px 32px); > } > + > +/* These buttons may also deserve an icon: Go ahead and remove this comment in all themes and put it into the followup bug description instead. ::: calendar/lightning/content/imip-bar-overlay.xul @@ +61,5 @@ > + - * must have type=menu-button > + - * should have a menupopup with at least one menuitem > + - * bubbling up of events from menuitems to toolbarbutton > + - can be prevented by using event.stopPropagation() in > + - menuitem's oncommand attribute This can also be solved by checking event.target / event.originalTarget in the event of the parent, then you don't need it in each child item. @@ +75,5 @@ > + defaultmode="full" > + inlinetoolbox="true"> > + <toolbar id="imip-view-toolbar" class="inline-toolbar" align="start" > + customizable="false" mode="full" > + defaulticonsize="small" defaultmode="full"> Sorry if I missed this before, but do we need a full toolbox here? Afaik they are used for buttons meant to be removable. @@ +100,5 @@ > + tooltiptext="&lightning.imipbar.btnUpdate.tooltiptext;" > + class="toolbarbutton-1 msgHeaderView-button imipUpdateButton" > + oncommand="ltnImipBar.executeAction('')" > + hidden="true"/> > + Trailing whitespace issues in this file. @@ +106,5 @@ > + <toolbarbutton id="imipDeleteButton" > + label="&lightning.imipbar.btnDelete.label;" > + tooltiptext="&lightning.imipbar.btnDelete.tooltiptext;" > + class="toolbarbutton-1 msgHeaderView-button imipDeleteButton" > + oncommand="ltnImipBar.executeAction('')" Why is executeAction empty here? There are a few more of these buttons. @@ +138,5 @@ > + <!-- add here more menuitem as needed --> > + </menupopup> > + </toolbarbutton> > + > + <!-- accept recurrences --> So we have buttons to accept single occurrences instead of the master event now? Is it possible to accept a single occurrence even if the master item hasn't been added to the calendar yet? What about providers that don't allow differentiating between accept and accept all? I believe its possible that the itip message contains multiple occurrences to accept. Would "Accept" still accept all the occurrences in the itip message? ::: calendar/lightning/content/imip-bar.js @@ +120,5 @@ > + let buttons = []; > + let nl = document.getElementById("imip-view-toolbar") > + .getElementsByTagName("toolbarbutton"); > + if (nl != null && nl.length > 0) { > + for (i=0; i<nl.length; i++) { for (let button of nl) should work for node lists @@ +152,5 @@ > + */ > + conformButtonType: function ltnConformButtonType() { > + // check only needed on visible and not simple buttons > + let buttons = ltnImipBar.getButtons() > + .filter(function(aElement){return (element.hasAttribute("type") && !aElement.hidden)}); filter(function(aElement) element.hasAttribute("type") && !aElement.hidden); At least s/element/aElement/ @@ +155,5 @@ > + let buttons = ltnImipBar.getButtons() > + .filter(function(aElement){return (element.hasAttribute("type") && !aElement.hidden)}); > + // change button if appropriate > + for (let button in buttons) { > + let items = ltnImipBar.getMenuItems(button).filter(function(aItem){return (!aItem.hidden)}); .filter(function(aItem) !aItem.hidden) You can remove the {} and return if you want a function that returns the result of the body. You can make use of this further in the file too.

(In reply to Philipp Kewisch [:Fallen] from comment #10) > Note that comm-central is on version 31, which means that whatever we push > until the next merge (April 29th I think) will be part of the next major > release. l10n strings are always frozen on -aurora and -beta, but we can > still do bugfixes down to beta, and of course chemspill 3.3.x releases. I'm > a little reluctant to add this before the merge because there are many edge > cases in invitations and we have next to no tests that cover these cases. I > don't want to hold off this functionality longer than needed, but if we do > push this we need to make dead sure that it works in all cases. > > Postponing this to after the merge on the other hand means that it will not > be part of TB31's Lightning release, which means the next major release this > is in will be TB38. It will be available in all beta releases though. We > have applications for a GSoC Project that will hopefully provide us with a > good set of tests for invitations, and this would give us a lot of time to > fill in features like delegation. > > What do you think? I'm setting r- for now to ensure we've discussed this > enough and because there are review comments anyway. The patch doesn't change as much as it's size may suggest. Mostly, it makes button definition static and adds the handling for menuitems inside of buttons. The hiding buttons and imipbar as well as processing the action after pressing a button works the same as before. I don't think it will break anything but would provide a base for some patches I'm currently working on. So, I pled for checking in this once the review is passed, but it's your decission at the end. I will provide a patch with your comments considered anyway. > > ::: calendar/lightning/content/imip-bar-overlay.xul > @@ +61,5 @@ > > + - * must have type=menu-button > > + - * should have a menupopup with at least one menuitem > > + - * bubbling up of events from menuitems to toolbarbutton > > + - can be prevented by using event.stopPropagation() in > > + - menuitem's oncommand attribute > > This can also be solved by checking event.target / event.originalTarget in > the event of the parent, then you don't need it in each child item. > > @@ +75,5 @@ > > + defaultmode="full" > > + inlinetoolbox="true"> > > + <toolbar id="imip-view-toolbar" class="inline-toolbar" align="start" > > + customizable="false" mode="full" > > + defaulticonsize="small" defaultmode="full"> > > Sorry if I missed this before, but do we need a full toolbox here? Afaik > they are used for buttons meant to be removable. I used this mainly to participate of the button style definitions of Thunderbird to visually integrate nicely - that way the buttons have the same style as the other msgHeader buttons without the need to define it separately. A short test with buttons only falls (partly) back the current UX. So I suggest to leave it with the toolbar, even it is defined not customizable (I even thought about making it customizable but left this out for now). > Why is executeAction empty here? There are a few more of these buttons. This is to provide the partstat. For action that don't need / rely on partstat, this is empty. Exception to this rule is handing over X-SHOWDETAILS to open the event dialog. So everything as of today. > So we have buttons to accept single occurrences instead of the master event > now? Is it possible to accept a single occurrence even if the master item > hasn't been added to the calendar yet? What about providers that don't allow > differentiating between accept and accept all? > > I believe its possible that the itip message contains multiple occurrences > to accept. Would "Accept" still accept all the occurrences in the itip > message? No, this will not provide additionally functionality in that direction. As of now it just distinguishs whether the event is a recurring one and provides a more clear button (=accept/decline/tentative all recurrences) in that case. Later on this button may be used to offer a feature to decide on each recurrence separately. But that would be another bug. With this patch, the operation will be operated on the same occurences as of today.

(In reply to MakeMyDay from comment #11) > The patch doesn't change as much as it's size may suggest. Mostly, it makes > button definition static and adds the handling for menuitems inside of > buttons. The hiding buttons and imipbar as well as processing the action > after pressing a button works the same as before. I don't think it will > break anything but would provide a base for some patches I'm currently > working on. So, I pled for checking in this once the review is passed, but > it's your decission at the end. Ok, it did indeed look like more than I thought, your reply resolves some of my concerns. > I used this mainly to participate of the button style definitions of > Thunderbird to visually integrate nicely - that way the buttons have the > same style as the other msgHeader buttons without the need to define it > separately. A short test with buttons only falls (partly) back the current > UX. So I suggest to leave it with the toolbar, even it is defined not > customizable (I even thought about making it customizable but left this out > for now). Ok, lets leave it this way. > > Why is executeAction empty here? There are a few more of these buttons. > > This is to provide the partstat. For action that don't need / rely on > partstat, this is empty. Exception to this rule is handing over > X-SHOWDETAILS to open the event dialog. So everything as of today. Can we just pass an empty argument in those cases? i.e bar.executeAction() ? > No, this will not provide additionally functionality in that direction. As > of now it just distinguishs whether the event is a recurring one and > provides a more clear button (=accept/decline/tentative all recurrences) in > that case. Later on this button may be used to offer a feature to decide on > each recurrence separately. But that would be another bug. With this patch, > the operation will be operated on the same occurences as of today. Great, thanks for the clarification!

Updated patch considering comments from the last review and enables passing empty arguments to ltnImipBar.executeAction()

Comment on attachment 8402399 [details] [diff] [review] ReworkImipBarButtons-V3.diff Review of attachment 8402399 [details] [diff] [review]: ----------------------------------------------------------------- r=philipp with these comments considered: ::: calendar/base/modules/calItipUtils.jsm @@ +273,5 @@ > } else if (actionFunc.method == "REQUEST:NEEDS-ACTION") { > data.label = _gs("imipBarProcessedNeedsAction"); > } > > + if (itipItem.getItemList({})[0].recurrenceInfo) { I know we use this in another place in our code too, but in theory, I think its possible that there are multiple items and the one with the recurrence information is not the first one? Also it might be possible that the item list is empty, although admittedly that is quite an edge case and rather a faked itip email created to produce a client error. ::: calendar/lightning/content/imip-bar-overlay.xul @@ +161,5 @@ > + <!-- add here more menuitem as needed --> > + </menupopup> > + </toolbarbutton> > + > + <!-- tentative; should only be used, if no imipMoreButton is used and Whitespace issue @@ +179,5 @@ > + <!-- add here more menuitem as needed --> > + </menupopup> > + </toolbarbutton> > + > + <!-- tentative recurrences; should only be used, if no imipMoreButton is used and Whitespace issue ::: calendar/lightning/content/imip-bar.js @@ +130,5 @@ > + > + /** > + * Provides a list of available menuitems of a button > + * > + * @param aButton button node Whitespace issue

> I know we use this in another place in our code too, but in theory, I think > its possible that there are multiple items and the one with the recurrence > information is not the first one? > > Also it might be possible that the item list is empty, although admittedly > that is quite an edge case and rather a faked itip email created to produce > a client error. As infact only the first item in list will be processed, I think the chekc here is correct for now. Once we made the code to process all contained items, this should be changed - even though even in a multiple item case all items need to have the same UID and therefore can only be different instances of the same event, I think it's expremely unlikely that on of them is a recurring item (if the orginating calendar agent is able to produce recurring events, why should it send out multiple instances of the same event?). This said, I will leave this as is unless you disagree and provide a patch with fixing the whitespace issues.

Patch fixing whitespace issues.

(In reply to MakeMyDay from comment #15) > even though even in a multiple item case all > items need to have the same UID and therefore can only be different > instances of the same event, I think it's expremely unlikely that on of them > is a recurring item I was thinking this could be the case if the invitation contains the master item of a recurring meeting, plus a few changed instances of the same event. What happens if the exceptions are listed first?

Updated patch with improved distinction for recurring events / recurrences.

Comment on attachment 8410185 [details] [diff] [review] ReworkImipBarButtons-V5.diff Review of attachment 8410185 [details] [diff] [review]: ----------------------------------------------------------------- r=philipp with this minor change: ::: calendar/base/modules/calItipUtils.jsm @@ +274,5 @@ > data.label = _gs("imipBarProcessedNeedsAction"); > } > > + let isRecurringMaster = false; > + for (let item in itipItem.getItemList({})) { getItemList returns an array, if you do for..in on that then it will iterate the array indices. You want this: let itemList = itipItem.getItemList({}); for (let item of itemList) { ... } if (itemList.length > 1) { ... }

Updated patch with comment considered.

Several times strings of this patch talk of "event participation" where I would have expected "event invitation" (accept, decline etc.). Just to be sure: This is on purpose?

No it's not. Probably "event invitation" is more precise. There is another issue with this strings, see bug 1001831. We can take that bug to follow up with string correction.