168495 - After adding a new event, events for disabled calendars show up

Status: RESOLVED WORKSFORME

(Calendar :: Internal Components, defect)

Description

[Mike Potter]

After adding an event to a calendar which is not checked (so its events are not
showing up), the event shows up in the month, week and day view.

To reproduce:
1. Uncheck the default calendar so its events are not showing up.
2. Add a new event to the default calendar.
3. Notice it shows up in the month, week or day view (along with the other
events from that file).

Notice as well, when you click forward and back, that the events don't show up,
so its only immediately after adding the events that they are showing up when
they shouldn't.

If this is a front end issue, feel free to send it back to me.

Comment 1

[Mike Potter]

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

Comment 2

[Barry Gerritsen]

When trying to edit or delete suchs an event Mozilla crashes most of the times.
I'm using w2k with
Mozilla/5.0 (Windows; U; Windows NT 5.0; nl-NL; rv:1.5) Gecko/20031007

This isn't always the case, sometimes the event can be edited or deleted.

Comment 3

[Barry Gerritsen]

I have traced the problem of the crash when trying to edit an event to
eventDialog.js function loadCalendarEventDialog
the following piece of code:

     if( gEvent.parent )
         setFieldValue( "server-field", gEvent.parent.server );
     else
     {
         document.getElementById( "server-field" ).selectedIndex = 1;
     }

craches on the setFieldValue.
I've tried to use dump for the value of gEvent.parent and gEvent.parent.server
but I get nothing or blanks in my console window. 
I don't know if it is the "if" statement that should return false or the
gEvent.parent.server containing an illegal value, but removing the if statement
while keeping "document.getElementById" doesn't crash.

If this comment shouldn't be in this bug or if I should make this comment in
another way, please let me know!!! I'm new to this.

Comment 4

[Barry Gerritsen]

I'm wondering if this really is a problem:

1 If you're adding an event to a caldendar which isn't checked, perhaps you want
to see all the events of that calendar, for instance as a visual check. 
2 Another approach could be not to allow adding an event to a calendar which
isn't checked.

When #1 is the case the problem I had with editing/deleting an event should be
fixed.
When #2 is the case the filling of the pulldown-menu in the new/edit event
window has to be limited to the checked calendars. (If this is easy to do but
not the desired effect perhaps it is useful as a temporary fix).

Comment 5

[Caliga (Daniel St.)]

this IS a problem because the drawn events are "phantoms" and accessing them
leads to a crash.
See also dupe bug 237311

Comment 6

[Rodger]

I'm not sure that I can duplicate this exact problem, but using the Calendar for
windows and Mozilla 7B nightly builds at version 200405108 adding an event and
filing it, the event shows up on every calendar month even though the event was
for a specific day and time without repeating.  Also the display for all events
seem to duplicate although leaving the calendar and coming back to it seems to
reset the display.

Comment 7

[Martin Bagge / brother]

Confirming this for TB 0.8 and extension 2004100812

When adding a event to a not selected calendar the events for that calendar is
showing up but the calendar stays unselected in the cal.picker.
changin views redraws the output to correct state (not showing any events from
the unselected calendar)

Comment 8

[Richard Klein]

Attachment: Limit available calendars

This patch only allows active calendars to be shown in the event or todo
dialog.

Comment 9

[Richard Klein]

Comment on attachment 164751 [details] [diff] [review]
Limit available calendars

Requesting review

Comment 10

[Mostafa Hosseini]

Comment on attachment 164751 [details] [diff] [review]
Limit available calendars

gekacheka: Could you please review this patch?

Comment 11

[gekacheka]

Comment on attachment 164751 [details] [diff] [review]
Limit available calendars

Thanks for the work on this patch.  Sorry it has taken me a while to
comment on it.

1. what do I need to do to apply this patch?  I get:
] cd mozilla/calendar/resources/content
] patch --dry-run -i c:/temp/calendar/168495.patch 
patching file eventDialog.js
patch: **** malformed patch at line 61: Index:
resources/content/eventDialog.xul

2. As I understand the approach is to remove the disabled calendars from the
choices in the dialog.	An alternative is to enable the calendar chosen in the
dialog.  Or don't show the added/modified events at all.   Which alternative is
best?

Approach1: Reduce calendar choices to make it impossible to store a
new/modified event or task into a disabled calendar from the
event/task dialog:
 * show only enabled calendars (checkbox on) in calendar drop down of
   event/task dialogs
 * for new events or tasks, preselect (in dropdown of dialog) the
   selected (outline) calendar only if that calendar is enabled
   (checkbox on)
 * for edit existing events or tasks, preselect in dropdown of dialog)
   the previous calendar of event/task only if that calendar is
   enabled (checkbox on),
 * otherwise preselect the first enabled calendar (checkbox on).

