920285 - Add UI to set category for received invitations

Status: NEW

(Calendar :: General, enhancement)

Description

[Mauro Cicognini]

Categories are local data that can be set on various items, including calendar events.

When adding an event, the UI offers a "Category" dropdown, that allows to optionally add one or more Category labels to the event. These labels are not propagated when the event is sent to other users.

It should be possible to set a Category on events received by others, too.

Outlook users can do this for any event, regardless of its origin.

Please also note that if the Category can be set otherwise (such as using GMail, if the calendar is a Google Calendar), then it is correctly displayed.

ACTION TO REPRODUCE
1. Double click on a calendar entry generated by accepting an invite
2. Examine the UI for a "Category" dropdown box.

EXPECTED OUTCOME
UI is available and allows to set (optionally) one or more Category labels.

ACTUAL OUTCOME
UI is absent.

Comment 1

[PaulB]

I am working on this bug. I have confirmed personally that this is a bug or rather a feature request because there has been historical intense debate about weather or not allow "editing" of received invitations. I agree that setting the category is technically editing the invitation, but only making a local personal categorization for that invitation. As stated by the reporter this feature is available in Outlook and I would like to see it available here as well.

Comment 2

[PaulB]

Attachment: diff patch for fixing the issue. actually created from the build, but only two files were changed which in the sources are located in comm-central/calendar/base/content/dialogs/

Added the UI to make the category in received invitation editable by copying the necessary components/functions from calendar-event-dialog to calendar-summary-dialog.

Comment 3

[PaulB]

review ping [e-mail]?

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Hey Paul, sorry for the delay. A lot has been going on, especially with the chemspill release for the Provider for Google Calendar I need to work on. I hope to get some eyes on this by Monday, feel free to ping me again if I forget.

Comment 5

[PaulB]

review ping [e-mail] (last ping was 2014-10-09 - 13 days ago)

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Ok, lets do it. I'm taking a look at your patch now.

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8496292 [details] [diff] [review]
diff patch for fixing the issue. actually created from the build, but only two files were changed which in the sources are located in comm-central/calendar/base/content/dialogs/

Review of attachment 8496292 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, part 1 is a code review. The code looks very high quality. I usually have style nits things to complain about, but in this case I don't! Thank you so much :-)

I am currently rebuilding and will take a look at the UI afterwards. The only concern I have here, and this is one I've always had with support for setting reminders in the summary dialog, is that making this change will change the item, so if you send back a reply to the organizer it will include these changes. I believe some servers respond that the change is not allowed. On the other hand, I think we need a more general solution for this anyway and won't turn down the patch because of this. I'm still asking feedback from MakeMyDay, who is our expert on invitations.

Next up I will try the patch in my build. I have to wait until it updates though, should be done in the next hour or two. I'll comment again then and set ui-r. In the meanwhile, r+ with comments considered:

::: chrome/calendar/content/calendar/calendar-summary-dialog.js
@@ -111,5 @@
>  
> -    var categories = item.getCategories({});
> -    if (categories.length > 0) {
> -        document.getElementById("category-row").removeAttribute("hidden");
> -        document.getElementById("item-category").value = categories.join(", "); // TODO l10n-unfriendly

Although maybe not ideal from a l10n standpoint, I think its ok. I don't recall any bug reports saying that comma separation doesn't work in a locale. Go ahead and remove the TODO comment.

@@ +184,5 @@
> +
> +    // Make sure the maximum number of categories is applied to the listbox
> +    let calendar = getCurrentCalendar();
> +    let maxCount = calendar.getProperty("capabilities.categories.maxCount");
> +    categoryPanel.maxCount = (maxCount === null ? -1 : maxCount);

Are you sure that getProperty returns null when its not set? I thought it would return undefined?

You could check (typeof maxCount == "number" ? maxCount : -1);

@@ +191,5 @@
> +    // Hide the categories listbox and label in case categories are not
> +    // supported
> +    setBooleanAttribute("item-categories", "hidden", (maxCount === 0));
> +    setBooleanAttribute("item-categories-label", "hidden", (maxCount === 0));
> +    setBooleanAttribute("item-calendar-label", "hidden", (maxCount === 0));

So this means only hide if maxcount is a number and its 0 ? This means that the aux label would show if there are multiple categories, and if the maxcount is undefined?

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8496292 [details] [diff] [review]
diff patch for fixing the issue. actually created from the build, but only two files were changed which in the sources are located in comm-central/calendar/base/content/dialogs/

Ok, I've tried out the patch. I noticed the category dropdown is also shown when opening a readonly event. The summary dialog is shared between invitations and events on readonly calendars, so you will have to hide the categories box for readonly calendars. The reminders dropdown does this too.

Setting ui-r- since the UI is not quite there yet. This is just a minor thing, so don't take the "-" too strongly.

Comment 9

[[:MakeMyDay]]

Comment on attachment 8496292 [details] [diff] [review]
diff patch for fixing the issue. actually created from the build, but only two files were changed which in the sources are located in comm-central/calendar/base/content/dialogs/

First of all welcome to Paul and thank you for providing this patch.

@Phillip:
The RfC prohibits to change properties like category in the reply (see see http://tools.ietf.org/html/rfc5546#section-3.2.3). This is probably one reason, because the event summary dialog provides only few abilities to change event details. So, f=- for this approach.

If we want to provide more options to the user with this, I see basically three options:

1) We use x-props to store the respective values to the event, which then get stripped from the ics before sending the reply. This would us require to read the event properties primarily from those x-props when initializing the event from ics with fall back to the original properties in case the respective x-prop don't exist.

I'm actually not quite sure if we can wrap this or have to implement this two-step logic at several places, so I see at least a potential for regressions here.

2) We use x-props to duplicate the original values in the ics, use the standard props for storing and restore the original props in case of sending out the reply.

This approach would be less probable to cause regressions in Lightning, because the ics parser can work as a wrapper and we don't need to change any usage of props elsewhere. However, there might be flaws in a multi-client network calendar scenario, because another client does not know about this logic and may send out an invalid reply when used instead of Lightning.

3) We store these infromation separately from the ics.

This would probably require the most work and is also a candidate for performance issues, so I'm also not sure on this.

So, unfortunately I'm not really assuming a straight forward way to go here, but maybe investigating on option #1 would turn out it is.

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Hmm I'm not too happy with #1 and #2 because x-props are just workarounds. I guess the best solution would be to devise a system that allows storing local data on top of our event data, which we could also use for local alarms. That itself is unfortunately a project that might fill a whole summer.

Paul, I don't know how much motivation you are bringing in, but maybe you are interested in #3? Matthew who I've CC'd, had been working on the local alarms feature. I'm sure he would assist you in designing the feature. MakeMyDay and me are also around for questions and discussions.

If this is too much at once, I guess option #1 would work out as long as there are tests. I wonder how we do this for alarms? Does it work because they are X-props and are stripped?

Comment 11

[[:MakeMyDay]]

Alarms are not an issue here, because they must not be returned within the reply, see also the link I posted above.

Comment 12

[PaulB]

Hi guys thanks a lot for your comments.
<rantWarning:on/>
I must admit this is a bit of an interesting experience. I want to donate my free time as developer to improve a favorite free product that I use but this is not a positive experience at all. First it takes forever to get a review for something (and even before that one has to search forever for instructions on how to submit a patch here and find someone that can review it (plus you have to find a person for review yourself even when you are completely new to a project...)).
And second, a feature implementation is made impossible by a nit-picking review and RFC-conformance nit-pickery.
I still want to see this thing in lightning so I will keep going, but my motivation is not increasing... 
<rantWarning:off/>


I just want to be able to set a category on event invitations to color them accorindly in my calendar - why is this such a pain...??

Well yes the RFC states that a category must not be changed in the reply.
a. But there is no problem if the event is first accepted, the reply is sent out and then the category is edited by the user (which is the main use case I anticipate) (as in: the modified categories are never sent out).

b. Can we not stripp off the categories in the created ICS-reply as we do with the alarms? This would be a quick suitable solution. I can do this but I need a pointer to the code where this takes place since I have no clue where in the code this would be hidden...

c. Any of the above suggestions (1,2,3) go way beyond what I expected to do here. I am not a JS guy, nor there seem to be good IDEs for JS development and especially it is a pain to debug the JS within thunderbird.

So, if b. cannot be implemented or you find it to be an unsuitable suggestion, then I guess I will create my own plugin for lighting which modifies the invitations as submitted here (alike the CalendarTweaks plugin) with the disclaimer that use of the plugin might not be RFC-conforming in some cases...

Comment 13

[[:MakeMyDay]]

Hi PaulB,

first of all, we didn't want to discourage you to contribute to Thunderbird or Lightning. You're rather welcome! I agree this is not the best experience when trying to submit a patch for the first time. And your right, it probably could be easier for new contributors to onboard. We discussed this recently already.

I really appreciate, that you don't want to get detained by these obstacles and still want to move forward here. Unfortunately, you have selected a subject which cannot resolved by some minor changes as you initially expected.

RfC conformance is key for calendaring and especially for invitation support - otherwise interoperability between different calendar clients will stop working, which would result in much more complaints than not beeing able to set a custom category. The same applies on the feature implemtation itself - if a review turns out there are loose ends, things should be fixed to get the approval - this is the only way we can keep a certain level of quality in the code base. It may look like nit-picking at a first glance, but this is not to distract you, but to keep quality and stability when new features get in. Regression fixing is much more more pain than delivering in quality upfront.

The root cause for not supporting more local changes is the current architecture. Basically, at the moment we do not distinct between a local version of an event and the ones which came in or went out by invitations. We only have the single instance of an event at a time. That said, any change on an received event would also be applied, once there is an intercation with the organizer in future.

This is also the reason, why option a does not work. If, after changing the category, you change your mind (and your partstat accordingly) or the organizer requests a REFRESH, the organizer gets the category modification with the new reply, too. Also, any update by the organizer would override the category modification.

