316927 - Multiple categories should be allowed for Events/Tasks

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Jeff]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051104 Mozilla Sunbird/0.3a1

It would be much more useful to allow multiple categories per event/task. I don't know if this is a conflict with the iCal spec or not but if it permits this feature would be great.

Reproducible: Always

Comment 1

[Andrew N Dowden]

My view is that rfc2445 (iCal spec.) allows, and shows examples of multiple categories.

eg. (from 4.6.1)

  CATEGORIES:ANNIVERSARY,PERSONAL,SPECIAL OCCASION

Comment 2

[Matthew (lilmatt) Willis]

Reassigning all automatically assigned bugs from Mostafa to nobody@m.o

Bugspam filter: TorontoMostafaMove

Comment 3

[Omar Bajraszewski]

But then it won't be possible to color categories...

Comment 4

[Joey Minta]

*** Bug 362174 has been marked as a duplicate of this bug. ***

Comment 5

[Eugene]

"------- Comment #3 From Omar B. 2006-08-22 09:29 PST  [reply] -------  
But then it won't be possible to color categories..."

It will still be possible.  You just flip the order of assigning the colors from what it is now.  Pick a color first then choose one or more categories for that color.  For example
Blue   = ANNIVERSARY
Red    = PERSONAL
Purple = ANNIVERSARY & PERSONAL
Green  = SPECIAL OCCASION
Yellow = ANNIVERSARY & PERSONAL & SPECIAL OCCASION

When assigning the color when drawing the calendar the color rules will need to be searched to see if the categories match one of the rules and then assign the color.
Most PIM's allow the assigning of colors to categories and the assigning of multiple categories to entries but then simply assign the color to the first category it finds in the color list.  I would like to see one step better by allowing assigning a color to more than one category and the multiple categories will be AND'ed to assign like the example above.  See my description in 362174 for how this could work as I didn't find this bug when I searched.

Comment 6

[Vladimír Solnický]

Bug 357339 is a possible functional duplicate.

Comment 10

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

Due to the fact that we only support one category, we also loose the remaining categories on import/event dialog edits. This should be fixed together.

Comment 12

[Günter]

Current LG supports only one parameter for CATEGORIES whereas RFC [1] allows multiple values separated by COMMA.
As stated with some other posts/bugs this results in data loss when working together with other iCal apps allowing multiple values for CATEGORIES.

Current LG version reads such multi value strings, but only takes the last value. Successive LG syncs with remote calendars write this single value out and finally erases the other ones.

Furthermore current LG supports 'escaped' COMMA for the CATEGORIES string. So any string containing a COMMA would be escaped for output, eg: "CATEGORIES:Boss\,Business"

To solve this bug some decisions are required:
1. should LG support values containing commas, eg: "CATEGORIES:Boss,Business\, Solution" which is two values: 1='Boss'  and 2='Business, Solution'
--> from my point of view this is not usual ... AFAIK also not forbidden by the RFC

2. the 'New Event/Task' UI needs to support the selection of multiple values

3. should LG with multiple CATEGORIES values support an automatic sorting or a user seleted sorting (prio for the cats)

4. how about the cats color? which color should be used? the color for the first one?
--> that's why sorting

Any advice??

Günter

[1] http://tools.ietf.org/html/rfc2445#section-4.8.1.2 Categories

Comment 13

[Günter]

To add to #12 comment:

To work with multiple CATEGORIES items it's required to output the ICS file with an "comma unescaped" string for the multiple category items (also each item may include escaped comma).
Consequently supporting multiple CATEGORIES items like (CATEGORIES:Boss,Business) needs NOT to escape those comma(s).

It's my understanding the  serializer [*] of Lightning escapes EVERY comma in an ICS string.
This requires an change in the back-end??

Günter


[*]  http://mxr.mozilla.org/mozilla1.8/source/calendar/base/src/calICSService.cpp#978

Comment 14

[gekacheka]

cf. libical manual, describes special technique for multi-valued categories: 
"Using Libical", section 4.3 "Multi-Valued Properties".

http://www.softwarestudio.org/libical/UsingLibical/node22.html
or
http://freeassociation.cvs.sourceforge.net/freeassociation/libical/doc/UsingLibical.txt?revision=1.3&view=markup#l_240

Comment 15

[Günter]

(In reply to comment #14)
> cf. libical manual, describes special technique for multi-valued categories: 
> "Using Libical", section 4.3 "Multi-Valued Properties".
Thanks for that recherche. Also Phillip and I said that a multiline CATEGORIES is 'unusual' ...we never saw this in any ICS file.
As with the manual said:
"Oddly, RFC2445 allows some multi-valued properties ( like FREEBUSY ) to exist as both a multi-values property and as multiple single value properties, while others ( like CATEGORIES ) can only exist as single multi-valued properties. This makes the internal representation for CATEGORIES illegal. However when you convert a component to a string, the library will collect all of the CATEGORIES properties into one."
this behavior ends up with a non-conformant ICS file. 
So we need a soultion like that Phillip described at m.d.a.c with thread "CATEGORIES item with multiple values"


 

Comment 16

[gekacheka]

> As with the manual said:
> others ( like CATEGORIES ) can only exist as single multi-valued properties.

I think this is incorrect.  From RFC2445 4.6.1 (VEVENT):

                ; the following are optional,
                ; and MAY occur more than once

                attach / attendee / categories / comment /

so CATEGORIES may appear more than once within VEVENT.
VTODO and VJOURNAL are similar.

> "This makes the internal representation for CATEGORIES
> illegal. However when you convert a component to a string, the
> library will collect all of the CATEGORIES properties into one."
>
> this behavior ends up with a non-conformant ICS file.

Are you basing this conclusion on your reading of the manual, or from testing the code?  My reading of the above text is that the internal (C++) representation doesn't match the ics file, in that internally it keeps multiple CATEGORIES properties instead of one multi-value property, but it still produces conforming ICS files: the multiple CATEGORIES properties of a component are converted to a single CATEGORIES property on output.  (And I hope it is converted from a single CATEGORIES property to multiple CATEGORIES properties when an ics file is parsed.  But I haven't tested the code.)

