299846 - Restore calendar and category colors

Status: RESOLVED FIXED

(Calendar :: Sunbird Only, defect)

Description

[Joey Minta]

Colors make things look nice.  I'd like them back.  Not an 0.3a1 blocker though.

Comment 1

[Joey Minta]

Attachment: colors

It's probably better to check in the alarms patch first (if both r+), since
this will mess up line numbers in calendar.js

Comment 2

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

Comment on attachment 188434 [details] [diff] [review]
colors

>diff -ur mozilla/calendar/resources/content/calendar.js mozpatch/calendar/resources/content/calendar.js

can you use -u8p for new diffs? That will add a bit more context and function
names.

>+/*  XXX DO we want to keep any of this?  None of it belongs here.

Yes, we want to keep this. Please don't remove things until you are sure they
shouldn't be there, and do so in seperate bugs (be nice to cvs blame)

Comment 3

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

Comment on attachment 188434 [details] [diff] [review]
colors

I'm not sure yet what is wrong, but for me this patch shows all events from all
calendars in the color of the first calendar.

Comment 4

[Joey Minta]

(In reply to comment #3)
> (From update of attachment 188434 [details] [diff] [review] [edit])
> I'm not sure yet what is wrong, but for me this patch shows all events from 
all
> calendars in the color of the first calendar.

If the calendars existed previously in your profile to applying this, you need 
to rename them to get this to work.  I should have said that the patch requires 
that to work.

>>+/*  XXX DO we want to keep any of this?  None of it belongs here.
>
>Yes, we want to keep this. Please don't remove things until you are sure they
>shouldn't be there, and do so in seperate bugs (be nice to cvs blame)

Are you saying I need to do something different here?  updateColors() was never 
called before, so there parts were never executed previously on trunk.  That's 
why I just commented them out.

Comment 5

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

> Are you saying I need to do something different here?  updateColors() was never 
> called before, so there parts were never executed previously on trunk.  That's 
> why I just commented them out.

-p woul dhave helped here :) These lines were once part of calendarInit(). They
should be moved back to there. They seem to have a purpose.

Is there a way you can remove the need for renaming the calendars?

Comment 6

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

Can you try to port the trick used for lighting,
http://lxr.mozilla.org/seamonkey/source/calendar/lightning/content/calendar-management.js#5

(empty stylesheet, a selector based on the uri instead of a class)

Comment 7

[Joey Minta]

Attachment: colors v2

Better version.  Uses shaver's cool colors code.  No re-naming necessary.  I'll
file another bug for getting the stuff stuck in updateColors() back in to
calendarInit().

Comment 8

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

Comment on attachment 189524 [details] [diff] [review]
colors v2

This patch looks mostly ok.
Wouldn't it amke more sense to move the calendar-coloring into
calCalendarManager.js ?
Can we make the categories coloring work without classes, but with a selector
similar to the calendar coloring?

Comment 9

[Joey Minta]

(In reply to comment #8)
> (From update of attachment 189524 [details] [diff] [review] [edit])
> This patch looks mostly ok.
> Wouldn't it amke more sense to move the calendar-coloring into
> calCalendarManager.js ?

Sure, I'll do that.

> Can we make the categories coloring work without classes, but with a selector
> similar to the calendar coloring?
We can, the only thing is that I think you can only have one category per
selector. Since we're only coloring the border it shouldn't be a big problem,
but it's a small issue.

Comment 10

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

You can have multiple categories if you use the ~= selector. You do need to
fixup the names then though, in order to make it a space-seperated list. But
that's all you have to do: changes spaces into underscores, or just remove them.

Comment 11

[Joey Minta]

Attachment: colors v3

Moves everything to calendarManagement and ports the new mechanism for
categories as well.

Comment 12

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

Comment on attachment 190050 [details] [diff] [review]
colors v3

>-var categoryPrefObserver =

You remove the observer here, but the removeObserver call is still in
calendar.js. I think it will work, but it isn't really clear. Maybe move the
removeObserver to calendarManager.js, and call that in the unload handlere in
calendar.js?

>+function getCalendarStyleSheet() {
>+    var calStyleSheet = null;
>+    for (var i = 0; i < document.styleSheets.length; i++) {
>+    if (document.styleSheets[i].href.match(

nit: indenting.
>+    for(i in calendars) {

other nit: space after for (same in a few other places), use var i.

>+    var prefCount = { value: 0 };
>+    var prefArray = categoryPrefBranch.getChildList("", prefCount);

If you won't use the prefCount value, just use getChildList("", {}); (some a
few lines higher)

>+    function selectorForCategory(name)
>+    {
>+        if(object.uri)
>+            return '*[item-calendar="' + name + '"]';
>+        return '*[item-category~="' + name + '"]';
>+    }

Huh? Aren't both doing the same?

>+var categoryPrefObserver =
>+{
>+   mCalendarStyleSheet: null,
>+   observe: function(aSubject, aTopic, aPrefName)
>+   {
>+       var name = aPrefName
>+       name = name.substr(24, aPrefName.length - 24);

Where does the 24 come from? Please add a comment.


Rest of the patch looks good! r=mvl if you fix above comments. (but r= for now,
mainly the 24 and the useless if)

Comment 13

[Joey Minta]

Attachment: colors v4

This should have the nits fixed, plus a comment on the 24.  Also, the 'if'
wasn't useless, but the function was misnamed.	Now it's "function
selectorForObject(name)" which returns a selector for either a calendar or
category (their selectors are slightly different), depending on what the object
is.

Comment 14

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

Comment on attachment 190736 [details] [diff] [review]
colors v4

>+    function selectorForObject(name)
>+    {
>+        if (object.uri)
>+            return '*[item-calendar="' + name + '"]';
>+        return '*[item-category~="' + name + '"]';
>+    }

Can you add some comments describing what this function does? Selecting if the
object is a calendar (you might want instanceof for better readability) and
doing some css magic. Why is it different? (in 2 weeks, i need to read the css
spec to find out what the rule does. i don't want to do that :) )

Patch looks good. I still need to test it.

Comment 15

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

Comment on attachment 190736 [details] [diff] [review]
colors v4

r=mvl when the comments mentioned in the above bugcomment are added.

Comment 16

[Joey Minta]

Attachment: colors v4.1

added an explanatory comment

Comment 17

[Joey Minta]

Comment on attachment 191331 [details] [diff] [review]
colors v4.1

carrying over r+ from mvl

Comment 18

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

patch checked in