Approach2: Enable the modified calendar (turn checkbox on)
permanently rather than phantomly.
 * show all calendars in calendar drop down of event/task dialogs (as
   now)
 * preselect the selected/previous calendar (as now)
 * when dialog closes, enable the modified calendar (if was disabled).

Approach3: Don't show added/modified event or task if its calendar is 
disabled (checkbox off).
 * as now, except don't show "phantom" events.

Issues:
 * what happens if there are no enabled calendars? (no checkboxes on)
   Approach1 seems to fail in this case.  Approach2 only fails if
   there are no calendars.
 * Approach1 might change the calendar of an existing event if there is
   some way to invoke the dialog on an event that is in a disabled
   calendar.
   - currently changing calendars is disallowed, probably for a reason.
   - Could this happen if the UI was running very slowly?
     (say by double clicking an event, then turning off its calendar
     before the dialog appears).
   - in any case Approach1 needs to guard against this:
     if mode == "edit" and calendar not found, then alert.
 * event and task dialogs are also invoked by import and paste/drop.  
   User might want to drop/paste something on a disabled calendar (not
   implemented yet). Approach1 would change destination to the first
   enabled (checkbox on) calendar, which seems surprising, such as when
   user pastes/drops on the last calendar (which happens to be disabled) and
   it is added to the first (which happens to be enabled).
 * With Approach1, selecting (outline) a disabled calendar does not
   have the desired effect of designating the destination for new
   events or import.  To avoid this confusion, selection of a disabled
   calendar should automatically enable the calendar.

It is not clear whether/why Approach1 is the best approach.
If we later decide approach1 is best approach after all,
this patch introduces problems.
 * no guard for no enabled calendars.
 * no guard for edited event/task's previous calendar not found
 * surprising that selected calendar is not used for new/import if disabled,
   can avoid this by enabling a disabled calendar when it is selected,
   and moving selection when selected calendar is disabled.

Comment 12

[Richard Klein]

Comment on attachment 164751 [details] [diff] [review]
Limit available calendars

I'm going to obsolete this patch.  I think that the code refactoring going on
will provide a better long term solution to this.

Comment 13

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

afaik, the refactoring will mostly touch backend code, so the userinterface
problems will stay.

Comment 14

[Mostafa Hosseini]

That was a tough reviewer. Thanks for the review, gekacheka.
If both approaches have close pros and cons, the one available (approach#1) wins.
Besides if approach2 is implemented, this bug will still be valid because they
might not see that the calendar has been enabled. I don't feel good about
modifying settings (e.g. enabling disabling) which are not obvious at the first
look but this still can be discussed.
As for the short comings, which are less likely to happen, other bugs should be
logged.
I appreciate your in-depth review gekacheka, but at this point we are ok with
sanity-checking with regard to reviews. With that in mind and if you are not
strongly opposed to approach#1, would a re-review be possible?

Comment 15

[gekacheka]

(In reply to comment #13)
> afaik, the refactoring will mostly touch backend code, so the userinterface
> problems will stay.

Note that this bug was filed against libxpical (backend) by Mike Potter who was
once the front-end tsar until he left oeone in 2003.   I have found no code in
the front end that specifically causes them to appear until refreshed again
(phantomly).  So the rebuilding the back end could in fact fix this problem
(approach 3).

Comment 16

[gekacheka]

(In reply to comment #14)
> I don't feel good about
> modifying settings (e.g. enabling disabling) which are not obvious at the first
> look

Changing an event's calendar, such as when importing, is surprising to me, i
don't look at that control unless I am actively changing it so I would not
notice the change.

I agree it is not good to change things automatically for mysterious reasons.

To me, it seems reasonable to add an event without making its calendar visible,
such as if a user is at office and wants to quickly add something to a home
calendar.  Approach3 seems least surprising from a user's standpoint if we could
figure out how to do it.  

From my viewpoint, both approach1 and approach2 are workarounds until we figure
out how to implement approach3.  Approach2 seems simpler and easier to undo once
the backend is fixed.

Comment 17

[Mostafa Hosseini]

With approach3, it may seem to the user that the event wasn't added if they
don't know that calendar is disabled.
Handling disabled calendars adds other complications too. For example, when
adding an event to a disabled remote calendar, should auto publish, update the
remote file?
I think following a policy of not providing any modification means for disabled
calendars, will clarify and simplify things. They can remain as just a simple
entry in the calendars list for further enabling.

Comment 18

[gekacheka]

I was using 'disabled' to mean 'hidden'.

A 'readonly' toggle would be independent of whether it is hidden.  So the user
can prevent accidentally modifying a calendar, but still view it.  See for
example bug 243919.

Comment 19

[gekacheka]

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

Comment 20

[Joey Minta]

This bug worksforme on trunk.  Anyone else still seeing this?

Comment 21

[Marcia Knous [:marcia]]

Mass move of libxpical bugs to the Internal Components, per ctalbert.

Comment 22

[Reed Loden [:reed]]

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