Closed Bug 350323 Opened 18 years ago Closed 17 years ago

show hidden calendars when they are selected.

Categories

(Calendar :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mvl, Assigned: thomas.benisch)

Details

Attachments

(2 files, 2 obsolete files)

I personally never liked the way that the selected calendar in the listbox is the calendar in which new items are created. There is no UI hint to that anywhere. You just have to know it.
Maybe we need some checkbox in the listbox? One that gets remembered across sessions.
I tend to agree that it would be nice if it were a bit more discoverable.  FWIW, iCal behaves just like we currently do.
I think the current situation is rather easy to use. If you create new event/task using the dialog there is a drop-down menu showing the calendar the item is created in. Or just select calendar in Calendars tab and create new events with drag'n'drop. You're done.

What might be useful is to define a default calendar that is selected right after application start. Maybe remembering the calendar that was selected on shutdown would be enough.

Another possibility would be to show the currently selected calendar in the up-to-now unused status bar.
No, the current UI is not easy to use. It's hard to discover the relation between the selected item and the default calendar. And once you discovered it, it is still hard to use. I forget to select the right calendar all the time.
We still need a better solution
(In reply to comment #1)
> I tend to agree that it would be nice if it were a bit more discoverable. 
> FWIW, iCal behaves just like we currently do.
> 

The subtle difference to iCal (and this is our biggest usability issue here) is the fact that you can select a calendar without having its (visibility-)checkmark set. As a result you can enter events into an invisible calendar, which is very confusing.
The simple solution to this is: selection of a calendar in the list must automatically toggle the checkbox to be checked. That's exactly how iCal behaves.
Nice catch, Stefan.  That suggestion sounds reasonable to me and has a good intuitive feel about it.  Offhand, at least, I can't imagine a situation where it would be necessary to permit invisible calendars to be selected.
(In reply to comment #4)
> the fact that you can select a calendar without having its
> (visibility-)checkmark set. As a result you can enter events into an invisible
> calendar, which is very confusing.

See also related Bug 345410

While adding items to invisible calendar is ineed a problem, it still does not fix my original problem: There is no hint that the selected calendar all the way to the left of the screen has anything to do with where new items end up.
When a calendar in the calendar tab is selected, this patch adds
the selected calendar to the composite calendar. The corresponding
checkbox is automatically toggled to be checked.
Attachment #248236 - Flags: first-review?(lilmatt)
I also provided a patch, which selects the stored default calendar in the
calendar tab list right after application start (see Bug #363432).
(In reply to comment #9)
> I also provided a patch, which selects the stored default calendar in the
> calendar tab list right after application start (see Bug #363432).

The bug I refered to is duplicate, please see Bug #362930.
Comment on attachment 248236 [details] [diff] [review]
add selected calendar to composite calendar patch

This looks good.  Could you also port this fix to Sunbird?
As I said before: this patch does not fix the bug as i filed it.
Attachment #248236 - Flags: first-review?(lilmatt)
This patch also includes the port to Sunbird.
Attachment #248236 - Attachment is obsolete: true
Attachment #248498 - Flags: first-review?(lilmatt)
(In reply to comment #12)
> As I said before: this patch does not fix the bug as i filed it.

You're absolutely right, that this patch doesn't fix your described
problem. Nevertheless, I think this patch makes sense and improves
the usability. So if you don't object to this patch in general,
after review this patch can be checked in as part of this bug
or alternatively I can file a new bug.
It's was just a reminder: don't go auto-pilot and mark this bug as fixed after checking in. It should be left open.
No need for another bug now.
Comment on attachment 248498 [details] [diff] [review]
add selected calendar to composite calendar patch v2

r=lilmatt
Attachment #248498 - Flags: first-review?(lilmatt) → first-review+
add selected calendar to composite calendar patch checked in on HEAD and
MOZILLA_1_8_BRANCH
The checked-in patch does not work for me on sunbird. I still can select an hidden calendar, and can still add items to it.
And besides, why was this checked-in without ui-review and second-review?
Using Sunbird/0.4a1 (2006121504) I now see the following misbehavior caused by the checkin: 
A calendar in disabled and not selected in calendar list. Now click the checkbox to enable calendar -> calendar stays disabled. I think the reason is that the calendar gets selected first (calendar will be enabled) and than the checkbox gets clicked (calendar will be disabled again).
Nominating for backout based on comment #18 and comment #19.
(In reply to comment #20)

Agreed.  This shouldn't have landed as it doesn't have r2, and it's apparently causing regressions.  Please back this out for now.
As requested I removed this patch from CVS.
By the way, in which cases is a 2nd review necessary?

Comment on attachment 248498 [details] [diff] [review]
add selected calendar to composite calendar patch v2

Before working on an improved version of this patch I'd like to ask
for UI review.

When a calendar in the calendar tab is selected, the current patch
adds the selected calendar to the composite calendar. The corresponding
checkbox is automatically toggled to be checked. The questions is,
if this behaviour does make sense. Please see also comment #5.

In addition I stumbled about the following problem.
E.g. in Lightning it is possible, that after a calendar was selected
and added to the composite calendar, that the user clicks the checkbox,
so that the calendar is removed from the composite calendar again.
That means, we have again the situation, that a calendar is selected,
which is not part of the composite calendar.
Attachment #248498 - Flags: ui-review?(mvl)
How about something like this?

Frankly, you guys looking/working on that patch seem to have missed the point. It's a user friendliness issue. Like Michiel said, it's just not obvious that the highlighted calendar is the default. Forcing a hidden calendar to be displayed when adding tasks/events is besides the point. For one thing, the user's view might not be in the range where you create the event (you can change the dates after all). So if you're going to force the visibility of a calendar you'd also logically need to force the current view to change to the start date of the event, otherwise you're not any further ahead than you were. I for one, probably wouldn't like that feature.

I have it working as depicted for Lightning, I have the xpi, no cvs access right now, not sure what's involved in that, haven't looked though. If anyone wants the xpi I can attach.
Changing the summary to indicate that this bug has been morphing (sad as it is). bug 366914 filed for the original issue.
Summary: make default calendar for new items more obvious → show hidden calendars when they are selected.
Just to voice my opinion on this bug for the new summary. I don't think this should be implemented. For one thing, if I accidentally click a non-visible calendar which has many items, it takes time to load (ok a few seconds, but still). I prefer the visibility checkbox.

Maybe I'm unique in this, but for another example, I have one calendar I keep invisible which I use to house a few recurring events. I don't need to see these events on my calendar (they popup notifications), but I might want to add one without having to wait a second or two for all existing items to display. I only need to see them if I want to edit something.
I'm not sold on the proposed patch. What is the problem it tries to solve? Adding items to hidden calendars?
(In reply to comment #27)
> I'm not sold on the proposed patch. What is the problem it tries to solve?
> Adding items to hidden calendars?

I'll describe the problem this patch tries to solve in more detail.
You select a calendar in the calendar tab where the checkbox is not
checked, that means the calendar is not part of the composite calendar.
Then you create an event, either by dragging in the view or by pressing
the new event button. The event dialog uses the selected calendar, that
means the event is created in the hidden calendar. After you've finished
with creating the event, the event is not diplayed in the view.
When creating the event by dragging, it seems as if your event creation
failed.

By adding a selected calendar to the composite calendar this problem
is solved.


Comment on attachment 248498 [details] [diff] [review]
add selected calendar to composite calendar patch v2

Hmm, i guess this does solve some problem. ui-review=mvl

I prefer the early-return if there is no calendar chosen (like it originally was in calendarManagement.js).
But i'll leave that up to you.
r2=mvl
Attachment #248498 - Flags: ui-review?(mvl)
Attachment #248498 - Flags: ui-review+
Attachment #248498 - Flags: second-review+
Based on comment #28, I would suggest that the only problem this solves is when you create the event by dragging. As I stated in comment #24, there's no guarantee that an event created using the new event button will even be in the range of the current view, so this proposed patch doesn't solve the supposed issue of the event not being in the view after it's created.

However, the solution I would envision for dragging, instead of adding the hidden calendar to the composite: 1. have the event slowly fade away and/or 2. immediately open an edit event dialog for the event. The problem as I see it is that because it disappears right away, there's currently no way other than adding the hidden calendar to the composite to edit that event.
This patch uses the early return proposed in comment #29.
In addition this patch fixes the problem from comment #19.

tbe->mvl: I'm not sure, if the problem from comment #18 is fixed.
          If not, can you please elaborate your problem in more
          detail.

Due to the changes in the patch I ask for r1 again.
Is it necessary to have r2 and ui-review again?
Assignee: nobody → thomas.benisch
Attachment #248498 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #256150 - Flags: first-review?(mvl)
Comment on attachment 256150 [details] [diff] [review]
add selected calendar to composite calendar patch v3

>+            clearTimeout(gCalendarListSelectTimeout);
This timeout looks like a hack to me. It really needs comments about why it's needed etc.
But i very much prefer not needing it at all. How about calling event.preventDefault in the onCalendarListSelect(), when you detected that the calendar needs to be added? That way, the checkbox won't receive the call. I think. If event bubbles the other way (i can't remember right now), you can stop the bubbling somewhere else. Either way, make sure the other handler won't get called.
(In reply to comment #32)
> (From update of attachment 256150 [details] [diff] [review])
> >+            clearTimeout(gCalendarListSelectTimeout);
> This timeout looks like a hack to me. It really needs comments about why it's
> needed etc.

I agree that the timer approach is quite hacky. But I haven't found
a better way to fix this task.

> But i very much prefer not needing it at all. How about calling
> event.preventDefault in the onCalendarListSelect(), when you detected that the
> calendar needs to be added? That way, the checkbox won't receive the call. I
> think. If event bubbles the other way (i can't remember right now), you can
> stop the bubbling somewhere else. Either way, make sure the other handler won't
> get called. 

This won't work. I'll try to describe the problem in more detail.

The idea is to add the selected calendar to the composite calendar
in all cases, except if the user has clicked with the mouse on the
checkbox. This exception is necessary, because otherwise it's not
possible for the user to add or remove calendars to or from the
composite calendar.

A list item can be selected by the user in two different ways:

a) clicking on the list item
   -------------------------
   In this case first a select event is fired 
   (onCalendarListSelect() handler), after that a click event
   is fired (onCalendarCheckboxClick() handler).
   Only in the click handler it is known, if the user clicked
   on a checkbox.

b) selecting the list item by using the keyboard
   ---------------------------------------------
   In this case only a select event is fired.

I tried to handle the above described exception in that way,
that the select handler sets up a timer, which schedules adding
the selected calendar to the composite calendar. If the user
clicked on a checkbox, the timer is cancelled in the click handler.

If I add the calendar to the composite calendar already in 
the select handler, then the click handler removes the calendar
again in the click handler, if the user clicked on a checkbox.
This problem is described in comment #19.

If I handle adding the calendar to the composite calendar
completely in the click handler, I have a problem with b).

An alternative solution I had in mind was to have the same
behaviour as in the Lightning calendar tab, which uses a
tree list box. The list item is only selected, if the
user clicks on the non-checkbox area of the item.
If the user clicks on the checkbox, the checkbox is
toggled, but the item is not selected.
This means, that a select event is only fired, if the
user clicks on the non-checkbox area.
But I didn't find a way to get this running, because this
means changing the default behaviour of a list box.
Of course one can think about using a tree listbox instead
of a listbox ...
Attachment #256150 - Flags: first-review?(mvl) → first-review?(daniel.boelzle)
Comment on attachment 256150 [details] [diff] [review]
add selected calendar to composite calendar patch v3

Using a timer is actually a hack, of course. IMO the code needs verbose documentation about the hack, marked XXX, because there is room for improvement.

r=dbo
Attachment #256150 - Flags: first-review?(daniel.boelzle) → first-review+
Whiteboard: [checkin needed after 0.5]
patch checked in on trunk and MOZILLA_1_8_BRANCH
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed after 0.5]
Target Milestone: --- → 0.7
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: