404900 - Add Accept/Decline to Calendar item's context menu

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, enhancement)

Description

[Gerald]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.9) Gecko/20071025 Firefox/2.0.0.9
Build Identifier: TB 2.0.0.9 and Lightning .7 and .8pre

Ability to right click on a meeting and choose accept/decline as well as the edit and delete options

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Sven Giermann]

This one gives much room for discussion...
- is this wanted by drivers?
- for Lightning only or Sunbird too (see bug 417429)

Comment 2

[Gerald]

Also, this would very much involve Bugs 404179 and 361635.  Building support here, would likely also result in resolving those issues.

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

I think we should do this. Bryan, what do you think? Also, where in the context menu should it be placed? Should it be disabled or hidden when the event in question is not an invitation?

Comment 4

[Les Barstow]

Philipp - If by "invitation" you mean an event with attendees, then you could probably disable the status change (accept/decline/tentative) context menu.  But if by "invitation" you mean anything more specific, such as limiting it to events created by others, then the answer is IMHO a "no".

I'll add my vote for this enhancement.  Needing to go in to the Edit screen and selecting Options->Status to change your attendance status is a pain.

Comment 5

[Andreas Nilsson (:andreasn)]

The only source I could quickly for this was this MSDN document [1] that seems in favor of hiding the items as they don't apply to items that are not invites.

To avoid things from jumping around too much and doing tricks to your muscle memory I would suggest putting them at the bottom of the menu.

1. http://msdn.microsoft.com/en-us/library/aa511502.aspx#removeDisable

Comment 6

[Andreas Nilsson (:andreasn)]

also, a right click action for this would make these things much more efficient without risking making the right click menu too long (yes, I'm looking at you thread pane right click menu! ;) )

Comment 7

[Andreas Nilsson (:andreasn)]

so ui-r+ on this approach from me

Comment 8

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v1

This patch takes care, carrying forward ui-r+ from Andreas

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Menu Screenshot - v1

Comment 10

[Simon Vaillancourt]

Comment on attachment 506379 [details] [diff] [review]
Fix - v1

Thanks for asking me to review this, I see 3 things that I think are important and are missing from the patch:

1) Ability to reply to all instances of a recurring meeting
2) Ability for the organizer to change his participation status
3) Ability for participation status of tasks to be changed

I will attach screenshots of what we did in the oracle add-on, I've wanted to push these in the Lightning code base for a long time but never found the time. For recurrence meeting reply, we basically allow for the same type of operations as if you were double clicking on an instance, if it's an exception, we change the partstat of the exception only, if it's not an exception, we offer, in the menu, 2 sets of option (All occurrences and one instance only) the screenshots are self explanatory IMO.

Comment 11

[Simon Vaillancourt]

Attachment: sample Ui for replying to recurring meetings and tasks.

Comment 12

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #10)
> Comment on attachment 506379 [details] [diff] [review]
> Fix - v1
> 
> Thanks for asking me to review this, I see 3 things that I think are important
> and are missing from the patch:
> 
> 1) Ability to reply to all instances of a recurring meeting
Hmm, I see. Maybe we can do the UI a bit differently though, I don't quite like the ( All Occurrences ) part. Maybe we can use special menuitems as headings. Not quite the usual context menu, but I think its better than using ()'s. Andreas, could you comment on this?


> 2) Ability for the organizer to change his participation status
Ok, so the same options to select but to change the organizer status. I'll still be hiding the context menu item if the item has no attendees, so its not in the way for users that don't take part in scheduling. Sound good?


> 3) Ability for participation status of tasks to be changed
Doesn't this already work with the patch with tasks in view? I agree "attend" isn't quite the right word for tasks though, and it will likely not work in the task pane. I'll fix this.

Comment 13

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #12)
> > 1) Ability to reply to all instances of a recurring meeting
Would you be ok with postponing this particular feature to after the release? This is basically the last bug before string freeze and I'd hate to hold the string freeze for another cycle of UI decisions and implementation.


> > 2) Ability for the organizer to change his participation status
Done in my local copy of the patch. No Emails appear to be sent when changing the organizer status, but maybe this has other reasons. This is also not a bug in the patch but likely in the itip code and should be fixed separately.

> > 3) Ability for participation status of tasks to be changed
I've tested this, and at least the attendance menu shows up with options like "I will attend". Given we don't support sending tasks via itip, I think its ok to leave the wording as is and fix it in a followup bug.

Comment 14

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v2

Comment 15

[Simon Vaillancourt]

> > > 1) Ability to reply to all instances of a recurring meeting
> Would you be ok with postponing this particular feature to after the release?
> This is basically the last bug before string freeze and I'd hate to hold the
> string freeze for another cycle of UI decisions and implementation.

I have an issue with this for 2 reasons, 1) bad user experience, try replying to a recurring meeting with 10s of instances that way :( . 2) From a server side perspective this is also bad because it will create a multitude of exceptions which are often more intensive to process than a meeting with just a rule.

Could we just keep the current UI and instead re-use the "Edit this occurrence/Edit All occurrences" prompt if the current instance is not an exception? I would be happy with that.

> > > 2) Ability for the organizer to change his participation status
> Done in my local copy of the patch. No Emails appear to be sent when changing
> the organizer status, but maybe this has other reasons. This is also not a bug
> in the patch but likely in the itip code and should be fixed separately.
Agree, and most caldav servers support implicit scheduling nowadays :)

> > > 3) Ability for participation status of tasks to be changed
> I've tested this, and at least the attendance menu shows up with options like
> "I will attend". Given we don't support sending tasks via itip, I think its ok
> to leave the wording as is and fix it in a followup bug.
Agree

I will review again when you reply to my comment on issue #1

Thanks!

Comment 16

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Menu Screenshot - v3

Ok, so styling the menu isn't all that hard. I was able to add labels to the  menu and make it look good, at least on Linux (haven't tried others).

Here's the plan for functionality:

The following cases can be selected. Two means two or more.
  - A recurring event occurrence and a single event --> ???
  - Two single events --> Show options only once (as in Screenshot v1)
  - Two recurring events --> Show both options (as in Screenshot v3)
  
Now to think about what should be shown for mixed single/recurring events?

  - Show only one set of options --> Bad, can't select right for recurring evnts
  - Show both options --> Good, but what happens for single events?

The solution may be that for single events, selecting both "this occurrence/..." and "all occurrences/..." causes the single event status to be changed.

Objections?

Comment 17

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v3

This patch should take care, I'd appreciate some testing on a server that does real invitations.

Comment 18

[Simon Vaillancourt]

Comment on attachment 510019 [details] [diff] [review]
Fix - v3

> The solution may be that for single events, selecting both "this
> occurrence/..." and "all occurrences/..." causes the single event status to be
> changed.

I agree.

For the review I have to r- because:
1) The patch seems to work ok for single events but when I have a recurring event it seems not to be working. I will refresh from comm-central and recompile everything just to be sure the problem is not on my side. 
2) Right clicking on tasks and events on the sidebar should work the same way as in the calendar view and calendar finder IMO, right not it does not. Same goes for task finder.

Test cases for replying to an event and task:
  1) Reply to single event/task
  2) Reply to one occurrence of recurring event/task that has no exception
  3) Reply to one occurrence of recurring event/task that is already an exception
  4) Reply to all occurrences of recurring event/task
  5) Reply to all occurrence of recurring event/task w/ one exception (all partstats but exception should be changed)
  6) Do tests 1 to 5 from sidebar, calendar view, task view, event finder.
  7) Multi-select single, recurring event/task with no exception, recurring meeting with exception, reply all occurrences. make sure all partstats have changed except the exception.
  8) Try bunch of other multi-select possibilities in the event finder, change event finder from "All Events" to "All future events" since they both expose different part of a meeting (master instance vs exceptions). 

Note that we could merge this bug in 2 phases, I think the strings are fine and could be merged right now so we can do the string freeze.

Comment 19

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v4

Ah, I switched the order of if statements in calendar-item-editing.js. We need to check for the invited attendee first, otherwise all events with an organizer that is not yourself will cause no action.

Comment 20

[Philipp Kewisch [:Fallen][📱📆]]

I'm going to check this in so we can string freeze soon. If there are any errors or missing features for this bug, we can take care afterwards.

Simon, I'd still appreciate review, when you have time :)

Comment 21

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/c25fd0381b62>
-> FIXED

Comment 22

[Philipp Kewisch [:Fallen][📱📆]]

Backported to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/b859ddbdb54a>
a=philipp

Comment 23

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Additional patch for task context menu