For option b, this is basically the same - removing a property is a change as well. For alarms, this is different, because the alarm are and must be stripped on the reply to conform the RfC - so alarms and categories are not comparable here, even it they look like from a users perspective.

@Philipp: maybe it's worth to discuss RfC interpretation on option b for dealing with optional properties with other vendors on calconnect

I can understand if you don't feel comfortable to drive option 3 from comment #9 for a first patch - but I think this is the most reasonable way to go here to really solve the problem at the moment. Providing this as an add-on is up to you, although breaking RfC conformance should also be avoided by additional add-ons for Lightning, imho.

Maybe you want to start with more easy bugs instead and come back to this one later? If so, take a look at Thunderbird / Calendar bugs, which have "[good first bug]" whiteboard entry in bugzilla. If you require assistance for a specific calendar bug, you can ping the one who set the according whiteboard entry or also Philipp or me. If you're not the JS guy as you mentioned, contribution in other areas is also highly appreciated: bug triaging, themes (mostly CSS and icons) or improving documentation (maybe on how to onboard as contributer for Thunderbird /Lightning?).

For whatever option you go, we really appreaciate your contribution of any kind!

Comment 14

[PaulB]

Hi MakeMyDay,

thank you for your considerate and quick reply - I really appreciate it. You made my day :) 

Today may not have been the best day of my life and this is maybe why the ranting became what it was in my previous comment - I am sure I could have found more positive words (just, not today) and for that I am sorry, yet in essence I stand by what I ranted.

I do understand your desire and need for code quality and yes you are right the earlier one fixes bugs or removes smells the lesser effort is required. I once heard the measure 1:10 for fixing bugs before (close to coding) : after production release.

Okay if RfC conformance is crucial (which I totally understand from an interoperability p.o.v. - see the mess WS-x and SOAP specs produced), then I understand.

Concerning your suggestion for discussion on Option b. I did think about how does MS Outlook do it - would they just forward the category if the user set it. I did want to try it out, but did not wanna go through the hassle of installing a who Office Suite just to try out that single fact.


If the calendar event is a single instance of the one received then of course any changes to it would reflect back to any forwards or updates or replies. So I am a Java guy and in OO I would just solve this problem by creating an inheriting class of that calendar class which you can change freely locally, but when "serialized" back into an ICS-reply only the data of the mother object is seen, i.e. the unmodified data. I don't even know if there is such a thing as "class" and "object" in JS... (sorry for this noob-question :)


It's not that I am not open for learning...
Could you point me to a good (if any) Integrated Development Environment for JS preferably including code-completion for Thunderbird-JS, the Thunderbird-JS-specific libs and imported JS-scripts? Iow. how do you develop and debug the JS in Thunderbird? This would help me a lot and speed-up the process for me.

Comment 15

[[:MakeMyDay]]

Great to read you are still interesset to go ahaed!

> Concerning your suggestion for discussion on Option b. I did think about how
> does MS Outlook do it - would they just forward the category if the user set
> it. I did want to try it out, but did not wanna go through the hassle of
> installing a who Office Suite just to try out that single fact.

Outlook has its own way to store elements, this is not reltated to the way Lighnting does. So, there are probably more options do do things like you intended with this bug.

> If the calendar event is a single instance of the one received then of
> course any changes to it would reflect back to any forwards or updates or
> replies. So I am a Java guy and in OO I would just solve this problem by
> creating an inheriting class of that calendar class which you can change
> freely locally, but when "serialized" back into an ICS-reply only the data
> of the mother object is seen, i.e. the unmodified data. I don't even know if
> there is such a thing as "class" and "object" in JS... (sorry for this
> noob-question :)

Overriding is also what may be the way to go here. The problem is not that we cannot, but that there is currently no place to persist any overlaying modifications. And to get rid of this limitation is basically what option 3 from above is about.

> It's not that I am not open for learning...
> Could you point me to a good (if any) Integrated Development Environment for
> JS preferably including code-completion for Thunderbird-JS, the
> Thunderbird-JS-specific libs and imported JS-scripts? Iow. how do you
> develop and debug the JS in Thunderbird? This would help me a lot and
> speed-up the process for me.

Regarding this, I will write you a separate e-mail (but not right now).

Comment 16

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to MakeMyDay from comment #13)

> @Philipp: maybe it's worth to discuss RfC interpretation on option b for
> dealing with optional properties with other vendors on calconnect

Given this is just a workaround for storing data locally, I'm not sure if we have a strong argument for it. I do think we've discussed an overlay system though, but that hasn't gone anywhere.

I just wanted to check in here to see how things are. Are you discussing this via email currently?

Comment 17

[[:MakeMyDay]]

Hi Philipp,

sorry for being irresponsive in the last couple of weeks - my job consumed nearly all of my time.

To answer your question: In short, unfortunately not for the very same reason at least at my end. I'm not sure whether PaulB is still interested to invest some time into this bug, maybe he can post a statement here. Meanwhile, I will follow to answer his question by mail.