Closed Bug 434092 Opened 16 years ago Closed 16 years ago

Allow disabling calendars completely (i.e on an outage)

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: Fallen, Assigned: Fallen)

Details

Attachments

(7 files, 2 obsolete files)

Attached patch Allwo disabling calendars - v1 (obsolete) β€” β€” Splinter Review
In some cases (i.e an outage), we want to be able to disable calendars completely (i.e no alarms, no getItems, no additions).

This patch takes care. It kind of depends on some other patches that are in review, therefore not requesting review yet.

Daniel, please take a look at this patch to see if you think it fixes the bug in the right way.

uiwanted: We need a disabled checkbox image
Attachment #321325 - Flags: ui-review? → ui-review?(christian.jansen)
I like it.
The purpose of this bug seems a little vague to me so I'll add some comments.  I think that there are two different cases:

1) There is an outage or a calendar is readonly.  In this case I think we should still be able to change the calendar's properties, receive pre-existing alarms, and see the events in the views (assuming that the calendar was loaded into memory before the outage).  IMO an outage or readonly state should allow everything except to change specific events/tasks in the calendar.

2) A second case is when a user explicitly disables a calendar.  In this case I agree that the calendar should not be loaded at all (and should be unloaded if it's already in memory), there should be no alarms, and we shouldn't be able to change the items in the calendar, but again I think it's probably always wrong to have a read-only dialog for the calendar's properties.  IMO we should always be able to change the properties, just not the events/tasks in the calendar.

Case (2) is even more complicated because there are two aspects:  (a) enabling/disabling a calendar completely and (b) specifying whether the calendar's events appear in the composite view.  Currently it's not obvious how to achieve (a) but we can achieve (b) with the checkbox in the calendar list.

I would prefer that the location of these checkboxes is reversed.  IMO the checkbox in the calendar list should completely enable/disable the calendar because I think that this is the most frequent need and it seems too inconvenient to have to open each calendar's properties to achieve this.

On the other hand, the new checkbox inside the properties dialog could then specify whether this calendar appears in the composite view.  I suggest to "hide" this checkbox in the properties because it seems that people will want to change this setting much less frequently.  (FYI, this checkbox would allow people to receive alarms from a calendar but without showing the calendar's events in the composite.  This is the current purpose of the checkbox in the calendar tree.)
(In reply to comment #3)
> I would prefer that the location of these checkboxes is reversed.  IMO the
> checkbox in the calendar list should completely enable/disable the calendar
> because I think that this is the most frequent need and it seems too
> inconvenient to have to open each calendar's properties to achieve this.

IMO this depends on how often you feel the need for switching off a calendar. I personally have lots of subscriptions I regularly visit, but only a few of those are checked (to be in composite) at the same time.
I think (totally) disabling a calendar is rather an edge case, thus it doesn't deserve a more prominent UI IMO.
If we change the checkbox to disable calendars completely, then users that knew of the old behavior will complain that their alarms are not shown. The act of disabling a calendar is indeed for edge cases like an outage. With this patch, the gdata provider will disable itself if you cancel the password dialog. This way you won't have any further requests when you cancel the dialog, but still have the possibility to re-enable the calendar.

I do see this case can be improved though, i.e when the calendar is cached the calendar could rather go into an "offline" mode, where only for this calendar all events are read from the cache. A certain user action similar to enabling the calendar (possibly manually clicking on "reload remote calendars") could make the calendar go back online. Not sure if this should happen in a different bug or not, depends on how much work it is, how fast I can get this patch ready for review, and what others think to it?
Comment on attachment 321325 [details]
Screenshot - Edit Calendar Dialog (Linux)

Since Christian is on vacation for a couple of weeks, I am taking this one over; ui+.
Attachment #321325 - Flags: ui-review?(christian.jansen) → ui-review+
Comment on attachment 321325 [details]
Screenshot - Edit Calendar Dialog (Linux)

I'm afraid I don't like the wording. 'Switch this calendar on' is weird. 'Switch' is a verb implying an action, but you use a checkbox (status), not a button (action).
I'd prefer 'Calendar enabled', 'Calendar in use' or something to that extend.

Actually, why can't you change the name and url if you just don't want to load the calendar file? I don't think you should disable everything.
Also, the checkbox is too prominent. It's nearly the least important option, but it's all the way to the top. Combined with not disabling everything, you can move it to the other checkboxes.
Although in general its not so prominent, I think for the calendar properties it should be prominent. The use case for this feature is to disable the calendar when the user cancels the login dialog. When the calendar is disabled, no operations should be done on it. I think if its just ordered in with the rest of the calendar properties, users might get confused with all the different options (Disabled, Read-Only, are still ok, but just think what happens when we separate the calendars shown in the today pane from the calendars shown in the calendar views.)

I talked with christian and he also disliked the idea of putting disabled together with the other properties.

Regarding the wording, by clicking the checkbox, you switch the calendar on. This sounds logical to me. "Calendar in use" sounds more as if the calendar were busy. "Calendar Enabled" might be a bit technical.

Unfortunately I don't have a better idea right now though.
Comment on attachment 321324 [details] [diff] [review]
Allwo disabling calendars - v1

Strange, I thought I requested code review on this one!?
Attachment #321324 - Flags: review?(daniel.boelzle)
Comment on attachment 321324 [details] [diff] [review]
Allwo disabling calendars - v1

>-                this.alarmService.findAlarms(this.alarmService.calendarManager.getCalendars({}),
>-                                             start, until);
>+                var cals = this.alarmService.calendarManager.getCalendars({});
>+                this.alarmService.findAlarms(cals, start, until);
why change that?

> function isCalendarWritable(aCalendar) {
>-    return (!aCalendar.readOnly &&
>+    return (!aCalendar.getProperty("disabled") &&
>+            !aCalendar.readOnly &&
>            (!getIOService().offline ||
>             aCalendar.getProperty("requiresNetwork") === false));
please indent the latter two

> <!ENTITY calendarproperties.format.label   "Format:">
> <!ENTITY calendarproperties.location.label "Location:">
> <!ENTITY calendarproperties.name.label     "Name:">
> <!ENTITY calendarproperties.readonly.label "Read Only">
> <!ENTITY calendarproperties.firealarms.label "Show Alarms">
> <!ENTITY calendarproperties.cache.label    "Cache (EXPERIMENTAL, requires restart)">
>+<!ENTITY calendarproperties.enabled.label "Switch this calendar on">
please indent

>     refresh: function() {
>         for each (cal in this.mCalendars) {
for each (var cal in this.enabledCalendars) ?

>+            if (cal.getProperty("disabled")) {
>+                continue;
>+            }
instead of this

>+        ASSERT(aNewItem.calendar, "Composite can't modify item with null calendar", true);
>+        ASSERT(aNewItem.calendar != this, "Composite can't modify item with this calendar", true);

I'd rather like to just throw UNEXPECTED (or another error) here, because a composite calendar must not be used for modification. Same goes for the other item "modifying" methods of composite.

>-            this.mRealListener.onGetResult (aCalendar, aStatus, 
>+            this.mRealListener.onGetResult (aCalendar, aStatus,
>                                             Components.interfaces.nsISupports,
>                                             aDetail, 0, []);
remove the space before opening brace


the patch works good for me (after bitrotting calCompositeCalendar.js), well done!

r=dbo

One follow-up suggestion (don't know whether this has already been discussed): What about visually disabling the calendar in the calendar list, e.g. strike-thru the name or graying it out?
Attachment #321324 - Flags: review?(daniel.boelzle) → review+
(In reply to comment #10)
> why change that?
Leftovers, removed.

> >+<!ENTITY calendarproperties.enabled.label "Switch this calendar on">
> please indent
I thought we wanted to get rid of the randomly aligned indentation, since there will always be an entity name thats longer than the proposed width.

> >+        ASSERT(aNewItem.calendar, "Composite can't modify item with null calendar", true);
> >+        ASSERT(aNewItem.calendar != this, "Composite can't modify item with this calendar", true);
> 
> I'd rather like to just throw UNEXPECTED (or another error) here, because a
> composite calendar must not be used for modification. Same goes for the other
> item "modifying" methods of composite.
Why not notify? the third argument makes the ASSERT() command throw, but makes the error message more useful.



> One follow-up suggestion (don't know whether this has already been discussed):
> What about visually disabling the calendar in the calendar list, e.g.
> strike-thru the name or graying it out?
I had the feeling this already works, due to other patches I've checked in. I'm going to attach a patch to fix up the checkbox image though soon and will make sure the text is grayed.

> 

First part checked in.
Attachment #321324 - Attachment is obsolete: true
Attachment #322909 - Flags: review+
This patch consolidates all the unifinder images into one and puts it into base, and also adds the disabled checkbox. With this patch we can get rid of:

resources/skin/classic/unifinder/*
base/themes/common/priority_*_inversed.png

As a bonus, I have added a checked disabled checkbox, in case we need it some time.
Attachment #322916 - Flags: review?(Berend.Cornelius)
Attached image unifinder-images.png (obsolete) β€”
Attachment #322929 - Flags: review?(philipp) → review+
Alternative checkbox images, as suggested by christian. I darkened the checkbox mark a bit, since I feel that it was hard to differ between disabled and active. I still think its not ideal, but I guess its better than the other one.
Attachment #322956 - Flags: ui-review?(daniel.boelzle)
Attachment #322929 - Attachment description: Disable WCAP calendar when login prompt is canceled → [checked in] Disable WCAP calendar when login prompt is canceled
Attachment #322956 - Flags: ui-review?(daniel.boelzle) → ui-review+
This patch fixes a problem when you have a wcap calendar. The cache checkbox is disabled, click the checkbox, then the cache checkbox is enabled, even though it should not be.
Attachment #323036 - Flags: review?(daniel.boelzle)
Attachment #322917 - Attachment is obsolete: true
Comment on attachment 323036 [details] [diff] [review]
[checked in] Fix checkboxes that are disabled due to a capability

r=dbo
Attachment #323036 - Flags: review?(daniel.boelzle) → review+
Attachment #323036 - Attachment description: Fix checkboxes that are disabled due to a capability → [checked in] Fix checkboxes that are disabled due to a capability
Comment on attachment 322916 [details] [diff] [review]
[checked in] Additional UI for disabling calendars - v1

patch looks good. r=berend. As we discussed Christian already gave me another set of checkbox-images that you could use instead.
Attachment #322916 - Flags: review?(Berend.Cornelius) → review+
Attachment #323057 - Attachment is patch: true
Attachment #323057 - Attachment mime type: application/octet-stream → text/plain
Attachment #323057 - Flags: review?(philipp)
Comment on attachment 323057 [details] [diff] [review]
[checked in] fix calendar manager

r=philipp, I just checked in the patch too.
Attachment #323057 - Attachment description: fix calendar manager → [checked in] fix calendar manager
Attachment #323057 - Flags: review?(philipp) → review+
Attachment #322916 - Attachment description: Additional UI for disabling calendars - v1 → [checked in] Additional UI for disabling calendars - v1
All patches checked in. I couldn't come up with an alternative wording for the checkbox, if someone has a better idea, please file a new bug. Further regressions/patches also in other bugs please.


Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED


It might be a good idea to create unit tests that check if certain operations that should not happen can be done while the calendar is disabled.

Also, a litmus test case for this would be great.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite?
Flags: in-litmus?
Keywords: uiwanted
Resolution: --- → FIXED
Target Milestone: --- → 0.9
Just out of curiosity, what's wrong with re-opening a bug if it has problems?  It seems that all of the relevant comments and patches are already here in a nice, centralized location.

Also, many people can't test new patches until the bugs are closed and the changes go into the nightlies.  It seems more convenient for us if we can simply re-open the bugs if we find problems the next day.
The bug has had quite some traffic, which makes further changes easier to overlook. I guess its ok to reopen this bug, but new bugs that depend on this one would be my preference. This is also the way we've been previously handling problems originating from a certain feature. Just think how many comments a large feature bug like the task view would have if everything was handled in the same bug :-)
Checked in lightning 2008082103 and sunbird 20080820 -> VERIFIED

Litmus Testcase ID# 5879
Status: RESOLVED → VERIFIED
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: