268084 - UI to set colors for categories

Status: RESOLVED FIXED

(Calendar :: Sunbird Only, defect)

Description

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

Comment 1

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

(sigh. stupid enter)
After bug 171657, there still needs to be UI to set the colors for categories.
It should set prefs like 'calendar.category.color.vacation' to '#FF0000'

Comment 2

[Simon Paquet [:sipaq]]

Ok, IMO the UI should go into the general prefs section.

I made a small mockup, which shows where I want to go. It is located at
http://www.babylonsounds.com/sunbird/pref_mockup.png

Instead of the words for the colors (like blue, green, etc.) in the mockup, I
would like to see a colorpicker. 

Comments?

Comment 3

[Dominik Seelow]

(In reply to comment #2)
> I made a small mockup, which shows where I want to go. It is located at
> http://www.babylonsounds.com/sunbird/pref_mockup.png
> 
SNIP
> 
> Comments?

I think, the GUI should look similar to the 'Labels' settings in Mozilla and
Thunderbird, i.e. with the selected colour directly being shown as a colour.
Plus 'add category' and 'remove category' buttons, of course.

However, mail labels are hardcoded with numbers up to 5 so the code cannot
easily be reused. But maybe a solution for Calendar/Sunbird could be used in the
suite for enhancement https://bugzilla.mozilla.org/show_bug.cgi?id=114656

Comment 4

[Joey Minta]

Attachment: *Rough* draft of UI

This is a rough draft I put together for a UI.	All the widgets and most of the
functionality are there.  It adds an 'edit category' box to the right of the
category list, to avoid openning another window.  I think that having a whole
dialog just for 'Name' and 'Color' is no good.	The edit box is disabled until
a user clicks 'edit' or 'add'.	A screenshot of the changed look is here:
http://www.nd.edu/~jminta/sunbird/color-prefs.png

There's still a few problems with this, though, that I couldn't seem to solve. 
I'm attaching this in the hopes someone else can take it from here.

Known Issues:
-not localized
-individual prefstrings aren't working right (This part is ugly anyway and
should probably be scrapped.)
-colorpicker doesn't reset after editting/adding a category
-colors are shown in #FFFFFF form (one option might be to apply these colors as
the style for the text, background, or border of the color cell, since xul
doesn't let you add a colorpicker to a list/tree.

Comments/advice on how I should go about solving these problems are most
welcome, or, as I said, anyone else can take this and run with it.

Comment 5

[Simon Paquet [:sipaq]]

