714431 - New Event... and New Task... are always disabled

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Wolfgang Germund]

User Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:10.0) Gecko/20111223 Firefox/10.0 SeaMonkey/2.7
Build ID: 20111223104554

Steps to reproduce:

I use the Beta Channel from SeaMonkey and Lightning. 

https://ftp.mozilla.org/pub/mozilla.org/calendar/lightning/tinderbox-builds/comm-beta/linux64-xpi-linux64/1325196130/lightning.xpi


Actual results:

Since the changeset from bug 586276 the menu items 
New Event... and New Task... are always disabled.




Expected results:

diff --git a/calendar/base/content/calendar-common-sets.js b/calendar/base/content/calendar-common-sets.js
--- a/calendar/base/content/calendar-common-sets.js
+++ b/calendar/base/content/calendar-common-sets.js
@@ -30,16 +30,19 @@
  * use your version of this file under the terms of the MPL, indicate your
  * decision by deleting the provisions above and replace them with the notice
  * and other provisions required by the GPL or the LGPL. If you do not delete
  * the provisions above, a recipient may use your version of this file under
  * the terms of any one of the MPL, the GPL or the LGPL.
  *
  * ***** END LICENSE BLOCK ***** */
 
+var CalendarDeleteCommandEnabled = false;
+var CalendarNewItemsCommandEnabled = false;
+
@@ -126,40 +129,42 @@ var calendarController = {
         }
         return false;
     },
 
     isCommandEnabled: function cC_isCommandEnabled(aCommand) {
         switch (aCommand) {
             case "calendar_new_event_command":
             case "calendar_new_event_context_command":
-                return this.writable && this.calendars_support_events;
+                return CalendarNewItemsCommandEnabled && this.writable && this.calendars_support_events;
             case "calendar_modify_focused_item_command":
                 return this.item_selected;


The bool CalendarNewItemsCommandEnabled is init with false and only set true in
function calendarUpdateNewItemsCommand().

The problem is, that the function is never called and so
the cammand calendar_new_event_command and calendar_new_todo_command
are always disabled.

Comment 1

[Philipp Kewisch [:Fallen][📱📆]]

Wolfgang (Sourdeau), could you fix this issue? I'd appreciate a patch!

Comment 2

[Stefan Sitter [:ssitter]]

Maybe we should just back-out the untested patch from Bug 586276 from comm-beta to avoid more regressions and to get a beta build released.

Comment 3

[Wolfgang Sourdeau]

I am going to provide a patch. Contrary to what you mention, this code *has* been tested. But it might fail here because I modified this part of the code at the last minute.

Comment 4

[Wolfgang Sourdeau]

Philipp, the bug was actually introduced by you. For some reason, the 2 changes made to calendar-list-tree.xml (present in my last patch) where "calendarUpdateNewItems" would be invoked didn't make it in any form into the patch you applied.

From you comments, I know you thought of using an alternative method, but from what I understand, nothing final was decided so the call would still be invoked from the binding.

Additionally, I have found a bug (my fault this time) in "calendarUpdateDeleteCommand", where the loop should be :

    /* we must disable "delete" when at least one item cannot be deleted */
    for each (let item in selectedItems) {
        if (!userCanDeleteItemsFromCalendar(item.calendar)) {
            CalendarDeleteCommandEnabled = false;
            break;
        }
    }

instead of "for each (let calendar in...)".

Can I leave that to you?

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v1

I was thinking the two changes there are not needed, due to what I did in the composite observer, but it seems this observer is never added. This patch fixes what you mentioned and adding the observer. It also updates the commands onLoad, which you previously did in the task list observer. I'm not sure we really need the onLoad updates - do we?

I can't reproduce this bug on mac at all, so I'm not quite sure if this patch works. I'd appreciate a quick review so we can get this fixed. Please test!

Comment 6

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v2

As discussed on irc

Comment 7

[Wolfgang Sourdeau]

Comment on attachment 586065 [details] [diff] [review]
Fix - v2

This fix works fine for me. Both "New XX" and "Delete" entries are working fine now.

Comment 8

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central changeset a038320a5ee6

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

Backported to releases/comm-aurora changeset aa55a1688b6f

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

Backported to releases/comm-beta changeset 8e3817064bcb

Comment 11

[Wolfgang Germund]

I have 2 profiles.

In the profile with CalDav calendar server the fix works fine.

In a profile with only the Home default calendar the fix don't work.
I have tested with venkman, that the function calendarUpdateNewItemsCommand() 
is not called in this case.

Comment 12

[Wolfgang Sourdeau]

It makes sense: the "onLoad" notification only occurs during a refresh. With caldav calendars, a refresh is forced during the first "getItems". With local calendars (calStorageCalendar), no such thing occurs since they cannot be refreshed. When the list of calendars is instantiated, the list of calendars should be queried for instances that do support "refresh". If none is found, calendarUpdateNewItemsCommand should be invoked.

Comment 13

[Wolfgang Sourdeau]

Attachment: Patch implementing the fix described in comment #12

Comment 14

[Wolfgang Sourdeau]

Attachment: Same as above but in the XUL code rather than calendar-list-tree

Comment 15

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central changeset 36e27e06b67c

Comment 16

[Philipp Kewisch [:Fallen][📱📆]]

Backported to releases/comm-aurora changeset b0157434c3f2

Comment 17

[Philipp Kewisch [:Fallen][📱📆]]

Backported to releases/comm-beta changeset f68cd367d6ed

Comment 18

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 587391 [details] [diff] [review]
Same as above but in the XUL code rather than calendar-list-tree

I'm going go go with Array.some instead to save some lines. See pushed patch!

Comment 19

[Matthew Mecca [:mmecca]]

The problem still exists for profiles with a single ics calendar without cache enabled. With multiple ics calendars, all without cache enabled, the commands are disabled at startup until the selected calendar is changed.

Comment 20

[Wolfgang Sourdeau]

(In reply to Matthew Mecca [:mmecca] from comment #19)
> The problem still exists for profiles with a single ics calendar without
> cache enabled. With multiple ics calendars, all without cache enabled, the
> commands are disabled at startup until the selected calendar is changed.

I have tried that you describe above and I could not reproduce that bug with Lightning 1.2b2. I even tried both cached and non-cached mode with the same result. Which build are you using?

Comment 21

[Wolfgang Sourdeau]

Additionally to #20 above, there might also be an exception occurring somewhere that prevent the "onLoad"event from occurring on your installation, did you check the JS console?

Comment 22

[Stefan Sitter [:ssitter]]

I can confirm the problem still exists. Using Lightning 1.2 (BuildID: 20120116144246) with Mozilla/5.0 (Windows NT 6.1; rv:10.0) Gecko/20120111 Thunderbird/10.0. I have subscribed six local ics calendar files and one calendar that is based on address book. Cache is disabled for all calendars. No errors in console.

Comment 23

[Wolfgang Sourdeau]

(In reply to Stefan Sitter from comment #22)
> I can confirm the problem still exists. Using Lightning 1.2 (BuildID:
> 20120116144246) with Mozilla/5.0 (Windows NT 6.1; rv:10.0) Gecko/20120111
> Thunderbird/10.0. I have subscribed six local ics calendar files and one
> calendar that is based on address book. Cache is disabled for all calendars.
> No errors in console.

What do you mean by "one calendar that is based on address book" ?
Thanks

Comment 24

[Stefan Sitter [:ssitter]]

The ThunderBirthDay extension <https://addons.mozilla.org/thunderbird/addon/5337> adds a calendar provider that gets its content from the birthday field of the contacts information in the Thunderbird address book.

But it doesn't matter. I can reproduce too in a fresh profile:
remote ics calendar on http server: works
local ics calendar on local disc: fails

Comment 25

[Wolfgang Sourdeau]

I can reproduce this issue but in a very specific case: by starting up thunderbird AND when all calendars are disabled (unchecked) at that moment. Then the selection is grayed out and then as soon as I select another calendar, disabled or not, the menus become available. Is that what happens with you?

Comment 26

[Wolfgang Sourdeau]

Attachment: additional fix for problem mentionned in comment #19

We now trigger "calendarUpdateNewItemsCommand" from commonInitCalendar when no calendar is active, in addition to the first condition.

This patch must be applied after all the others above.

Comment 27

[Wolfgang Sourdeau]

(In reply to Stefan Sitter from comment #22)
> I can confirm the problem still exists. Using Lightning 1.2 (BuildID:
> 20120116144246) with Mozilla/5.0 (Windows NT 6.1; rv:10.0) Gecko/20120111
> Thunderbird/10.0. I have subscribed six local ics calendar files and one
> calendar that is based on address book. Cache is disabled for all calendars.
> No errors in console.

Can you retry with the last patch applied: https://bugzilla.mozilla.org/attachment.cgi?id=589253 ?

Comment 28

[Stefan Sitter [:ssitter]]

You can use the following steps to reproduce:
1. Create a new Thunderbird profile and install Lightning
2. Subscribe to ics file that is stored on local disc using menu File > Open > Calendar
3. Remove the default Home calendar
4. Restart Thunderbird

Result: calendar is enabled, selected, and not read-only but the New Event/Task commends are disabled.

Comment 29

[Wolfgang Sourdeau]

Ok, that's a very specific use case. I would even go as far as pretend that it reveals a bug in the rest of the lightning code rather than in the ACL patch. Here is what happens:

Some code in calendar-task-tree invokes getCompositeCalendar, which ends up in calCalendarManager::assureCache, which finally invokes "refresh" in the ICS calendar. All of this occurs before the helper listener is setup in the composite calendar. Whereas my code is invoked during an "onLoad" event produced by the composite calendar, it seems the refresh for local files is completed before the underlying mechanisms is properly setup.

I am still working on this and will likely provide a patch soon, I would suggest reopening a new bug and closing this one as to not mark the ACL patchs as blocked.

Comment 30

[Wolfgang Sourdeau]

Attachment: additional fix/work-around for problems mentionned in comments #19 -> #26

This patch is a work-around for a situation that becomes unmanageable due to the specifics of each calendar provider. I guess a new bugreport could be filled regarding a standard method providing the ability to reliably control the "readiness" of each calendar...

Comment 31

[Wolfgang Sourdeau]

I have noticed a very strange behaviour with Thunderbird 12.0a1: the "Delete" action is never enabled. I have investigated and  have noticed that the calendar-month-base-view::setSelectedItems is never invoked with aSuppressEvent equal to false.

This does not occur with Thunderbird 10 but might be good to keep in mind.

Comment 32

[Wolfgang Sourdeau]

(In reply to Wolfgang Sourdeau from comment #31)
> I have noticed a very strange behaviour with Thunderbird 12.0a1: the
> "Delete" action is never enabled. [snip]

Nor is the "Open" action actually, where my patch has no impact at all.

Comment 33

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 589277 [details] [diff] [review]
additional fix/work-around for problems mentionned in comments #19 -> #26

Aside from the commented out code, looks good. r=philipp

Comment 34

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central changeset b43d9f97aad8

Comment 35

[Philipp Kewisch [:Fallen][📱📆]]

Backported to releases/comm-aurora changeset df6e759ba85c

Comment 36

[Philipp Kewisch [:Fallen][📱📆]]

Backported to releases/comm-beta changeset 737178f3f23a