306244 - Impossible to unset calendar color when set

Status: RESOLVED WONTFIX

(Calendar :: Sunbird Only, enhancement)

Description

[Robin Edrenius]

It's currently impossible to unset a calendar color when it's set

Comment 1

[Robin Edrenius]

Attachment: Add string to calendar.dtd

Comment 2

[Robin Edrenius]

Attachment: Patch v1

Comment 3

[Joey Minta]

Comment on attachment 194103 [details] [diff] [review]
Patch v1

    document.getElementById("calendar-color").color = 
	calManager.getCalendarPref(gCalendar, 'color');
    document.getElementById("calendar-uri").value = gCalendar.uri.spec;
+   if(calManager.getCalendarPref(gCalendar, 'color'))
+	document.getElementById("useColor").checked = true;

Gettings prefs is a somewhat expensive (slow) call.  Better would be to get the
color once, save it in a variable and use it in both places

+   <script type="application/x-javascript">
+   var oldColor="#000000";

This belongs in calendarProperties.js

(Also, if you put javascript in a xul file, you need to enclose it in CDATA
tags.)

+				label="&calendarproperties.useColor.label;"
This string already exists, and duplicating it is a bad idea.  (If you're going
to use it here, the best solution would probably be to remove the one in
prefs.dtd and fix editCategory.xul as well.)

+++ calendar/resources/content/calendarProperties.xul	
For consistency, you should also make this an option in calendarCreation.xul. 
There, once you select a color, you are forced into picking one.

Please keep all changes to all files in one patch.

Comment 4

[Robin Edrenius]

Attachment: Patch v2

Changes made to reflect jmnitas comment.

Comment 5

[Joey Minta]

Comment on attachment 194110 [details] [diff] [review]
Patch v2

+   var oldColor="#000000";

variables like this (that aren't included in a function) should get a 'g' in
their name, ie gOldColor.

+   function toggleColor()

Nit: cut and paste is fine, but you should make sure to tweak it so that it
matches the style of the new location.	(fix indenting, '{' location)

+   if(document.getElementById("useColor").checked)

Nit: 'if' gets a space after it, before the '('

+				     
onchange="document.getElementById('useColor').checked=true;"/>

Nit: lines longer than 80 chars should be broken up into 2 lines.  (same in
calendarProperties.xul)
    document.getElementById("calendar-color").color = 
-	calManager.getCalendarPref(gCalendar, 'color');
+	calendarColor;

Nit: This is the reverse.  This line is no longer too long, so it shouldn't be
broken up.
    xmlns:nc="http://home.netscape.com/NC-rdf#">
-    
+

Nit: Watch for random whitespace changes.

+		     <checkbox	id="useColor"
+				label="&calendarproperties.useColor.label;"
+				oncommand="toggleColor()"/>

As this is right now, it looks bad.  We need to figure out a nicer way to
present this option to the user.  Unfortunately, I don't have any good
suggestions of the topc of my head.  Maybe someone else in #calendar does.

Comment 6

[Robin Edrenius]

Attachment: Patch v3

This should fix everything except the last thing about it looking weird.

Comment 7

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

What does it mean when a calendar doesn't have a color set? IT needs some color
to draw the event boxes, so 'no color' doesn't seem to make sense to me.

Comment 8

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

Sorry, but we don't need ui to unset the calendar color. It add ui (which
confuses users) to make the calendar look weird (which also confuses users). I
really fail to see any usecase for this.

Comment 9

[Joey Minta]

Comment on attachment 194102 [details] [diff] [review]
Add string to calendar.dtd

removing review request, patch is marked obsolete

Comment 10

[Joey Minta]

Comment on attachment 194114 [details] [diff] [review]
Patch v3

Removing review request, see mvl's comment #8, although depending on how the new views implementation goes, we may want to reconsider this.

Comment 11

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

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

Comment 12

[gekacheka]

The calendar color is used as the *background* color.  The events are still visible because the foreground color is present.

With transparent events, the background color of the boxes (workday/non-workday) still shows through even if you have a full day.

The calendar is shown in the calendar list with no color unless you set one, and it is currently impossible to return it to that state after you set a color.  (This is more evident if your OS theme settings are such that the calendars-list pane background is not white; on windows: control panel | display properties | appearance | item: window)

Comment 13

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

(In reply to comment #12)
> The calendar color is used as the *background* color.  The events are still
> visible because the foreground color is present.

All you wold see is a text and a border. Would look weird to me. Not a good idea.

> The calendar is shown in the calendar list with no color unless you set one,
> and it is currently impossible to return it to that state after you set a
> color.

That's a bug, not a feature. It should default to some color.

Comment 14

[Robin Edrenius]

(In reply to comment #13)
> That's a bug, not a feature. It should default to some color.
> 

It does default to some color, however, once you set a different color it's impossible to set it back to the default color.

Comment 15

[Stefan Sitter [:ssitter]]

(In reply to comment #14)
> It does default to some color, however, once you set a different color it's
> impossible to set it back to the default color.

Currently a new calendar has no color assigned. If you open the calendar properties dialog you will see the following error in jsconsole:
  Error: Error in parsing value for property 'background-color'. Declaration
  dropped. Source File: chrome://calendar/content/calendarProperties.xul Line:0

Anyway there is a default color used for displaying the events. But this color is not assigned to the calendar.

I agree with mvl that there should not be extra UI to unset the calendar color. Instead the calendar should use a default color from the standard color palette. This way the user could only change the color, not set/unset the color. (I hope you understand what i mean)

Comment 16

[Robin Edrenius]

(In reply to comment #15)
> Instead the calendar should use a default color from the standard color
> palette. This way the user could only change the color, not set/unset the
> color. (I hope you understand what i mean)
> 

What I would like is a 'default' option.

Comment 17

[gekacheka]

I think the default calendar event color is (or should be) part of the theme (defined in CSS).

Since it is part of the theme, it should change if the user changes theme.

Therefore need to be able to unset the the calendar color to get back to the default theme color.

Comment 18

[Stefan Sitter [:ssitter]]

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