This patch should be applied in addition to Fix v4, to show the attendance menu also from the task panes.

Comment 24

[Simon Vaillancourt]

Comment on attachment 512041 [details] [diff] [review]
Fix - v4

Approving this but will comment more on the next patch

Comment 25

[Simon Vaillancourt]

Comment on attachment 515512 [details] [diff] [review]
Additional patch for task context menu

This looks good but I think the exit criteria for this patch would be to also have the context menu on the "Today Pane" which is the pane people use for quick access to their calendars.

Comment 26

[Martin Schröder [:mschroeder]]

What about the additional patch that got review-? Reopening to clarify the situation.

Comment 27

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Additional patch for task context/today pane

This takes care of the review comment and also consolidates some duplicated code. Simon, do you have time to review this?

Comment 28

[Martin Schröder [:mschroeder]]

Comment on attachment 538490 [details] [diff] [review]
Additional patch for task context/today pane

Looks good code wise! r=mschroeder with following considered:

[...]

diff -r 5aef837e76ac calendar/base/content/calendar-common-sets.xul
--- a/calendar/base/content/calendar-common-sets.xul	Fri Jun 10 10:39:37 2011 +0200
+++ b/calendar/base/content/calendar-common-sets.xul	Fri Jun 10 14:55:18 2011 +0200

[...]

@@ -425,6 +426,63 @@

[...]

+        <menupopup id="task-context-menu-attendance-menupopup">
+          <label id="task-context-attendance-thisoccurrence-label"
+                 class="calendar-context-heading-label"
+                 scope="all-occurrences"
+                 value="&calendar.context.attendance.occurrence.label;"/>

Should the scope be "this-occurence" as the menuitems?

[...]

diff -r 5aef837e76ac calendar/base/content/today-pane.xul
--- a/calendar/base/content/today-pane.xul	Fri Jun 10 10:39:37 2011 +0200
+++ b/calendar/base/content/today-pane.xul	Fri Jun 10 14:55:18 2011 +0200
@@ -46,6 +46,7 @@
     <!ENTITY % dtd3 SYSTEM "chrome://messenger/locale/messenger.dtd" > %dtd3;
     <!ENTITY % dtd4 SYSTEM "chrome://calendar/locale/calendar.dtd" > %dtd4;
     <!ENTITY % dtd5 SYSTEM "chrome://global/locale/global.dtd" > %dtd5;
+    <!ENTITY % dtd6 SYSTEM "chrome://calendar/locale/calendar-event-dialog.dtd"> %dtd6;

Time for another l10n cleanup and reorganisation? ;)

[...]

@@ -239,15 +214,98 @@

[...]

+                   <menupopup id="calendar-today-pane-menu-attendance-menupopup">
+                     <label id="calendar-today-pane-attendance-thisoccurrence-label"
+                            class="calendar-context-heading-label"
+                            scope="all-occurrences"
+                            value="&calendar.context.attendance.occurrence.label;"/>

Should the scope be "this-occurence" as the menuitems?

[...]

Comment 29

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #28)
> Comment on attachment 538490 [details] [diff] [review] [review]
> Additional patch for task context/today pane
> 
> Looks good code wise! r=mschroeder with following considered:

Martin, thank you very much for your time and review!
 

> +        <menupopup id="task-context-menu-attendance-menupopup">
> +          <label id="task-context-attendance-thisoccurrence-label"
> +                 class="calendar-context-heading-label"
> +                 scope="all-occurrences"
> +                 value="&calendar.context.attendance.occurrence.label;"/>
> 
> Should the scope be "this-occurence" as the menuitems?

Not for the labels. The idea is to hide even the labels ("This Occurrence: " / "All Occurrences:") when its a single event, since there is only one set of menuitems (accept/decline/...) anyway.


>      <!ENTITY % dtd5 SYSTEM "chrome://global/locale/global.dtd" > %dtd5;
> +    <!ENTITY % dtd6 SYSTEM
> "chrome://calendar/locale/calendar-event-dialog.dtd"> %dtd6;
> 
> Time for another l10n cleanup and reorganisation? ;)
Yes, that would be swell :)

Comment 30

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/16cf72def333>
-> FIXED

Comment 31

[Philipp Kewisch [:Fallen][📱📆]]

Backported to comm-miramar <http://hg.mozilla.org/releases/comm-miramar/rev/8264edcd4cf2>