So this description indicates there can be a round-trip syntactic change, but I don't think it is semantically significant: multiple CATEGORIES properties in an original ics file will become multiple CATEGORIES properties internally, and then be combined into a single CATEGORIES property when output to another ics file.  But CATEGORIES within VEVENT mean the same thing whether they are specified in one property or several.

Comment 17

[Günter]

# 16
>Are you basing this conclusion on your reading of the manual, or from
>testing the code?

We did some tests with the code. Based from an external stored ICS file with an categories item-string like this "CATEGORIES:Boss,Business" with some minor mods to the code we can see LG is working internally with 'one' item-string. It's no question each value can be accessed individually. Such an import would display the whole item-string in the unifinder. Also any modification to that item-string (Boss,Business,Important) would be displayed as such. So internally there is no real problem.

Problems come up with the external world. Exporting (using "Reload all remote calendar" button) LG/libical will do the following:
-- take the item-string and escapes it, so the ext.string will be (fe: Boss\,Business\,Important)
-- the current code treats the item-string as one CATEGORIES output ONLY!, so it 'thinks' it's one category just including a comma ... and escapes it!

My favorite Validator [*] accepts multiline CATEGORIES. I found one ICS calendar in 'mozilla\calendar\libical\test-data\restriction.ics' and with deleting the double SEQUENCE it accepts the multiline CATEGORIES.

Also this is said to be valid, for me this seems to be a VERY unusual notification. Never saw any ICS file with such multiline CATEGORIES.

Because the current code doesn't produce multiline CATEGORIES I would like to promote a solution with singleline CATEGORIES with multiple csv text-values.

[*]  http://severinghaus.org/projects/icv/

Comment 18

[Daniel Boelzle [:dbo]]

