Closed Bug 714431 Opened 8 years ago Closed 8 years ago

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

Categories

(Calendar :: General, defect, blocker)

Lightning 1.2
defect
Not set
blocker

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wolfgang, Assigned: WSourdeau)

References

Details

(Keywords: regression)

Attachments

(3 files, 3 obsolete files)

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.
Severity: normal → blocker
Wolfgang (Sourdeau), could you fix this issue? I'd appreciate a patch!
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Keywords: regression
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.
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?
Attached patch Fix - v1 (obsolete) β€” β€” Splinter Review
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!
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #586034 - Flags: review?(WSourdeau)
Attached patch Fix - v2 β€” β€” Splinter Review
As discussed on irc
Attachment #586034 - Attachment is obsolete: true
Attachment #586034 - Flags: review?(WSourdeau)
Attachment #586065 - Flags: review?(WSourdeau)
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.
Attachment #586065 - Flags: review?(WSourdeau) → review+
Pushed to comm-central changeset a038320a5ee6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4
Backported to releases/comm-aurora changeset aa55a1688b6f
Target Milestone: 1.4 → 1.3
Backported to releases/comm-beta changeset 8e3817064bcb
Target Milestone: 1.3 → 1.2
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Assignee: philipp → WSourdeau
Attachment #587307 - Flags: review?(philipp)
Blocks: 586276
Attachment #587307 - Attachment is obsolete: true
Attachment #587307 - Flags: review?(philipp)
Attachment #587391 - Flags: review?(philipp)
Pushed to comm-central changeset 36e27e06b67c
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: 1.2 → 1.4
Backported to releases/comm-aurora changeset b0157434c3f2
Target Milestone: 1.4 → 1.3
Backported to releases/comm-beta changeset f68cd367d6ed
Target Milestone: 1.3 → 1.2
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!
Attachment #587391 - Flags: review?(philipp) → review+
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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?
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?
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.
(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
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
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?
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.
Attachment #589253 - Flags: review?(philipp)
(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 ?
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.
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.
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...
Attachment #589253 - Attachment is obsolete: true
Attachment #589253 - Flags: review?(philipp)
Attachment #589277 - Flags: review?(philipp)
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.
(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 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
Attachment #589277 - Flags: review?(philipp) → review+
Pushed to comm-central changeset b43d9f97aad8
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: 1.2 → 1.4
Backported to releases/comm-aurora changeset df6e759ba85c
Target Milestone: 1.4 → 1.3
Backported to releases/comm-beta changeset 737178f3f23a
Target Milestone: 1.3 → 1.2
You need to log in before you can comment on or make changes to this bug.