411254 - categories have no default colors

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Andreas Treumann]

STEPS TO REPRODUCE:
===================

- create a new event and add a default category

RESULT:
=======

- the category has no color

EXPECTED RESULT:
================

- the category should have a default color 

REPRODUCIBLE:
=============

- always

Comment 1

[Simon Paquet [:sipaq]]

Not going to happen for 0.8.

Comment 2

[Przemyslaw Bialik]

Attachment: Proposed fix v1

This is proposed set of colors for categories to get this bug going; I've used names for colors so it would be easier to discuss / review.

Comment 3

[Stefan Sitter [:ssitter]]

I doubt that this will work in non English localizations of Lightning. All localizations define their own category names that won't match the rules added in the patch.

Comment 4

[Martin Schröder [:mschroeder]]

Comment on attachment 342509 [details] [diff] [review]
Proposed fix v1

This needs ui-review before review, and I second Stefan's concerns.

Comment 5

[Przemyslaw Bialik]

(In reply to comment #3)
> I doubt that this will work in non English localizations of Lightning. All
> localizations define their own category names that won't match the rules added
> in the patch.
doh! I've missed this...

[quote=lightning.js]
> // categories settings
> // XXX One day we might want to move this to a locale specific file
> //     and include a list of locale specific default categories
> pref("calendar.categories.names", "");
then this comment can be removed
(\calendar\locales\en-US\chrome\calendar\categories.properties lists default localizable categories)

so there are 2 options:
1. based on localizations add locale specific categories to lightning.js - rather not a good idea...
2. make every category a pref with localizeable name - name of the pref would be constant but display differently depending on the locale - my patch would fit this case
3. other ?

Comment 6

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

Personally I'd find it quite spiffy if the default color for a category would be set dynamically based on a hash of the characters used. This way if you have two installations of lightning/sunbird, you don't need to set the same color again, you just need to have the same category name.

Not sure how that fits in with default pref values and such.

Comment 7

[Przemyslaw Bialik]

I like your idea but I also second your concerns, so maybe it should only apply to the new (added by the user) categories ?
Until there are test results it's hard to say if we would be happy from the outcome (auto-colored default categories).
I think we must answer the question - do we (and how much) care for the color every category has by default (there might be some "standards" in this matter, I'm not sure) or not.

Comment 8

[Christian Jansen]

Comment on attachment 342509 [details] [diff] [review]
Proposed fix v1

Thanks for the patch. I personally do not like the used colors. They are in parts too flashy. 

I suggest to use a more softened color palette 

anniversary     #E5696F
birthday        #F4985A
business        #FFCC53
calls           #FFF95B
clients         #88D27F
competition     #7FD3B7
customer        #779EDA
favorites       #C587A5
follow up       #C7CEDE
gifts           #4B5978
holidays        #000000
ideas           #B14F0D
issues          #AB7B05
meeting         #999400
miscellaneous   #2C6625
personal        #5F6C3A
projects        #AD7FA8
public holiday  #75507B
status          #5C3566
suppliers       #729FCF
travel          #3465A4
vacation        #204A87

ui- due to the used colors.

Comment 9

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

We might want to be careful with using default colors. If we decide to later use dynamic colors, then users might wonder why their category colors changed after upgrading.

I doubt there is a standard for a such color hashing algorithm, I've seen pidgin im hash chat user-colors, so maybe we should take a look how they do it. Maybe we can even use their algorithm directly.

Another thing that strikes me is the color picker. Obviously the color picker won't be able to allow you to choose those colors, so users might wonder how to return to the default colors (no, using about:config is not an option ;-). I have some graveyard code that allows specifying the colors used for the colorpicker widget, I might be able to dig that up so we can allow the users to pick those colors. It might also be a valuable addition to toolkit.

Comment 10

[Przemyslaw Bialik]

Assigning to nobody - I'm not working on this and it has to be decided how to tackle this (my proposal would be to use good static defaults - maybe even investigate what colors for categories other calendaring soft is using and use hashes for new user added categories - proposed colors; also there's still the l10n case - comment #5)
Nominating wanted, as it would be good to have for 1.0

Comment 11

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

I have a WiP patch for this. It converts the category name to a color using a simple hash algorithm. I have used the colors available in the default color picker to make sure the user has no surprises when choosing colors.

The patch allows generating default colors even for localized builds by executing a specific .js file via xpcshell. I need to think about how to best include the prefs in our files.

Comment 12

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

Attachment: Fix - v2

This patch takes care. The default set of category colors calculated by this algorithm isn't exactly the prettiest, but there's not much we can do if we use an automatic creation algorithm. Even if we do change the algorithm for the en-US colors, the localized colors might not look that good then either.

What we could do is reduce the set of colors for automatic generation, but that means its more probable, that two default categories get the same color.

Comment 13

[Markus Adrario [:Taraman]]

Comment on attachment 429109 [details] [diff] [review]
Fix - v2

Taking the review from Stefan.

Although I'm not done yet some comments on the patch:
Patch is bitrotten, change from Bug 349987 has to included into Patch. I will post an updated version hereafter.

I think we should move categories.properties to subdir /preferences. Although categories strings may occur outside preferences until now they don't. Since category handling code for runtime is in the views, I believe this properties file will not be used outside prefs. In fact this was the first place I searched for the file.

why don't you put the new helper-functions in calUtils.jsm i.s.o. calUtils.js?
Quote from calUtils.jsm:
>// New code must not load/import calUtils.js, but should use calUtils.jsm.>
>
>EXPORTED_SYMBOLS = ["cal"];
>let cal = {
>    // new code should land here,

Comment 14

[Markus Adrario [:Taraman]]

Attachment: Fix v2.1

The debitrotted patch with change from Bug 349987

Comment 15

[Markus Adrario [:Taraman]]

Comment on attachment 437607 [details] [diff] [review]
Fix v2.1

When unchecking the "use Color" box in the edit Dialog I get the following error in the error-console:
>Warning: Expected color but found 'null'.  Error in parsing value for 'background-color'.  Declaration dropped.
>Source File: chrome://calendar/content/preferences/editCategory.xul
>Line: 0
This is similar to Bug 349987.

>+    if (!customColorSelected && document.getElementById("useColor").checked) {
>+        // Color is wanted, choose the color based on the category name's hash.
>+        document.getElementById("categoryColor").color = cal.hashColor(newValue);
>+    }
This function is not working for me. No automatic color is assigned.
No error message is shown.

>+/**
>+ * Handler function to be called when the color picker's color has been changed.
>+ */
>+function colorPickerChanged() {
>+    document.getElementById('useColor').checked = true;
>+    customColorSelected = true;
>+}
This function is not called by the color-picker:
>        <colorpicker id="categoryColor"
>                 type="button"
>                 onchange="document.getElementById('useColor').checked=true;"/>

If we touch editCategory.xul here, shouldn't we fix the indentation on this file as well in this patch?

with this and the stuff from comment #13: r-

Comment 16

[Pander]

Please see also https://bugzilla.mozilla.org/show_bug.cgi?id=733338

Comment 17

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

Wow this is an old bug, I just found it again! It shouldn't be too hard to update the code, I'm on it as soon as I get my builds back up. I've filed bug 736280 to get new colors into the toolkit colorpicker widget.

Comment 18

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

(In reply to Markus Adrario [:Taraman] from comment #15)
> Comment on attachment 437607 [details] [diff] [review]
> Fix v2.1
> 
> When unchecking the "use Color" box in the edit Dialog I get the following
> error in the error-console:
> >Warning: Expected color but found 'null'.  Error in parsing value for 'background-color'.  Declaration dropped.
> >Source File: chrome://calendar/content/preferences/editCategory.xul
> >Line: 0
> This is similar to Bug 349987.
Where it was likely fixed, doesn't show up anymore.

> 
> >+    if (!customColorSelected && document.getElementById("useColor").checked) {
> >+        // Color is wanted, choose the color based on the category name's hash.
> >+        document.getElementById("categoryColor").color = cal.hashColor(newValue);
> >+    }
> This function is not working for me. No automatic color is assigned.
> No error message is shown.
For me a color is assigned as soon as I click add a new category, enter a name for the category and then click the "use color" checkbox. I guess this could be changed to change the color while the user is entering the text though to make it more obvious.


> If we touch editCategory.xul here, shouldn't we fix the indentation on this
> file as well in this patch?
Fixed in debitrot.

Comment 19

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

Attachment: Fix - v3

Comment 20

[Markus Adrario [:Taraman]]

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

I found some problems in this patch.

>function doOK() {
>    let color = document.getElementById("useColor").checked ?
>                document.getElementById("categoryColor") :
>                null;
.color missing in line 3 should read:
    document.getElementById("categoryColor").color :
else no color gets assigned and outcome is always "transparent"

in editCategory.xul, the colorpicker onchange has to be updated:
><colorpicker id="categoryColor"
>                 type="button"
>                 onchange="document.getElementById('useColor').checked=true;"/>
    onchange="colorPickerChanged()"/>
else the mentioned Function is never called.

style nit in jar.mn:
> content/calendar/preferences/editCategory.xul          (content/preferences/editCategory.xul)
>+content/calendar/preferences/editCategory.js          (content/preferences/editCategory.js)
> content/calendar/preferences/general.js                (content/preferences/general.js)
indentation should be fixed

With these fixed, the patch works as expected.
I'll upload a fixed patch.

r-

Comment 21

[Markus Adrario [:Taraman]]

Attachment: fix V3.1

Patch updated to fix the above mentioned problems.

r=markus

Comment 22

[Martin Schröder [:mschroeder]]

Comment on attachment 616535 [details] [diff] [review]
fix V3.1

Markus, Philipp: What is the status of this patch? checkin-needed?

Comment 23

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

Looks pretty much checkin-needed to me :-)

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/920018c90abc