(In reply to comment #17)
> -- the current code treats the item-string as one CATEGORIES output ONLY!, so
> it 'thinks' it's one category just including a comma ... and escapes it!

So a proposed fix would export all categories as individual CATEGORIES properties, e.g.

CATEGORIES:a
CATEGORIES:b

Comment 19

[Daniel Boelzle [:dbo]]

Attachment: fixing dataloss

Aside from the UI changes needed, this patch should fix the dataloss when importing/exporting data. I've done some basic tests with storage/ics/wcap, some other code areas haven't been tested.

Comment 20

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

Comment on attachment 321954 [details] [diff] [review]
fixing dataloss

I'm not sure I like the approach to do special casing for categories, because we'll probably run into the same problems for other properties that can have more than one property.

I'd prefer making getProperty() return a comma separated string of all properties with correct escaping, and introducing new methods getPropertyAsArray (or getArrayProperty), and isMultiValueProperty() to allow callers to get any ICS value needed as an array.

r- based on the above, I'll happily reconsider if you have good arguments :-)

In any case, code review:


>+
>+    return aItem.getCategories({}).some(
>+        function someFunc(cat) {
>+            return (cat.toLowerCase().indexOf(searchText) != -1);
>+        });
Similar comments as below. Is there a way we can improve the performance of this block?


>+  /**
>+   * Gets the array of categories this item belongs to.
>+   */
>+  void getCategories(out PRUint32 aCount,
>+                     [array, size_is(aCount), retval] out wstring aCategories);
>+
>+  /**
>+   * Sets the array of categories this item belongs to.
>+   */
>+  void setCategories(in PRUint32 aCount,
>+                     [array, size_is(aCount)] in wstring aCategories);
>+
So what do we do about utf8? I know AUTF8String won't work here, but does wstring cover it correctly?


>-            try {
>-                catColor = pb2.getCharPref("calendar.category.color."+item.getProperty("CATEGORIES").toLowerCase());
>-            } catch(ex) {}
>+            for each (var cat in item.getCategories({})) {
>+                try {
>+                    catColor = pb2.getCharPref("calendar.category.color." + cat.toLowerCase());
>+                    break; // take first matching
>+                } catch(ex) {}
>+            }
Looks a little much for getting the first element?

var cats = item.getCategories({});
if (cats && cats.length) {
   catColor = getPrefSafe("calendar.category.color." + cat.toLowerCase());
}

Same goes for week printer

>+    // categories
>+    var aCat = a.getCategories({});
>+    var bCat = b.getCategories({});
>+    if ((aCat.length != bCat.length) ||
>+        aCat.some(function notIn(cat) { return (bCat.indexOf(cat) == -1); })) {
>+        return false;
>+    }
>+
While its totally correct, its O(n^2) or more. I'd prefer assuming the array is sorted the same as it was before, so only comparing each element to see if the same index on the other event is different. This might cause an extra update, but greatly improve performance in other cases.

Comment 21

[Daniel Boelzle [:dbo]]

(In reply to comment #20)
> (From update of attachment 321954 [details] [diff] [review])
> I'm not sure I like the approach to do special casing for categories, because
> we'll probably run into the same problems for other properties that can have
> more than one property.
> 
> I'd prefer making getProperty() return a comma separated string of all
> properties with correct escaping, and introducing new methods
> getPropertyAsArray (or getArrayProperty), and isMultiValueProperty() to allow
> callers to get any ICS value needed as an array.

Escaping is always a bad choice IMHO, because it leaves the decoding burden on the API user. Ask yourself how often you've used valueAsIcalString and why. The alternative I am OK with is getPropertyAsArray if it really pays off (i.e. if there are enough 2445 props). Else I think there's nothing against property promotion like we do for other properties; after all calIEvent et al are there making developer's life easier instead of using calIIcalComponent et al.

> >+    return aItem.getCategories({}).some(
> >+        function someFunc(cat) {
> >+            return (cat.toLowerCase().indexOf(searchText) != -1);
> >+        });
> Similar comments as below. Is there a way we can improve the performance of
> this block?
Is there any need? AFAIR it's triggered by the event dialog.

> >+  /**
> >+   * Gets the array of categories this item belongs to.
> >+   */
> >+  void getCategories(out PRUint32 aCount,
> >+                     [array, size_is(aCount), retval] out wstring aCategories);
> >+
> >+  /**
> >+   * Sets the array of categories this item belongs to.
> >+   */
> >+  void setCategories(in PRUint32 aCount,
> >+                     [array, size_is(aCount)] in wstring aCategories);
> >+
> So what do we do about utf8? I know AUTF8String won't work here, but does
> wstring cover it correctly?
wstring can cover unicode strings. The UTF-8 conversion is done automagically by spidermonkey when calling UTF-8 methods. So we don't care really about UTF-8, but return/use it where appropriate in IDL, e.g. the ics-service/libical naturally works on UTF-8. That's at least my understanding, maybe I am wrong ;)

> >-            try {
> >-                catColor = pb2.getCharPref("calendar.category.color."+item.getProperty("CATEGORIES").toLowerCase());
> >-            } catch(ex) {}
> >+            for each (var cat in item.getCategories({})) {
> >+                try {
> >+                    catColor = pb2.getCharPref("calendar.category.color." + cat.toLowerCase());
> >+                    break; // take first matching
> >+                } catch(ex) {}
> >+            }
> Looks a little much for getting the first element?
I don't know much about the existing code, but if there's no char pref matching the category, it ought to loop, shouldn't it?

> >+    // categories
> >+    var aCat = a.getCategories({});
> >+    var bCat = b.getCategories({});
> >+    if ((aCat.length != bCat.length) ||
> >+        aCat.some(function notIn(cat) { return (bCat.indexOf(cat) == -1); })) {
> >+        return false;
> >+    }
> >+
> While its totally correct, its O(n^2) or more. I'd prefer assuming the array is
> sorted the same as it was before, so only comparing each element to see if the
> same index on the other event is different. This might cause an extra update,
> but greatly improve performance in other cases.
Yes, it's O(n^2), but think about how many categories are in a common set: I assume 3-5 at most. IMO it won't make any difference from a performance POV, and moreover AFAIR that method is called on item modification, not on reading out items (which is more performance critical). Ok, so far about arguing...
After all, aside from performance considerations, I think you are right,  we should sort and compare, because it's easier to read than the above code.

Comment 22

[Daniel Boelzle [:dbo]]

Doh! I forgot to add some unit tests. Shame on me :/

Comment 23

[Daniel Boelzle [:dbo]]

Thinking further about an API (, and about the dataloss with multiple X-props at the same component), IMO we should refrain from adding further getPropertyAsArray (or getAllProperties) resp. setPropertyAsArray (or setAllProperties) methods, because it confuses callers about orthogonality and when to use what function:
- What does getProperty return if multiple properties have been collected?
- What does setProperty if multiple properties exist? Replacing a random single one or replacing all properties with the single passed one?

Finally, I think it's better to allow getProperty/setProperty to handle with an array of strings. As a consequence client code would need to cope with arrays if the property is allowed to occur multiple times; only client code could know that (e.g. multiple X-props).
To make developer's life a little easier, we could add a second parameter to getProperty, e.g.

  nsIVariant getProperty(in AString name, in boolean enForceReturnAsArray);

or alternatively offer a helper function like e.g.

function ensureArray(val);

While all this solves the multi-value property getter/setter thing, a further problem is just at the horizon: How do we expose property parameters correctly? The existing API is insufficient, too.

PS: Philipp, I still appreciate a comment on my reasonings in comment #21.

Comment 24

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

(In reply to comment #21)
> (In reply to comment #20)
> > I'd prefer making getProperty() return a comma separated string of all
> > properties with correct escaping, and introducing new methods
> > getPropertyAsArray (or getArrayProperty), and isMultiValueProperty() to allow
> > callers to get any ICS value needed as an array.
> 
> Escaping is always a bad choice IMHO, because it leaves the decoding burden on
> the API user. Ask yourself how often you've used valueAsIcalString and why. The
> alternative I am OK with is getPropertyAsArray if it really pays off (i.e. if
> there are enough 2445 props). Else I think there's nothing against property
> promotion like we do for other properties; after all calIEvent et al are there
> making developer's life easier instead of using calIIcalComponent et al.
I think since API users have getPropertyAsArray() available, they don't need to decode anything. The only use case is that getProperty("CATEGORIES") returns something sensible in case it is called for some reason. A non-escaped string would be wrong, and if the API user wants to handle multiple categories he/she can use getPropertyAsArray() and get a properly processed array that doesn't require escaping.

List of properties that accept multiple values separated by comma:

CATEGORIES
RESOURCES
FREEBUSY
EXDATE
RDATE



> > Similar comments as below. Is there a way we can improve the performance of
> > this block?
> Is there any need? AFAIR it's triggered by the event dialog.
The event dialog is quite slow itself, don't you think?

> > So what do we do about utf8? I know AUTF8String won't work here, but does
> > wstring cover it correctly?
I guess we can use wstring for now and if C++ users experience problems then we can hope that the utf8 bug is fixed and change the api :-)


> I don't know much about the existing code, but if there's no char pref matching
> the category, it ought to loop, shouldn't it?
If the pref for the category is boolean or other, then something is definitely wrong and the user played around in his prefs ;-) Feel free to ignore, since its not your code though.

(In reply to comment #23)
> While all this solves the multi-value property getter/setter thing, a further
> problem is just at the horizon: How do we expose property parameters correctly?
> The existing API is insufficient, too.
Interesting thought and totally valid. We are getting closer and closer to directly offering almost all of the calIICalProperty methods in calIItemBase. If we were to do that, the API might get overly complicated for new users though.

If getProperty is nsIVariant anyway, we could make the second parameter be

in PRUint32 type

where type defaults to PROPERTY_TYPE_STRING. If PROPERTY_TYPE_ARRAY is specificed, then an array is returned, if PROPERTY_TYPE_ICALPROP is specified, then a calIICalProperty is returned. In this advanced case, the caller can directly manipulate the ical property and also the parameters.

This in turn gives us the same problems though, what happens if PROPERTY_TYPE_STRING is requested and its a multi-prop?

Also we still have a problem of how to save the properties internally, which makes me think that saving the calIIcalComponent for the event, or at least the calIICalProperty for each property makes sense.

Comment 25

[Daniel Boelzle [:dbo]]

(In reply to comment #24)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > I'd prefer making getProperty() return a comma separated string of all
> > > properties with correct escaping, and introducing new methods
> > > getPropertyAsArray (or getArrayProperty), and isMultiValueProperty() to allow
> > > callers to get any ICS value needed as an array.
> > 
> > Escaping is always a bad choice IMHO, because it leaves the decoding burden on
> > the API user. Ask yourself how often you've used valueAsIcalString and why. The
> > alternative I am OK with is getPropertyAsArray if it really pays off (i.e. if
> > there are enough 2445 props). Else I think there's nothing against property
> > promotion like we do for other properties; after all calIEvent et al are there
> > making developer's life easier instead of using calIIcalComponent et al.
> I think since API users have getPropertyAsArray() available, they don't need to
> decode anything. The only use case is that getProperty("CATEGORIES") returns
> something sensible in case it is called for some reason. A non-escaped string
> would be wrong, and if the API user wants to handle multiple categories he/she
> can use getPropertyAsArray() and get a properly processed array that doesn't
> require escaping.
> 
> List of properties that accept multiple values separated by comma:
> 
> CATEGORIES
> RESOURCES
> FREEBUSY
> EXDATE
> RDATE

What do we do in case of multiple X-props? Returning those as multiple (escaped) comma-separated values sounds bad to me:
1) We are enforcing the separator, even though it might not be valid to have one.
2) It still leaves decoding burden on the caller. Handling those per array is mucher easier.

> > > Similar comments as below. Is there a way we can improve the performance of
> > > this block?
> > Is there any need? AFAIR it's triggered by the event dialog.
> The event dialog is quite slow itself, don't you think?
Yes, but I really doubt that this pays off. you usually don't have many categories.

> > > So what do we do about utf8? I know AUTF8String won't work here, but does
> > > wstring cover it correctly?
> I guess we can use wstring for now and if C++ users experience problems then we
> can hope that the utf8 bug is fixed and change the api :-)
We vanish anyway, we are heading for a different API I guess ;)

> (In reply to comment #23)
> > While all this solves the multi-value property getter/setter thing, a further
> > problem is just at the horizon: How do we expose property parameters correctly?
> > The existing API is insufficient, too.
> Interesting thought and totally valid. We are getting closer and closer to
> directly offering almost all of the calIICalProperty methods in calIItemBase.
> If we were to do that, the API might get overly complicated for new users
> though.
> 
> If getProperty is nsIVariant anyway, we could make the second parameter be
> 
> in PRUint32 type
> 
> where type defaults to PROPERTY_TYPE_STRING. If PROPERTY_TYPE_ARRAY is
> specificed, then an array is returned, if PROPERTY_TYPE_ICALPROP is specified,
> then a calIICalProperty is returned. In this advanced case, the caller can
> directly manipulate the ical property and also the parameters.
> 
> This in turn gives us the same problems though, what happens if
> PROPERTY_TYPE_STRING is requested and its a multi-prop?
Yes, thus it could only be a hint (resp. convenience conversion); client code *always* needs to cope with arrays if the property appears multiple times.
I like the idea of adding a hint to return more complex property objects (e.g. calIIcalProperty objects). This would make getPropertyParameter etc obsolete.

> Also we still have a problem of how to save the properties internally, which
> makes me think that saving the calIIcalComponent for the event, or at least the
> calIICalProperty for each property makes sense.

I am not sure whether we want to use it for storing, but if calEvent et al is fed by icalComponent (like ics/caldav/wcap provider do), then it makes sense to hold the icalComponent, to prevent costly property cloning.

Comment 26

[Simon Paquet [:sipaq]]

(In reply to comment #25)
>>>> Similar comments as below. Is there a way we can improve the performance 
>>>> of this block?
>>>
>>> Is there any need? AFAIR it's triggered by the event dialog.
>>
>> The event dialog is quite slow itself, don't you think?
>
> Yes, but I really doubt that this pays off. you usually don't have many
> categories.

1. I would prefer it if we fix potential performance issues before they 
   become a huge burden (e.g. the views) not afterwards.
2. We ship with quite a few default categories and users can (and will) 
   probably define a few of their own. So I don't necessarily follow your 
   assumption here. 

Comment 27

[Daniel Boelzle [:dbo]]

(In reply to comment #26)
Simon, you are getting off-topic. The mentioned patch does not do it worse than it currently is, but in some cases better.

Further improvements to that are not topic of this bug; Philipp just asked if we could easily do that, but IMHO those are no hot spots.

However, if we want to improve that: I doubt that
1) cloning and sorting the array, then comparing O(n)
is better than
2) comparing O(n^2)
for a small number of elements.

But again, this should be done in a separate bug. Premature optimization is no good, and has often drawbacks w.r.t. code readability.

Comment 28

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 321954 [details] [diff] [review]
fixing dataloss

>diff -u -8 -p -r1.1.2.6 calendar-summary-dialog.js
>+        document.getElementById("item-category").value = categories.join(", ");

I don't think that's l10n-friendly. I don't think that all locales use "comma-space" to join items. But I don't know the correct way to do it either.  Can you file a followup-bug to figure out if it really wrong, and how to fix it?

Comment 29

[Günter]

(In reply to comment #28)
> (From update of attachment 321954 [details] [diff] [review])
> >diff -u -8 -p -r1.1.2.6 calendar-summary-dialog.js
> >+     document.getElementById("item-category").value = categories.join(", ");
> 
> I don't think that's l10n-friendly. I don't think that all locales use
> "comma-space" to join items. But I don't know the correct way to do it either. 

The RFC defines a 'pure' comma as the delimiter between items on a line. Also there is no alternative for locales, they have to use a comma!

From my POV an space after the comma belongs to the following item, so having 
CATEGORY:a, a
will endup with two different item: 'a' and ' a'.


Rechecking the '0.9pre 20080731119':
Still has data loss!

Importing following ICS data

CATEGORY:a,b,c     will rewrite CATEGORY:c

a sequence of 
CATEGORY:x
CATEGORY:y
CATEGORY:z         will rewrite CATEGORY:z

a sequence of 
CATEGORY:a,b,c
CATEGORY:y
CATEGORY:z,w       will rewrite CATEGORY:w

to the ICS file. So the export will produce DATA LOSS!

Should add following 
flags: tb-integration, blocking‑calendar0.9
keywords: uiwanted  (a MUST to support multiple items for the user)

Comment 30

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #29)
> > >+     document.getElementById("item-category").value = categories.join(", > The RFC defines a 'pure' comma as the delimiter between items on a line. Also
> there is no alternative for locales, they have to use a comma!

In what format the categories are shown to the user is totally unrelated to how the data is stored internally. The rfc has nothing to say about our UI.

Comment 31

[Simon Paquet [:sipaq]]

I don't think that this constitutes a blocker.
Recommend blocking0.9-

Comment 32

[Daniel Boelzle [:dbo]]

Attachment: [checked in] fixing dataloss, debitrotted, adopted - v2

Since we will go with adding API where needed (RELATED-TO discussion) and resolve it when we've figured out how to generically handle multi-properties, this is the debitrotted patch to get rid of the dataloss part. As long as the user doesn't modify an item via dialog (which still can only cope with a single category), this should preserve multiple CATEGORIES, especially in ics files. It's been adopted to Berend's recent works on task categories, too.

Comment 33

[Daniel Boelzle [:dbo]]

The feature-part (UI support for multiple categories) does not block 0.9 (we're in UI/string freeze anyway), but fixing the dataloss part is strongly wanted.

Comment 34

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

Comment on attachment 334872 [details] [diff] [review]
[checked in] fixing dataloss, debitrotted, adopted - v2

>+    var categories = item.getCategories({});
>+    if (categories.length > 0) {
>         document.getElementById("category-row").removeAttribute("hidden");
>+        document.getElementById("item-category").value = categories.join(", "); // admittedly l10n-unfriendly
>     }
Something like TODO L10N would be good for the comment maybe?


>+                  case "calendar-task-tree-col-categories": {
>+                      var categories = task.getCategories({});
>+                      return (categories.length > 0 ? categories[0] : null);
>+                  }
Do we really only want to show one category? Maybe use categoriesArrayToString() ?


>+        for each (var itemCategory in aItem.getCategories({})) {
>+            if (!categoriesList.some(function(cat){ return cat == itemCategory; })){
>+                categoriesList.push(itemCategory);
>             }
>         }
To avoid potentially O(n^2), I'd suggest looping through one array and creating a hashmap, then just using Array.filter() in the second run.


>+    // xxx todo: what about category "NONE"?
>+    if (category == "NONE") {
>+        aItem.setCategories(0, []);
>     } else {
>+        aItem.setCategories(1, [category]);
>     }
> }
What *is* category "NONE" ? Sounds like something we don't need to me...


>+        line.push(txtString(categoriesArrayToString(item.getCategories({})))); // xxx todo: what's the correct way to encode ',' in csv?, how are multi-values expressed?
I've heard you need to enclose the whole value in quotes, i.e ...,foo,"bar,baz",fubar,...


>+    var categories = aItem.getCategories({});
>+    if (categories.length > 0) {
>         var gdCategories = <gd:extendedProperty xmlns:gd={gd}/>;
>         gdCategories.@name = "X-MOZ-CATEGORIES";
>-        gdCategories.@value = categories;
>+        gdCategories.@value = categoriesArrayToString(categories);
>         entry.gd::extendedProperty += gdCategories;
>     }
While you are here, please *always* set X-MOZ-CATEGORIES. This might be a different bug but is a trivial fix.


r=philipp

Comment 35

[Daniel Boelzle [:dbo]]

Comment on attachment 334872 [details] [diff] [review]
[checked in] fixing dataloss, debitrotted, adopted - v2

Adopted and checked in on HEAD and MOZILLA_1_8_BRANCH.

The only dataloss that's left is if the user edits an existing event that comes with multiple categories.

Comment 36

[Dan Mosedale (:dmosedale, :dmose)]

> // xxx todo: what's the correct way to encode ',' in csv?, how are multi-values expressed?
> I've heard you need to enclose the whole value in quotes, i.e
> ...,foo,"bar,baz",fubar,...

For reference, CSV is semi-specified in RFC 4180, and section 2.6 agrees with the above.

Comment 37

[David Ascher (:davida)]

It sounds to me like the most likely dataloss issues have been addressed, so I don't think it would block tb-integration.  It still sounds like a good thing to address, though.

Comment 39

[klonos]

emails support tags. I think there should be tag support for events and tasks.

(contacts as well -now that I think of it- could have the same mechanism, except instead of 'tags' they could be called 'categories')

Comment 42

[Dimas]

I am very interested in resolving this bug, but do not really know the state in which it currently is.

 - What's left to do? What are the remaining tasks?
 - Have you any idea how to handle multi-properties?

Comment 43

[Günter]

(In reply to comment #42)
> 
>  - What's left to do? What are the remaining tasks?

As you can see with up to #35 the main issue for "CATEGORIES" has been solved by Daniel. That is, internally LG works with it, but the current UI doesn't support multiple, comma separated values. AFAIR only the last CATEGORIES value of an imported event will be stored to LG after working on the UI. 
So "all" is about working on the UI.

>  - Have you any idea how to handle multi-properties?
"multi-properties" (see Philipps comment # 24) should be handled in a similar manner as the CATEGORIES.

Comment 44

[Dimas]

(In reply to comment #43)
Ok, I will try to resolve this bug. Thank you.

Comment 45

[Dimas]

Attachment: UI for multiple categories support

Comment 46

[Dimas]

Attachment: Screenshot for one category

Comment 47

[Dimas]

Attachment: Screenshot for multiple categories

Comment 48

[Dimas]

Attachment: Screenshot for custom menuItem

Comment 49

[Dimas]

Attachment: Screenshot for multiple categories dialog

Comment 50

[Nis Donatzsky]

Attachment: UI proposal for multiple category assignment

While functional, Dimas' UI will quickly become annoying if more than a few categories has to be assigned; essentially requiring too many mouse clicks. A listbox also has some desirable properties - compared to a dropdown - such as not "resetting" when losing focus.
It also has no provision for creating new categories.

As to my proposal:
The "Create new" could also be called "Manage ..." and simply open the current category management dialog, but that should probably be avoided in favor of showing the (also current) "Add category" dialog.

Comment 51

[Günter]

When it comes to the UI discussion I would like to point to another solution, using the new feature "panel" in the hope the current Lightning version does support it (at least TB3.1 does!).

A good example can be found with the Postbox mailing app (a Mozilla/TB variant, see http://postbox-inc.com/)
They use 'panel' for "Topic", the Thunderbird equivalent it's "Tag".

The beauty is: one dialog with all necessary elements: by tic box [ ] select multiple items already defined, enter a new item, let the new be a single one _or_ add it to the "Topic" list (named favorite). 
See attachment "topic_panel_example.png"

Such a concept would be great for the "Multiple Categories"

Comment 52

[Günter]

Attachment: Postbox example for "Panel" .. to be used for Multiple Categories in LG ??

see previous comment

Comment 53

[Dimas]

I've implemented this way so that the categories operate similarly to
the reminders. I think that is better for the user to work in the same
way with the categories and reminders.

Comment 55

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 447059 [details] [diff] [review]
UI for multiple categories support

I like the system from attachment 449880 [details] most however I'll just review what we have for now as it's a good step forward.

Everything looks ok, my only question is with the "Custom..." text used in several places.  

* At the bottom of the menu list should "Custom..." instead be "Multiple Categories..."?

* When the list is collapsed and custom/multiple categories is selected it would be nice if the categories were shown instead of the text "Custom..." or "Multiple Categories...".  i.e. "Anniversary, Birthday, Event..." instead of "Custom..." just so people could know the categories applied without opening the dialog.

Otherwise it looks good.  I'll plus for now and look for followup comments.

Comment 56

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

Comment on attachment 447059 [details] [diff] [review]
UI for multiple categories support

I'd also prefer a dropdown dialog like Bryan suggested in the previous comment. I think this is much more clear for the user and easier to manage. Yet another new dialog takes the user a bit out of context. Dimas, would you be interested in implementing a dropdown like this? I believe its just a matter of using a <panel> as the dropdown element, and there is an attribute that  makes sure the dropdown is not auto-hidden when its clicked on. This would allow the user to select multiple categories easily directly from the dropdown.

I'll give you a review for the current patch though:

> /**
>+* Fills up a menu - either a menupopup or a menulist - with menuitems that refer
>+* to a multiple categories.
Good work on writing documentation! Thanks for keeping that in mind.


>+function appendMultipleCategoryItems(args, aCategoryMenuList, aCategoryListBox, aCommand) {
>+    var categoriesList = getPrefCategoriesArray();
Please use let instead of var in new functions and those you change.


>+    while (aCategoryMenuList.hasChildNodes()) {
>+        aCategoryMenuList.removeChild(aCategoryMenuList.lastChild);
>+     }
>+
>+     while (aCategoryListBox.hasChildNodes()) {
>+         aCategoryListBox.removeChild(aCategoryListBox.lastChild);
>+     }
We have a helper for this in calendar-ui-utils.js, I believe its called removeChildren()


>+        for each (var itemCategory in itemCategories) {
>+            if (!categoriesList.some(function(cat){ return cat == itemCategory; })){
The new JS versions allow you to write this as:

if (!categoriesList.some(function(cat) cat == itemCategory)) {


>+function matchCustomCategoryToMenuitem(category) {
>+    let categoryList = document.getElementById("item-category");
>+    let categoryPopup = categoryList.firstChild;
>+
>+    if(category) {
>+        for each (let menuitem in Array.slice(categoryPopup.childNodes)) {
>+            if (menuitem.label == category) {
>+                categoryList.selectedItem = menuitem;
>+                return true;
>+            }
>+        }
>+    }
Can't you use categoryPopup.getElementsByAttribute("label", category) here? Not sure this works, please test :)

>diff -r 8bf70dc833c5 calendar/base/content/dialogs/calendar-event-dialog-category.js
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/calendar/base/content/dialogs/calendar-event-dialog-category.js	Mon May 24 11:34:58 2010 +0200
>+ *
>+ * Contributor(s):
>+ *   Michael Buettner <[e-mail]>

Go ahead and add yourself as a contributor for files you've changed.

>+            <!-- Calendar -->
>+            <row id="event-grid-calendar-color-row"
>+                 align="center">
>+                <label value="&event.calendar.label;"
>+                       accesskey="&event.calendar.accesskey;"
>+                       control="item-calendar"
>+                       disable-on-readonly="true"/>
>+                <menulist id="item-calendar"
>+                       disable-on-readonly="true"
>+                       oncommand="updateCalendar();"/>
>+            </row>
Bryan has ui+'d this, so I guess I'm ok with it. But personally I think that moving the calendar selector to a new row leaves us with two very long dropdowns, and especially on small screens it makes the event dialog even less readable. A solution with a panel dropdown would be nicer.


>+
> None=None
>+Custom=Custom...
I'd suggest changing the label here to "Multiple Categories..." or "Multiple...". Also please use the utf8 ... character instead of three dots.



>diff -r 8bf70dc833c5 calendar/locales/en-US/chrome/calendar/dialogs/calendar-event-dialog-category.dtd
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/calendar/locales/en-US/chrome/calendar/dialogs/calendar-event-dialog-category.dtd	Mon May 24 11:34:58 2010 +0200
>@@ -0,0 +1,41 @@
>+<!-- ***** BEGIN LICENSE BLOCK *****
>+   - Version: MPL 1.1/GPL 2.0/LGPL 2.1
Go ahead and use a new license header here (i.e you as the initial developer, The Original Code is Mozilla Calendar Code). There is not much copied from the other dialog. The same goes for the other new files. If you feel you've copied a lot from the other files, then find out who wrote those lines (using hg annotate / cvs blame)  and add them as a contributor.

>+<!ENTITY categorydialog.title                              "Set up Categories">
Maybe a bit more specific, i.e:  "Set up Categories: New Event", or "Edit Categories".



>diff -r 8bf70dc833c5 calendar/providers/gdata/jar.mn
>--- a/calendar/providers/gdata/jar.mn	Sun May 16 14:52:44 2010 +0200
>+++ b/calendar/providers/gdata/jar.mn	Mon May 24 11:34:58 2010 +0200
>@@ -7,6 +7,7 @@
> % overlay chrome://calendar/content/calendar-event-dialog-reminder.xul chrome://gdata-provider/content/gdata-event-dialog-reminder.xul
>+% overlay chrome://calendar/content/calendar-event-dialog-category.xul chrome://gdata-provider/content/gdata-event-dialog-category.xul
> % style chrome://calendar/content/calendar-event-dialog.xul chrome://gdata-provider/skin/gdata-event-dialog-reminder.css

I may have missed it, but is there a gdata-event-dialog-category.xul? If so, why is it needed? For the alarms I only needed this file to add the SMS action to the dialogs.


The patch looks good, but I'd appreciate a new patch for checkin, therefore setting r-. Before checking in I'll give it a test, and I'd like to get some input from you (Dimas) on how you feel about the panel dropdown.

Comment 57

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

Oh, regarding the panel again. The textbox to add a new category on the fly would be nice, but not required for now. Just a panel with a way to select one or more categories via checkbox would suffice.

Comment 58

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

Attachment: Panel Solution - v1

This is the panel solution I was talking about. Dimas, do you care to take a look at this patch to see if the code makes sense and give it a test? (preferably on something other than mac)

Comment 59

[Decathlon]

Comment on attachment 517295 [details] [diff] [review]
Panel Solution - v1

The patch basically works, but it has a problem in the menulist in the New Event dialog. As you can see in the following screenshot (first image), the label and the dropdown marker are missing in the menulist. The image also shows the structure as reported by DOM inspector where it is different from a normal menulist.

It seems a problem with the bindings. I tried to change the patch by adding an element "panel-menulist" instead of a "menulist" in the file calendar-event-dialog.xul along with a "panelmenulist" instead of "menulist.panelmenulist"
in the file calendar-widget-binding.css and this changes the structure as showed in the second image of the screenshot, but something is still missing, and I'm not able to do something better.



Maybe a pair of things might be changed (maybe in another bug):
- when there are multiple categories, the label could show the categories list separated by commas and an ellipsis at the end instead of the fixed text "Multiple Categories";
- in the ics file the categories are added one by one each of them with the "CATEGORIES" property (one property and one category) that seems correct according to rfc5545 (the categories property is optional and may occur more than once), though, in the property's description is also written:
 "more than one category can be specified as a COMMA-separated list of categories"



Due to recent changes in the file calendar-task-view.xul, the patch doesn't apply this code and I had to add it manually:

>@@ -178,8 +179,10 @@
>                                  command="calendar_task_category_command"
>                                  observes="calendar_task_category_command"
>                                  class="toolbarbutton-1 msgHeaderView-button">
>-                    <menupopup id="task-actions-category-menupopup"
>-                               onpopupshowing="addCategoryNames(event)"/>
>+                    <panel id="task-actions-category-panel"
>+                           type="categories-panel"
>+                           onpopupshowing="taskDetailsView.loadCategories(event)"
>+                           onpopuphiding="taskDetailsView.saveCategories(event)"/>
>                   </toolbarbutton>


>                   <toolbarbutton id="task-actions-markcompleted"
>                                  type="menu-button"


r-  caused by the problem in the menulist.

Comment 60

[Decathlon]

Attachment: Screenshot: problem with Panel Solution - v1

Comment 62

[Markus Adrario [:Taraman]]

OK, I'm having a go at this one

Comment 63

[Markus Adrario [:Taraman]]

I debitrotted the patch and I solved the problem with the menulist.

After having tried the same things as decathlon again, I discovered, that the binding was:
    menulist.panel-menulist {...}
so there was a class in the selector - and missing in the .xul. Adding it did the trick.
Meanwhile I changed that to
    menulist[type="panel-menulist"] {...}
because that's the way the panel is coded and I think "type" sounds better here.

I'll do some more testing before I upload the patch.

Comment 64

[Markus Adrario [:Taraman]]

Attachment: Panel Solution V2

In addition to comment #63 I changed the following:
>    if (categoryList.length > 1) {
>        label = cal.calGetString("calendar", "multipleCategories");
>    } else if (categoryList.length > 0) {
>        label = categoryList[0];
>    } else {
>        label = cal.calGetString("calendar", "None");
>    }
in the "else if" I set "length == 1". Although "> 0" is logically correct, I think "== 1" is more intuitive to understand. And there should not be items with "> 0 && < 1" categories.

I changed the type-name of the panel to "category-panel" ISO "categories-panel". Just sounds better and the widget is also named this way.

Comment 65

[Decathlon]

Comment on attachment 594580 [details] [diff] [review]
Panel Solution V2

The patch works fine.
Only the calendar-widgets.css part of the code for the gnomestripe theme is missing. I can't test on Linux, but if the effect is the same on Windows, I think that the css rule is necessary on Linux too.

r+ with the above consideration.

The patch doesn't apply perfectly on comm-central and comm-aurora due to the different calendar-task-view.js file.

Maybe some details might be take in count:

- the panel should show more categories when opened in order to allow to see the selected categories with a minimum, or without scrolling the list when, e.g., the selected categories are the first and the last (this obviously depends on the numbers of the categories, but I think that at least the panel might be as long as the menulist without the patch);

- the category color showed in the event in the views is one of the color of the selected categories without a precise rule (maybe the last defined color in the categories options dialog?). Would be useful a way to decide what color has to be showed in the event, but I wouldn't know a good way to do it.

A very trivial issue is that on Win7 the selected categories on the task view and on the New Event dialog have a slightly different look. The space between the items is also slightly higher in the task view (the panel shows 11 categories in the dialog and 10 in the task view with the same height).

Comment 66

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

Attachment: Panel Solution V3

As much as I wanted to "just quickly fix the remaining issues", the most I can offer is a debitrotted patch :-(

It would be sweet if we could land this by Tuesday (Merge Date), or at least land the strings by Tuesday and then fix on aurora afterwards. Markus, Decathlon, do you think one of you could try to find out whats going wrong? I guess we could land it as is too, but then we need to file (and fix) the followup issues before this makes the final release.


(In reply to Decathlon from comment #65)
> Only the calendar-widgets.css part of the code for the gnomestripe theme is
> missing. I can't test on Linux, but if the effect is the same on Windows, I
> think that the css rule is necessary on Linux too.
Fixed by debitrot, we have common CSS now.


> - the panel should show more categories when opened in order to allow to see
> the selected categories with a minimum, or without scrolling the list when,
> e.g., the selected categories are the first and the last (this obviously
> depends on the numbers of the categories, but I think that at least the
> panel might be as long as the menulist without the patch);
What would be even cooler is if the listbox scrolled using one of those arrowscrollbox'es. I tried this by injecting a modified binding, but I never got it to scroll. Then I tried to give the listbox a new maximum height, but this didn't work out. The only way I could modify the hsize was to change the "rows" attribute, but when there were too many rows (in the task view, where the button is in the middle of the screen), the listbox just overflowed and didn't turn smaller.


> - the category color showed in the event in the views is one of the color of
> the selected categories without a precise rule (maybe the last defined color
> in the categories options dialog?). Would be useful a way to decide what
> color has to be showed in the event, but I wouldn't know a good way to do it.
I don't quite understand, could you elaborate?

> A very trivial issue is that on Win7 the selected categories on the task
> view and on the New Event dialog have a slightly different look. The space
> between the items is also slightly higher in the task view (the panel shows
> 11 categories in the dialog and 10 in the task view with the same height).
On mac the checkboxes are also different sizes. I couldn't find anything obvious in the CSS rules.

Comment 67

[Markus Adrario [:Taraman]]

I can't work on it this week, probably not until end of march. :-(

Comment 68

[Fabián]

Hi all. First of all, I am a co-worker of Dimas. During some tome we have been using our own distribution of Lightning, in which we have implemented the solution fore multicategories proposed by Dimas. When version 8 of thunderbird was released, we faced the problem that we had to made a complete review of the plugin because of the mayor changes of the version. Trying to avoid these facts, we are very interested in work together to achieve a good solution for all, so we can develop changes needed, but before, is there any agreement about the requirements to be implemented?

Comment 69

[Markus Adrario [:Taraman]]

Hi Fabián,

I think the last patch is a good place to start.
As I was the last to work on this, I believe that it was agreed as good solution.
I think comments 65 & 66 cover this quite good.

I would be happy to see you work on this.

Comment 70

[Fabián]

Hi again, starting to work with this, but first of all a question. What Lightning version is done for? Comm-central version is now inversion 2.6, for Thunderbird 24.

Comment 71

[Fabián]

Aplying this patch to Lightning v1.9b1 over Thunderbird 17.0.6 the categories panel does not drop down and when you type with it selected this error is shown:

Date and time: 21/05/13 15:47:57
Error: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMenuBoxObject.activeChild]
Archivo de origen: chrome://global/content/bindings/menulist.xml
Line: 59

Working on this.

Comment 72

[Daniel]

Attachment: Proposal for adding a new category

Comment 73

[Daniel]

Hello,

I've made some minor modifications to the patch in order to add support for assigning a new category straight from the dropdown. I'd like to get some feedback before proceeding, so please check the attached picture to see the work in progress.

Comment 74

[Daniel]

Attachment: Modified dropdown

Added a button and modified the look.

Comment 75

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

Generally I think this is the right direction. The dropdown could use a bit more styling though, it looks like some borders are missing and it doesn't look like a normal dropdown. I think it should look a bit like this:

https://addons.cdn.mozilla.net/img/uploads/previews/full/32/32473.png?modified=1331247702

Of course with a different purpose and not a search box but one with the emptytext to add a new category.

Daniel, glad to hear you are picking this up!

Comment 76

[Daniel]

Attachment: Panel solution V4

I've tried to avoid the scrollbar but it seems you can not change the number of visible rows of the listbox after its creation.

Comment 77

[Daniel]

Attachment: New dropdown

Comment 78

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

The new style looks great! I'm going to push at least the strings for this bug tomorrow evening before the trees merge. I'll try to review and test the rest of the patch by then too.

Comment 79

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

A few minor usability issues:

* When a category is added, the textbox should be emptied.
* The textbox still has the "search" style, with the magnifier and everything. It would be nice if we could show a normal textbox instead, this can be done using the css rule -moz-appearance: textfield.
* For some reason the bottom corners are squared while they are usually rounded on mac. This can be solved by a 4px bottom margin on the listbox.

I will continue later on and complete code review this evening.

Comment 80

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

Oh and we should have a provider property so the provider can specify the maximum number of categories. Default is infinite, some providers might want to specify 1. If the value is 1, then the UI should look like it did before. Either do this by hiding the textbox and the checkboxes, or show a normal menupopup.

Comment 81

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

Pushed to comm-central changeset 72a3ee809ec7

Comment 82

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

I've fixed the issues from comment 79 before checkin. But will file a followup bug for the provider. Daniel, I'd appreciate if you could take care of this within the next 4 weeks. See bug 886211.

Comment 83

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

Comment on attachment 764035 [details] [diff] [review]
Panel solution V4

This is r+ with the changes I have made before checkin. Please see hg for the full patch.