(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'

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?

(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

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.

(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

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.

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.

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.

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 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.

(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?

(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.

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 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.

(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!

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.

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

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.

(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?

(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.

(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.

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

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 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.

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 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).

(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.

(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.

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)

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 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.

(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?

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?

(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.

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 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.

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.

(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)

patch checked in (with some whitespace changes)

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

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