(In reply to comment #4)
> This is a rough draft I put together for a UI. All the widgets and most of the
> functionality are there. It adds an 'edit category' box to the right of the
> category list, to avoid openning another window.  I think that having a whole
> dialog just for 'Name' and 'Color' is no good. The edit box is disabled until
> a user clicks 'edit' or 'add'. A screenshot of the changed look is here:
> http://www.nd.edu/~jminta/sunbird/color-prefs.png

Thanks for your work.
But IMO your UI is quite confusing for the average user. Changing the color
should lie in a dialog opened by pressing the edit button. Changing the color
and changing the name should go together IMO.
 
> There's still a few problems with this, though, that I couldn't seem to solve. 
> I'm attaching this in the hopes someone else can take it from here.
> 
> Known Issues:
> -not localized

For the strings in your XUL code, you will have to mov these strings into
prefs.dtd and just call the relevant entities in your XUL code. For dealing with
strings in the js code, I recommend reading
http://www.xulplanet.com/tutorials/xultu/locprops.html

Comment 6

[Joey Minta]

Attachment: Not so rough draft

Simon,
   Thank you very much for your help/feedback!	The website helped, and this
new attempt has almost all the problems I listed solved.  It's localized.  It's
fully functional, and the colorpicker problem disappeared once I moved to a
dialog mode as you suggsted.  Screenshots are here:
http://www.nd.edu/~jminta/sunbird/color-prefs2.png (w/o dialog open)
http://www.nd.edu/~jminta/sunbird/color-prefs3.png (with dialog open)

The only remaining issue is how better to display the colors.  The colorpicker
seems to have way too many to name all of them, so I'm open to suggestions as
to how it should look.

I'll attach the new editCategory.xul file once that issue is cleaned up and the
patch is ready for review.

Comment 7

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

I would suggest to display the colour as a coloured box. In the end, how it
looks it what you care about, not what it is named.

Comment 8

[Joey Minta]

Attachment: Category Colors UI

Alright, colored boxes have been included.  Screenshots here:
http://www.nd.edu/~jminta/sunbird/color-prefs4.png (w/o dialog open)
http://www.nd.edu/~jminta/sunbird/color-prefs5.png (with dialog open)

I did diff -urN to include the new editCategory.xul in the diff, let me know if
this should be done a different way.

MVL, since this is your bug and you created the back-end, I thought I'd ask for
your review.

Comment 9

[Joey Minta]

Attachment: Category Colors UI v2

Overnight I realized about half a dozen ways a user could mess up the system. 
This new version includes a bunch of error checks to prevent this.

Comment 10

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

Comment on attachment 181058 [details] [diff] [review]
Category Colors UI v2

>diff -urN Sunbird/content/calendar/pref/calendarPref.xul mozpatch/content/calendar/pref/calendarPref.xul
>       var gCategoryList;
>+      var gCategoryColorList;

Would it be possible to joins those two arrays into one?
gCategoryList[i]={name:'foo', color:'#ff0000'};

>+        if(gCategoryColorList.length!=gCategoryList.length ) {

Try to keep a consitent style. For example, thise line cool be:
	if (gCategoryColorList.length != gCategoryList.length) {
Same goes in other places.


>      <textbox id="categories" prefstring="calendar.categories.names" hidden="true"/>
>+     <textbox id="categoryColors" prefstring="calendar.categories.colors" hidden="true"/>

There really is a pref calendar.categories.names, which is one string of all
the names. But the pref calendar.categories.colors isn't used. Do you need it?
I think you can just read/set the real prefs
(calendar.categories.colors.catname) directly.

Comment 11

[Joey Minta]

(In reply to comment #10)
> Would it be possible to joins those two arrays into one?
> gCategoryList[i]={name:'foo', color:'#ff0000'};

It's possible, but I think the code loses some clarity when the 'for' loops
become +=2.  I'm also not sure if a bit of efficiency might be lost adding in
all of the colors to gCategoryList at every startup.  Using 'join' for a new
array seems simpler.  You tell me, though.

> Try to keep a consitent style.
Will do.
 
> There really is a pref calendar.categories.names, which is one string of all
> the names. But the pref calendar.categories.colors isn't used. Do you need it?
> I think you can just read/set the real prefs
> (calendar.categories.colors.catname) directly.

The main point of this was to try and preserve the function of the Preferences
window's Cancel button to undo all changes made.  Looking at it, this
preservation isn't perfect yet though, with my system.  Tell me, is the back-end
you built smart enough to handle this on its own, or do I need to make sure the
colors are reset to their old values on cancel?

Finally, I'm still not happy with the handling of spaces.  It'd be much simpler
if they're forbidden all together in category names.  Right now, there can still
be a problem if someone names two category 'foo blue' and 'foo red' since the
color preference is "calendar.categories.colors.foo" for each.  Is it acceptable
if I forbid spaces or do I need to try to work around this?

Comment 12

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

(In reply to comment #11)
> (In reply to comment #10)
> > Would it be possible to joins those two arrays into one?
> > gCategoryList[i]={name:'foo', color:'#ff0000'};
> 
> It's possible, but I think the code loses some clarity when the 'for' loops
> become +=2.

All the element of the arrays will be objects. So you don't use +2. just
gCatList[i].color

> The main point of this was to try and preserve the function of the Preferences
> window's Cancel button to undo all changes made.

Ok, didn't think of cancel yet. I wonder how other panels, like helper
applications or languages deal with it.

> Is it acceptable
> if I forbid spaces or do I need to try to work around this?

Please don't forbid them. You can replace them with _ in prefnames for example.

Comment 13

[Joey Minta]

Attachment: Category Colors UI v3

Dare I say finished?

Changes since last version:
-No more gCategoryColorList and no more 2nd hidden textbox.  Color prefs are
called from the prefService whenever needed.
-Style consistency fixed.
-To solve the Preferences window's 'Cancel' button problem, I added a new
preference: 'calendar.category.backup'.  This preference is a string that holds
the old category name and color of any category that is changed.  (It is smart
enough to only add one entry per category.)  I then modified the prefBird.xul
and pref.xul files' ondialogcancel functions to call a function that reads this
data and resets the colors appropriately.  Note: This data is in string form,
which creates a i+=2 for loop.	The object method didn't work as well, because
objects can't be saved as a strings easily.

I've tested this with calendar for Firefox/Linux (using 2005011113-cal as a
base) and I found no problems.

Comment 14

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

Comment on attachment 181253 [details] [diff] [review]
Category Colors UI v3

>diff -urN Sunbird/content/calendar/pref/calendarPref.xul mozpatch/content/calendar/pref/calendarPref.xul
>+           catch (ex) { //Throws exception if color is not defined.
>+              categoryPrefBranch.setCharPref(categoryNameFix, "#FFFFFF");

It should be possible to have a category without a color set. If you have 10
categories, and only want to make one of them stand out, you shouldn't be
forced to set colors for all of them. Also, in the future, it should be
possible to assign an icon instead of a color to a category.

>+          window.openDialog("chrome://calendar/content/pref/editCategory.xul", "addCategory", "modal,centerscreen,chrome,resizable=no", "", "#FFFFFF", addTitle);

Nit: make the line wrap at 80 chars. (same a few lines down)

>+      function saveCategory(categoryName,categoryColor) {
>+               if(confirm(overwrite)) {

From chrome, you should not use confirm, but use the promptservice.

>+         if(list.selectedIndex == -1) {
>+            }
>+            else {
weird indenting.

>diff -urN Sunbird/content/calendar/pref/editCategory.xul mozpatch/content/calendar/pref/editCategory.xul
>+<?xml-stylesheet href="chrome://global/skin/global.css" type="text/css"?>
>+<?xul-overlay href="chrome://global/content/dialogOverlay.xul"?>

Add a license header.

>+<script>
Add a type, and a CDATA.

>+function doOK()
>+{
>+   window.opener.saveCategory(document.getElementById("categoryName").value,document.getElementById("categoryColor").color);
Please make this line wrap too.

>+  <vbox id="dialog-box">
>+             <grid>

Indenting?


The rest looks good.

Comment 15

[Joey Minta]

(In reply to comment #14)
> It should be possible to have a category without a color set. 

This can be done, but I think it will be a bit ugly.  A couple issues to consider:
-What should the color column display if no color is selected?  Leaving it blank
would make it the listbox background color (=white).
-Once a color is chosen, it is impossible to select <no color> from a
colorpicker.  If <no color> is a legitimate value, how should this option be
available to the user in 'Edit Category'?
-When a colorpicker is initialized without assigning a color to it, it sets
itself to the gray color of the window background.  If we're going to allow <no
color>, the 'Add Category' dialog should obviously use this.  However, if a user
doesn't touch this when creating a category, this could be interpretted as 'I
don't want a color' or as 'I want this gray color'.  This could lead to some
confusion.

> If you have 10
> categories, and only want to make one of them stand out, you shouldn't be
> forced to set colors for all of them. 

In light of the above issues, I set the categories without defined colors to
white (#FFFFFF), since this would be unobtrusive and identical for each.  As
long as every category which hasn't been manually updated by the user is the
same (and isn't bright red or something), I think it suffices to keep them from
standing out.  If there is a better color to default things to, let me know.

> Also, in the future, it should be
> possible to assign an icon instead of a color to a category.

I don't think this prevents icons from being set in the future.  Again, if the
color is set to the default, then setting an icon should work nicely just as if
no color was chosen.

In short, I think we need a default color for categories without colors, but I'm
open to suggestions as to what it should be.

All the other changes will be made.  Thanks again for the thorough review!

Comment 16

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

I was more talking about the backend. If there is no color set for a certain
catergory, it should not assign a (somewhat random) default, but just do
nothing. If the UI can't handle it for now, it isn't a real problem imo.

Comment 17

[Joey Minta]

Attachment: Category Colors UI v4

All changes made with the exception of the default value for undefined colors
just discussed.

Comment 18

[Joey Minta]

I'm CC'ing Rod Whiteley on this because once the patch is checked in, there
could be some issues with users who have doctored their userchrome.css files to
take advantage of the category colors.  I believe leaving those changes in the
.css file will result in any changes made in the preferences window being
overwritten every time calendar is loaded.  As the author of the tutorial on how
to make those changes, I thought you at least might want a heads-up, or perhaps
include a warning for when the next version comes out.

Comment 19

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

(In reply to comment #17)
> All changes made with the exception of the default value for undefined colors
> just discussed.

Does that mean you are going to change it? or will you leave it as it is now?

Comment 20

[Joey Minta]

(In reply to comment #19)
> (In reply to comment #17)
> > All changes made with the exception of the default value for undefined colors
> > just discussed.
> 
> Does that mean you are going to change it? or will you leave it as it is now?
> 

I'd like to leave it as is because of the above issues.  The new patch should
have  everything else in it.  Sorry for the confusion.

Comment 21

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

(In reply to comment #15)
> In light of the above issues, I set the categories without defined colors to
> white (#FFFFFF), since this would be unobtrusive and identical for each. 

A better choice would be to not set it at all. White might look like the default
on your system, but not on mine.
There is no problem with the pref not being set. The system handles that just fine.

Comment 22

[Joey Minta]

Attachment: Category Colors UI v5

The UI no longer sets a color preference if one was not previously defined by
the user.  To accomodate this, several more try/catch sets were necessary.  In
the color column it now displays a localizable "(none)" if there was no
preference assigned.  I also had to modify the editCategory dialog somewhat to
allow a user to clear this preference and therefore return the value to
"(none)".  A screenshot showing both these changes is available at
http://www.nd.edu/~jminta/sunbird/color-prefs6.png

Comment 23

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

Just an idea for a different UI:

Name: [          ]
[ ] Use color: [   ]

(though i don't like the aligning of that, need to figure that out.) The point
is a checkbox, not a button for clear.

Comment 24

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

Comment on attachment 181892 [details] [diff] [review]
Category Colors UI v5

Another idea: Can you set a property on the window instead of a pref to store
the backup? That would allow to set an array, instead of this string.

Comment 25

[Joey Minta]

Attachment: Category Colors UI v6

This version tries to bring in the changes asked for in comment 23 and comment
24.  It now uses registerCancelCallbackFunc, so no modifications of
prefBird.xul or pref.xul are now necessary.  I also switched the previous Clear
button to a checkbox as requested.  To avoid the alignment issue, I centered
the checkbox/colorpicker.  A screenshot is available at
http://www.nd.edu/~jminta/sunbird/color-prefs7.png

Comment 26

[Simon Paquet [:sipaq]]

Comment on attachment 182501 [details] [diff] [review]
Category Colors UI v6

>--- Sunbird/locale/en-US/calendar/prefs.dtd     2004-11-03 17:05:55.000000000 +0000
>+++ mozpatch/locale/en-US/calendar/prefs.dtd    2005-05-03 16:22:26.000000000 +0100
>@@ -124,12 +124,15 @@
>
>[...]
>-
>+<!ENTITY pref.categories.name.label   "Name">
>+<!ENTITY pref.categories.color.label  "Color">

jminta, take a look at attachment 182344 [details] [diff] [review] (bug 290228). That patch moves often
used strings like those seen above to a shared location in calendar.dtd. It
would be great if your patch could take advantage of that (provided that mvl
sets review+ on my patch).

Comment 27

[Joey Minta]

(In reply to comment #26)
> jminta, take a look at attachment 182344 [details] [diff] [review] [edit] (bug 290228). That patch moves
often
> used strings like those seen above to a shared location in calendar.dtd. It
> would be great if your patch could take advantage of that (provided that mvl
> sets review+ on my patch).

While I think that's a good idea, my only issue with it is the colons in the
strings.  Here I'm using 'Name' and 'Color' for column titles in a table, and I
think having them end in colons then doesn't look correct.  If you and/or MVL
prefer though, I can take a substring of them in my patch that cuts off the
colon.  I'm not sure whether that way or the current one is the better option. 
Personally, I'd suggest keeping the strings in .dtd files without colons, and
letting those areas that want colons add them.

Comment 28

[Rod Whiteley]

(In reply to comment #27)
> Personally, I'd suggest keeping the strings in .dtd files without colons, and
> letting those areas that want colons add them.

Be aware that fr-FR and perhaps other locales place a space before a colon.

It is also possible that some languages use plural forms for column headings.

To be sure that you avoid problems, you have to put both forms in the DTD, even
though they are almost identical in English.

Comment 29

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

You can share entries if they are used in the same way, like a label in a
dialog. A label and a column heading are different enough to warrant different
entries. You can still put them together in one file though. (for example, the
listbox in the main calendar window might one day use it)

Comment 30

[Joey Minta]

Attachment: Category Colors UI v7

After so many attempts, what's one more version? ;-)

Looking through the Calendar Help project, several other bugs were identified
in the category listbox.  I've modified this slightly to address them, although
I couldn't find any bugzilla bugs reported about them.

1.) Edit/Remove buttons should be disabled when no category is selected.
-I thought implementing a broadcaster for just this little bit was slightly
overkill.  There are only 3 spots where the disabled status needs to be
changed, so it's +6 lines either way, with a broadcaster, or disabling them
directly.

2.) The category list should be sorted alphabetically.
-For some reason, .sort() was giving me a bit of trouble, so I had to implement
it both in updateCategories and after deleting/modifying a category in order
for it to sort properly.  Calling it in updateCategories should suffice,
though.  If someone wants to point out where I'm going wrong, please do!

Everything else remains exactly the same as v6.

Comment 31

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

Comment on attachment 182809 [details] [diff] [review]
Category Colors UI v7


>       function addCategory()
>       {
>-        var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>-                                      .getService(Components.interfaces.nsIPromptService);
>+          window.openDialog("chrome://calendar/content/pref/editCategory.xul",
>+                  "addCategory", "modal,centerscreen,chrome,resizable=no",
>+                  "", null, addTitle);

Don't remove the use of the promptservice. You can't use normal prompt, alert
etc from chrome. (same in editCategory, and other places)

>+          var categoryNameFix = gCategoryList[list.selectedIndex].toLowerCase();
>+          categoryNameFix = categoryNameFix.replace(' ','_');

Move that into a helper function.

>+<script type="application/x-javascript">
>+ <![CDATA[
>+   var oldColor="#FFFFFF"

Magic numbers are no cool. What is so special about white?

>+     document.getElementById("categoryName").value=window.arguments[0];

General comment: The style i prefer is with spaces around the = (so: foo =
bar).

>+         //The only way to reset a colorpicker is to remove it.
Really? If so, that is a bug and shoulb be fixed, instead of worked around. At
least reference to a bug number in this comment.

If you fix those nits, this patch looks ok to me.

Comment 32

[Joey Minta]

(In reply to comment #31)

> >       function addCategory()
> >       {
> >-        var promptService =
Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
> >-                                     
.getService(Components.interfaces.nsIPromptService);
> >+          window.openDialog("chrome://calendar/content/pref/editCategory.xul",
> >+                  "addCategory", "modal,centerscreen,chrome,resizable=no",
> >+                  "", null, addTitle);
> 
> Don't remove the use of the promptservice. You can't use normal prompt, alert
> etc from chrome. (same in editCategory, and other places)

I don't understand.  Promptservice was previously used to prompt for the name of
the category.  Since editCategory.xul is now being opened, no prompt/alert is
ever called.  var promptService was local to addCategory(), so removing it
shouldn't cause problems anywhere else, and it isn't needed here.

> >+<script type="application/x-javascript">
> >+ <![CDATA[
> >+   var oldColor="#FFFFFF"
> 
> Magic numbers are no cool. What is so special about white?

This prevents a user from clicking 'Use Color' without selecting a color, since
otherwise clicking the checkbox leaves the colorpicker in the 'null' state.  I
felt this was better, but it can go if you want.  Yes/No?

Comment 33

[Joey Minta]

Note about the promptService issue: I will add promptService to the one alert
for blank categories in saveCategory().  (It is already declared there for the
confirm box about overwriting duplicates.)  Is that all you meant?

Comment 34

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

(In reply to comment #32)
> I don't understand.  Promptservice was previously used to prompt for the name of
> the category.  Since editCategory.xul is now being opened, no prompt/alert is
> ever called.  var promptService was local to addCategory(), so removing it
> shouldn't cause problems anywhere else, and it isn't needed here.

Yeah, i was confused. window.openDialog is ok.

> This prevents a user from clicking 'Use Color' without selecting a color, since
> otherwise clicking the checkbox leaves the colorpicker in the 'null' state.  I
> felt this was better, but it can go if you want.  Yes/No?

Ok, a default is no problem. But maybe some other value, that is more visible in
the current theme (in the views i mean, not in the dialog). White will likely
blend into the background.

Comment 35

[Joey Minta]

Attachment: Category Colors UI v8

Changes made from previous version:
-promptService added to the one alert that was missing it.
-helper function fixName now added/used
-default for 'Use Color' when adding a new category is now #000000 (black)
-styling fixed
-colorpicker reset now done correctly=no bug

Comment 36

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

Comment on attachment 182943 [details] [diff] [review]
Category Colors UI v8

>+           window.openDialog("chrome://calendar/content/pref/editCategory.xul",+                   "editCategory", "modal,centerscreen,chrome,resizable=no",

Do you any idea what is going on here? That second part should be on a new
line.

Comment 37

[Joey Minta]

Attachment: Category Colors UI v8a

I have no idea what went wrong with the last diff.  I ran it again, though, and
as I best I can tell, the problem has disappeared.

Comment 38

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

(for the next patch, please use cvs to create it. That allows other to cleanly
apply it. The directory structure of cvs and calendar.jar are not the same)

Comment 39

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

patch checked in (with some whitespace changes)

Comment 40

[Simon Paquet [:sipaq]]

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

Comment 41

[Reed Loden [:reed]]

The bugspam monkeys have been set free and are feeding on Calendar :: Sunbird Only. Be afraid for your sanity!