586276 - Hooks / stubs mechanisms for Mozilla Lightning

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[anirvana]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.8) Gecko/20100723 Ubuntu/10.04 (lucid) Firefox/3.6.8
Build Identifier: 1.02b

Add extension points to Lightning to facilitate extension developers that would like to provide tight integration between Lightning and their groupware of choice.
Extension points could be defined to allow full access control list (ACL) support in Lightning.The proper read, write, modify (and others) checks would be done in Lightning but the results of those would come from an extension providing the right responses based on input criteria. Other stubs could be defined for handling various integration aspects, like Delegation.

Reproducible: Always




Part of GSoC 2010 Project

Comment 1

[anirvana]

Attachment: with ACL hooks

Looking for a quick feedback on this patch, so that I can finish it quickly.

Comment 2

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

Comment on attachment 467141 [details] [diff] [review]
with ACL hooks

>diff -r c1186371ddee calendar/base/content/calendar-common-sets.js
> 
>+// For ACL hooks
>+var SCEnableDelete = false;
>+var SCEnableNewItems = true;
>+
>+
>+/* ACL hooks */
>+function SCComputeEnableNewItems() {
>+    let oldValue = SCEnableNewItems;
>+    SCEnableNewItems = false;
>+    
>+    var listener = {
>+        onResult: function ACL_onGetResult(calendar, entry){
>+            SCEnableNewItems = entry.userCanAddItems;
>+        }  
>+    };
>+
>+    try{
I'll spare the style nit comments this time, please fix.


>+        let aclMgr = Components.classes["@mozilla.org/calendar/aclprovider;1"]
>+                               .getService(Components.interfaces.nsISupports)
>+                               .wrappedJSObject;
This needs an interface, you must not use wrappedJSObject.


>+         if (calendar.type == "caldav" && !cal.readOnly) {
>+             aclMgr.getCalendarACLEntry(listener);
>+         }
I don't quite understand. How does it know what calendar?

>+    } catch(e){}
You shouldn't swallow exceptions, make sure its not expected that one shows up.

>+
>+    if (SCEnableNewItems != oldValue) {
>+        let commands = ["calendar_new_event_command",
>+                        "calendar_new_event_context_command",
>+                        "calendar_new_todo_command",
>+                        "calendar_new_todo_context_command",
>+                        "calendar_new_todo_todaypane_command"];
Instead, I'd suggest to just update all commands. We don't have command categories yet, adding a certain subset here makes the code less maintainable.

>+
>+function SCComputeEnableDelete(selectedItems) {
Similar comments as above for this function.

>+        for (let i = 0; i < selectedItems.length; i++) {
>+        let calendar = selectedItems[i].calendar;
>+            if (calendar.type == "caldav") {
>+                aclMgr.setCalendar(calendar);
>+                aclMgr.getCalendarACLEntry(listener);
This doesn't look right. You set the calendar on a service. Does this mean the service can only process one calendar at a time? I think it would be better to give the latter function a calendar argument.

> 
>+/* hackich: this is the old implementation of isCalendarWritable, without the
>+   ACL code */
>+function isCalendarAvailable(aCalendar) {
>+    return (!aCalendar.getProperty("disabled") &&
>+            !aCalendar.readOnly &&
>+            (!getIOService().offline ||
>+             aCalendar.getProperty("requiresNetwork") === false));
>+}
>+
>+    var url = "chrome://calendar/content/calendar-event-dialog.xul";
>+    //ACL hooks
>+    var operationListener = {
>+        onResult: function ACL_onGetResult(cal, itemACLEntry){
>+            if (isCalendarAvailable(calendar)
>+                && (mode == "new"
>+                || (mode == "modify"
>+                && !isInvitation
>+                && itemACLEntry.userCanModify))){
>+                url = "chrome://calendar/content/calendar-event-dialog.xul";
>+            } else {
>+                url = "chrome://calendar/content/calendar-summary-dialog.xul";	
>+            }
>+        }
>+    };
So we do a request each time the event dialog is opened?
>+  //ACL hooks
>+        let aclIncluded = true;
>+        var listener = {
>+            onResult: function ACL_onGetResult(calendar, entry){
>+                let isNew = !(aItem.id);
>+                aclIncluded = !isNew || entry.userCanAddItems;
>+            }
>+        };
>+
>+        try{
>+            let aclMgr = Components.classes["@mozilla.org/calendar/aclprovider;1"]
>+                                   .getService(Components.interfaces.nsISupports)
>+                                   .wrappedJSObject;
>+
>+            if (calendar.type == "caldav") {
>+                aclMgr.setCalendar(calendar);
>+                aclMgr.getCalendarACLEntry(listener);
>+            }
>+        } catch(e){}
You're using a callback here, but assume that the callback is synchronous. Can't you just return from the acl manager functions?

> 
>+        /* ACL code */
>+        SCComputeEnableDelete(aItemArray);
>+        /* /ACL code */
Please be a little bit more elaborate on this type of comment. Don't just write ACL code, but explain (shortly!) why it needs to be called here. You also don't need an ending comment.

>+            if (calendar.type == "caldav" && aclMgr) {
>+                var realCalendar = calendar.getProperty("cache.uncachedCalendar");
>+                if (!realCalendar) {
>+                    realCalendar = calendar;
>+                }
>+                realCalendar = realCalendar.wrappedJSObject;
>+                var cache = realCalendar.mItemInfoCache;
>+                if (cache[item.id]) {
>+                    aclMgr.setCalendar(calendar);
>+                    aclMgr.getItemACLEntry(listener);
>+                }
>+            }
>+        }
What happens to non-caldav calendars? They should be able to take advantage of the ACL goodness if they support it.
Also, you shouldn't be using wrappedJSObject and not rely on the internal structure of caldav.

>diff -r c1186371ddee calendar/base/content/widgets/calendar-list-tree.xml
>+       /* ACL hooks */
>+        SCComputeEnableNewItems();
>+       /* /ACL hooks */
This doesn't look like the right place to call this function.
lendar-js

>diff -r c1186371ddee calendar/providers/caldav/calDavACLProvider.js

>+ * The Original Code is Oracle Corporation code.
This is Oracle Code? How so?

>+QueryInterface: function(aIID) {
>+        if (!aIID.equals(Components.interfaces.nsISupports))
>+            throw Components.results.NS_ERROR_NO_INTERFACE;
>+
>+        return this;
>+    },
Please use cal.doQueryInterface()

>+    get calendar() {
>+        return this.mCalendar;
>+    },
>+
>+    setCalendar: function ACL_setCalendar(aCalendar){
>+        this.mCalendar = aCalendar;
>+    },
Hmm you have a getter and a function to set. Aside from the fact that I don't see why the service should keep a reference to the calendar, you should be using getter/setter here.

>+    getCalendarACLEntry: function ACL_getCalendarACLEntry(opListener){
>+        let calACLEntry = new calACLCalendarEntry();
>+        opListener.onResult(this.mCalendar, calACLEntry);
>+
>+    },
These functions work synchronously. Is there any reason you chose to use callbacks, instead of returning the value?

>+// nsIFactory
>+const calDavACLProviderFactory = {
>+    QueryInterface: function (aIID) {
>+        if (!aIID.equals(Components.interfaces.nsISupports) &&

This changed a lot on trunk. Since we do checkins on trunk first, this will have to be changed before checkin.

>+	let refreshDelay = getPrefSafe("calendar.caldav.refresh.initialdelay", 0);
>+    if (refreshDelay > 0) {
>+        let now = (new Date()).getTime();
>+        this.mFirstRefreshDelay = now + refreshDelay * 1000;
>+        LOG("[caldav] first refresh should not occur before: "
>+            + this.mFirstRefreshDelay);
>+    }
>+    else {
>+        this.mFirstRefreshDelay = 0;
>+    }

This looks like part of a different patch?


Also, it seems you hold a reference to the acl manager in calDavCalendar.js, but don't really use it. I assume this is work in progress.

>+interface calIACLProvider : nsISupports
>+{
>+  readonly attribute calICalendar aCalendar;
>+  void setCalendar(in calICalendar aCalendar);
>+  void getCalendarACLEntry(in calIGenericOperationListener aOperationListener);
>+  void getItemACLEntry(in calIItemBase aItem, in calIGenericOperationListener aOperationListener);
>+};                             
Please document this file more.

Comment 3

[Ludovic Marcotte]

Any news on an updated patch?

Comment 4

[Mohit Kanwal [:redDragon]]

Attachment: Updated patch for Hooks

Hi all,

Attached is the updated patch for the ACL Hooks. It includes the previous attachment#467141 [details] [diff] [review] code and also code from Inverse Version of Lightning. If required I will follow this up with a clean patch with all the styling addressed. For now, looking if this patch is what's required for this bug.

Cheers
Mohit

Comment 5

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

Comment on attachment 555157 [details] [diff] [review]
Updated patch for Hooks

Review of attachment 555157 [details] [diff] [review]:
-----------------------------------------------------------------

The only real issue that bothers me is that this patch inserts a lot of code specific to caldav in places that should be independent of the provider implementation. We should discuss how to best resolve that, setting r- in the meanwhile.

The following issues are rather minor:

::: calendar/base/content/calendar-common-sets.js
@@ +913,5 @@
> +                    "calendar_new_todo_context_command",
> +                    "calendar_new_todo_todaypane_command"];
> +
> +    SCEnableNewItems = false;
> +    let cal = getSelectedCalendar();

Using "cal" as a variable name isn't a good idea since it shadows the "cal." namespace imported by calUtils.jsm.

::: calendar/providers/caldav/calDavACLManager.js
@@ +57,5 @@
> +}
> +
> +/* CalDAVACLOfflineManager */
> +function CalDAVACLOfflineManager() {
> +    this.initDB();

Initializing in the constructor might shoot us in the foot later on, if possible this should be done the first time the service is used.

::: calendar/providers/caldav/calDavCalendar.js
@@ +447,5 @@
> +                }
> +                break;
> +
> +            case "imip.identity":
> +                this.mACLEntry.getOwnerIdentities({}, ownerIdentities);

Does this work out even if there is no matching Thunderbird identity?

@@ +454,5 @@
> +                    return ownerIdentities[0];
> +                }
> +                break;
> +	    case "capabilities.alarms.actionValues":
> +	        return ["DISPLAY", "EMAIL"];

Fixed alarm values if acl is available?

Comment 6

[Ludovic Marcotte]

Also reviewing a bit this patch.

We have in multiple places:

+let SCEnableDelete = false;
+let SCEnableNewItems = true;
...
+function SCComputeEnableNewItems() {

You should remove the "SC" prefix. That is for "SOGo Connector" since that code was initially put in there. You should either use a standard Ligtning prefix or nothing at all.

As for the "CalDAV" approach, I think it makes senses for now. Note that VERY little CalDAV servers actually implement ACL handling properly. So when you add on top of that other potential providers, like ICS over a simple FTP/HTTP link, gdata and potentially others, it makes little sense for now to overscope things and try to do it all in one shot. I think a small, granular approach is needed and we should start with what works right now. The hooks could even be marked as "first try - might change soon" but at least, it would allow us, the SOGo project, to move more code in our extensions rather than modifying Lightning over and over for each releases.

Comment 7

[Mohit Kanwal [:redDragon]]

Attachment: Fix-2

Updated patch with the style nits fixed.

Fixed the identity part, it can now work even if the application does not have any default identities.
For the fixed alarm values, I have commented them out, Ludovic do you think its okay to do it?

Comment 8

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

Comment on attachment 568237 [details] [diff] [review]
Fix-2

Review of attachment 568237 [details] [diff] [review]:
-----------------------------------------------------------------

I can go with specialcasing for now, but we should definitely file a bug to generalize this. One thing we could do to not scatter the ACL code in all the other files is to add overlay files living in the caldav/ directory, that override the needed functions. To overwrite a function do something like this:


var caldavSaveSomeLightningFunc = someLightningFunc;
someLightningFunc = function caldav_someLightningFunc(arg1, ...) {
 // the if you need to, call:
  caldavSaveSomeLightningFunc(arg1, ...);
};


Looking at the code this should work out in most places, let me know if you need help there.

r- for now, we should chat about this some time.

::: calendar/base/content/calendar-common-sets.js
@@ +41,5 @@
>  /**
>   * Command controller to execute calendar specific commands
>   * @see nsICommandController
>   */
> +let calendarController = {

I don't know what the official take on this is, but I think variables on top scope can/should still be var. Sorry for the confusion :)

@@ +950,5 @@
> +                        else {
> +                            gEnableNewItems = false;
> +                        }
> +                        if (gEnableNewItems != oldValue) {
> +                            for (let i = 0; i < commands.length; i++) {

for each (let cmd in commands)

::: calendar/base/content/calendar-ui-utils.js
@@ +307,4 @@
>  */
>  function appendCalendarItems(aItem, aCalendarMenuParent, aCalendarToUse, aOnCommand) {
>      let calendarToUse = aCalendarToUse || aItem.calendar;
> +    let calendars = sortCalendarArray(getCalendarManager().getCalendars({}));

Why drop cal. ?

::: calendar/providers/caldav/calDavACLManager.js
@@ +1441,5 @@
> +function NSGetFactory(cid) {
> +    if (!this.scriptsLoaded) {
> +        Services.io.getProtocolHandler("resource")
> +                .QueryInterface(Components.interfaces.nsIResProtocolHandler)
> +                .setSubstitution("calendar", Services.io.newFileURI(__LOCATION__.parent.parent));

If you can make this file call all calUtils functions prefixed with "cal.", then you don't have to do this stuff. Just Cu.import calUtils.jsm at the top and then

var NSGetFactory = XPCOMUtils.generateNSGetFactory([CalDAVACLManager]);

::: calendar/providers/caldav/public/calICalendarACLManager.idl
@@ +44,5 @@
> +interface calICalendar;
> +
> +
> +[scriptable, uuid(a5e0ce7c-4757-4aa8-967e-b62d92bb6391)]
> +interface calICalendarACLManager : nsISupports

Some comments on these interfaces would be nice.

Comment 9

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

As discussed with Ludovic, we're not taking this for 1.0. Thunderbird ESR 1.0 will be released with Thunderbird 10, so thats the latest date this needs to be in.

Comment 10

[Stefan Sitter [:ssitter]]

(In reply to Philipp Kewisch [:Fallen] from comment #9)

Thunderbird 10 (and Lightning 1.2) will move to BETA on December 20, 2011. If you want to ship this feature with Thunderbird 10 this should be the latest date to land a patch.

Comment 11

[Mohit Kanwal [:redDragon]]

Attachment: Fix-3

Version 3 of the fix with the above nits applied, except the overlaying of the ACL functions in a separate file. I think we can open up a separate bug for it. I am sorry for the patch taking a long time, I am done with exams and don't have anything major on my plate right now =)

Comment 12

[Wolfgang Sourdeau]

(In reply to Mohit Kanwal [:redDragon] from comment #11)
> Created attachment 578894 [details] [diff] [review]
> Fix-3

Mohit, you took most of the idls and code from Inverse's Lightning, which I wrote, and you put the name of Anirvana Mishra as primary author!

The proper head would be:

 * The Original Code is Inverse Inc. code.                                                                                                   
 *                                                                                                                                           
 * The Initial Developers of the Original Code are                                                                                           
 *   Wolfgang Sourdeau <[e-mail]>                                                                                                
 * Portions created by the Initial Developer are Copyright (C) 2011                                                                          
 * the Initial Developer. All Rights Reserved.       


and


 * Contributor(s):                                                                                                                           
 *                  Mohit Singh Kanwal <[e-mail]>

Anirvana has nothing to do with the design of those. I sent him a first versoin of the IDL, which does not even match this one. And the CalDAVAclManager class has been present in SOGo Connector for at least 3 years now...

Note that it does not matter as I am going to review that patch and possibly modify it... But please give credit where it's due.

Comment 13

[Mohit Kanwal [:redDragon]]

Ahh... Damn it.. I took the original file and added the code from inverse's repo, should have been more careful.. Do you want me to upload a new patch with the authorship fixed?

Comment 14

[Wolfgang Sourdeau]

(In reply to Mohit Kanwal [:redDragon] from comment #13)
> Ahh... Damn it.. I took the original file and added the code from inverse's
> repo, should have been more careful.. Do you want me to upload a new patch
> with the authorship fixed?

No it's ok. I need to rewrite some parts of the patch because what we want is to provide a hook mechanism, not a direct ACL implementation for CalDAV. I'll fix credit attribution in the meantime.

Comment 15

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

Comment on attachment 578894 [details] [diff] [review]
Fix-3

r- given Wolfgang will post a new version soon

Comment 16

[Wolfgang Sourdeau]

Attachment: Implementation of calendar acls through stubs

Cleaner and better implementation:
- applies on 1.0
- implement a default interface so that the current code will work as-is
- implementation class defined through "calendarAclManager" property

No Makefile.in for new acl subdirectory. No adaptation of the calMemoryCalendar. calStorageCalendar required a few "asynchronous" callback mechanisms, might need discussion.

Comment 17

[Wolfgang Sourdeau]

Attachment: Implementation of calendar acls through stubs

Same as above + missing "calICalendarACLManager.idl"

Comment 18

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

Comment on attachment 582814 [details] [diff] [review]
Implementation of calendar acls through stubs

Review of attachment 582814 [details] [diff] [review]:
-----------------------------------------------------------------

Wow, a lot more changes than I expected. We should talk about how to land this best. Merging for Thunderbird 10 happens tomorrow, which would mean we'd need to land this on comm-beta to make it part of Lightning 1.2 (Tb10 compat). I'm a bit reluctant to landing such deep changes on the beta branch.

First pass is code review, I haven't yet tested this.

::: calendar/acl/default/calDefaultACLManager.js
@@ +109,5 @@
> +        return this.mACLManager;
> +    },
> +
> +    get hasAccessControl() {
> +        return false;

We could shorten these a bit, xpcom takes care of controlling access to these attributes:

hasAccessControl: false,
userIsOwner: true,
useCanAddItems: true,
...

@@ +153,5 @@
> +
> +    /* nsISupports */
> +    QueryInterface: function(aIID) {
> +        if (!aIID.equals(Components.interfaces.nsISupports)
> +            && !aIID.equals(Components.interfaces.calICalendarACLCalendarEntry))

Consider using XPCOMUtils.generateQI or cal.doQueryInterface

@@ +194,5 @@
> +        Services.io.getProtocolHandler("resource")
> +                .QueryInterface(Components.interfaces.nsIResProtocolHandler)
> +                .setSubstitution("calendar", Services.io.newFileURI(__LOCATION__.parent.parent));
> +        Components.utils.import("resource://calendar/modules/calUtils.jsm");
> +        cal.loadScripts(scriptLoadOrder, Components.utils.getGlobalForObject(this));

If you make sure all calUtils functions are prefixed with cal. then there is no need for loading calUtils this way.

::: calendar/base/content/calendar-view-core.xml
@@ +373,4 @@
>              return;
>          }
>          let item = this.occurrence;
> +        if (!isCalendarWritable(item.calendar) || (item.aclEntry && (!item.aclEntry.userCanModify || !item.aclEntry.calendarEntry.userIsOwner)) || (item.calendar instanceof Components.interfaces.calISchedulingSupport && item.calendar.isInvitation(item))) {

This seems to be done more than once (i.e. also calendar-multiday-view), can we make it a helper?

::: calendar/base/content/dialogs/calendar-event-dialog.js
@@ -1906,5 @@
>  
> -    if (!canNotifyAttendees(calendar, item) && calendar.getProperty("imip.identity")) {
> -        enableElement("notify-attendees-checkbox");
> -    } else {
> -        disableElement("notify-attendees-checkbox");

What happens to this code? Don't we have to enable/disable that checkbox sometimes?

::: calendar/base/modules/calItipUtils.jsm
@@ +593,5 @@
>                                 ? aItem.calendar.getInvitedAttendee(aItem) : null);
>          if (invitedAttendee) { // actually is an invitation copy, fix attendee list to send REPLY
> +            /* Much like in calendar-event-dialog-attendees.xml (onInitialize)
> +             * we have to check if the attendee is equal to the on of the
> +             * userAddresses. If they aren't equal, it means that

Again, maybe a helper for this?

::: calendar/base/public/calICalendarACLManager.idl
@@ +52,5 @@
> +    void getItemEntries(in unsigned long aCount, [array,size_is(aCount)] in calIItemBase aItems, in calIOperationListener aListener);
> +};
> +
> +[scriptable, uuid(f3da7954-52a4-45a9-bd7d-96c518133d0c)]
> +interface calICalendarACLEntry : nsISupports

Some documentation on these interfaces would be nice to have

::: calendar/base/src/calItemBase.js
@@ +99,5 @@
>      /**
>       * @see calIItemBase
>       */
> +    get aclEntry() {
> +        return this.parentItem.mACLEntry;

Does this work in all cases? What if the parent item is a wrapped calIEvent? Then we won't have access to the member. I'd suggest:

get aclEntry() {
    if (this.parentitem == this) {
        return this.mACLEntry;
    } else {
        return this.parentItem.aclEntry;
    }
}

::: calendar/base/src/calUtils.js
@@ +305,5 @@
> + * @param aCalendar     The calendar to check
> + * @return              True if the calendar is writable
> + */
> +function userCanAddItemsToCalendar(aCalendar) {
> +    let aclEntry = cal.aclEntry;

Shouldn't this be aCalendar.aclEntry?

@@ +316,5 @@
> + * @param aCalendar     The calendar to check
> + * @return              True if the calendar is writable
> + */
> +function userCanDeleteItemsFromCalendar(aCalendar) {
> +    let aclEntry = cal.aclEntry;

Same here

::: calendar/lightning/content/lightning-utils.js
@@ +92,4 @@
>      }
> +    /* /ACL code */
> +    else {
> +        let identities = getAccountManager().allIdentities;

cal.getAccountManager()

@@ +92,5 @@
>      }
> +    /* /ACL code */
> +    else {
> +        let identities = getAccountManager().allIdentities;
> +        for (var i = 0; i <  identities.Count(); ++i) {

Consider using <http://mxr.mozilla.org/comm-central/source/mailnews/base/util/iteratorUtils.jsm>, as it has an iterator that makes this more readable.

::: calendar/providers/caldav/calDavCalendar.js
@@ +86,5 @@
>      this.mGenerallySupportedItemTypes = ["VEVENT", "VTODO"];
>      this.mSupportedItemTypes = this.mGenerallySupportedItemTypes.slice(0);
> +    /* ACL code */
> +    this.mACLEntry = null;
> +    this.mACLProperties = {};

These should also be set as keys on the prototype object. You don't need to initialize null values in that case.

@@ +453,4 @@
>      },
>  
>      getProperty: function caldav_getProperty(aName) {
> +        if (this.mACLProperties[aName]) {

Either use calPropertyBag or at least

if (aName in this.mACLProperties)

::: calendar/providers/storage/calStorageCalendar.js
@@ +1975,5 @@
>                      while (this.mSelectEventExceptions.step()) {
>                          var row = this.mSelectEventExceptions.row;
> +                        let exc = none;
> +                        /* we cheat a little because we know that
> +                         getEventFromRow is asynchronous for exceptions */

Don't you mean synchronous here?

@@ +1994,5 @@
>                      while (this.mSelectTodoExceptions.step()) {
>                          var row = this.mSelectTodoExceptions.row;
> +                        let exc = none;
> +                        /* we cheat a little because we know that
> +                         getTodoFromRow is asynchronous for exceptions */

Same here?

Comment 19

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

Wolfgang, to answer your question on IRC: I believe its best if we maybe have a phone call to discuss how to handle the ESR stuff and specifically this patch.

If we had already done the new release process a bit earlier, this patch should have landed at least 7 weeks ago. Of course we are not in a normal situation with the releases, so I agreed that this patch could land on aurora which means the patch would need to land until today.

Looking at the earlier patches, it looked to me like the hooks were mostly in the UI and only slightly changed the behavior. More so these changes were always special-cased to having an ACL provider installed which further reduced the risk. Therefore I was confident that we could slip this in just before the merge without much risk.

The patch you posted changes quite a lot more than I expected, especially in the storage provider. This could cause unforeseen consequences if these changes cause regressions with the storage provider that also affect users without a specific ACL provider. Especially the async calls worry me, since calISyncWriteCalendar assumes that the storage calendar is synchronous in some cases.

If we slip in these changes now, there will be only a handful of test builds (1-3) in which we have time to fix the behavior. Any regression fixes we do will have to just work and need to be there on time. Everything needs to be working until ~ Jan 24th, which means finding all quirks, reviewing them, testing them ourselves, having them tested by the users in a beta build which is done once a week at max. This sounds like a risk I don't think we should take, especially since I will be busy with exams in the next 8 weeks.

Nevertheless, I'm all for finding a solution to getting this bug in. I'd love to allow you to unfork your work asap since this means you have more time to do productive things instead of constant forward-portings. Lets talk about this!

Comment 20

[Wolfgang Sourdeau]

Attachment: Implementation of calendar acls through stubs (take 2)

This new version takes the above comments into account. Please note that a few comments about the previous patches were actually about code that was already present in Lightning but reindented. I fixed it too nonetheless as it was indeed more compliant and consistent with the best practices.

That being written, I have also changed the interface again in this version to avoid the asynchronous changes to calStorageCalendar, resulting in 2 * 2 lines changes in the end. This will only require initializing item entries from our caldav acl manager when the calendar acl entry is being generated. Further item entries will be generated synchronously, which should be acceptable anyway, considering the risk being avoided.

I have retested the implementation by playing with the attribute values in calDefaultACLManager. Regarding the UI, everything seems to be working properly
with regardes to item creation and modification. I did not test the "respond to" or "date and time only" cases. Anyway, for the release, this change should be absolutely seamless.

Regarding further discussions, I would of course be glad to sort out any other issue. I'll connect to #calendar as soon as I get to the office tomorrow morning .

Comment 21

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

Comment on attachment 583054 [details] [diff] [review]
Implementation of calendar acls through stubs (take 2)

Review of attachment 583054 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, I tested the patch now which made me come across some other issues and minor nits.

The biggest issue I'm having some trouble deleting events with this patch applied. It works well from the event dialog, but neither context menu > Delete Item nor the Del key works. I assume some command is not correctly enabled. It doesn't happen without the patch applied. I need to try again with the original patch applied (without the minor changes I made) to make sure I didn't break this myself.

::: calendar/acl/default/calDefaultACLManager.js
@@ +46,5 @@
> +
> +calDefaultACLManager.prototype = {
> +    mCalendarEntries: null,
> +
> +    /* nsIClassInfo */

I've decided to give the new XPCOMUtils.generateCI a try and this is a perfect candidate. I'll change this for you.

@@ +179,5 @@
> +const scriptLoadOrder = [
> +    "calUtils.js"
> +];
> +
> +function NSGetFactory(cid) {

We can go with even less code in this file. I'll fix this for you.

::: calendar/base/content/dialogs/calendar-event-dialog.js
@@ +2282,5 @@
> +    // calendar-user-address-set. The organzerID is the owner of the calendar.
> +    // If it's different, that is because someone is acting on behalf of
> +    // the organizer.
> +    if (item.organizer && item.calendar.aclEntry) {
> +        let userAddresses = item.calendar.aclEntry.getUserAddresses({});

I tried this with my Mozilla caldav calendar and the function returned all email addresses from all identities I have set up. Is this wanted? I would have expected it should returns all the calendar-user-address-set emails and if thats not there then the email that is set up in the calendar properties?

::: calendar/base/content/widgets/calendar-list-tree.xml
@@ +1063,4 @@
>      <handlers>
>        <handler event="select"><![CDATA[
>          this.compositeCalendar.defaultCalendar = this.getCalendar(this.tree.currentIndex);
> +        calendarUpdateNewItemsCommand();

This needs to be

if ("calendarUpdateNewItemsCommand" in window) {
  calendarUpdateNewItemsCommand();
}

since the binding can potentially be used in dialogs that are not the main window. Too bad we have to do the updating right here, but I guess we can do that for now.

Ideally, an observer on the composite calendar should be updating the command when the calendar changes.

::: calendar/base/public/calICalendarACLManager.idl
@@ +82,5 @@
> +
> +    /* Returns the list of user ids matching the user accessing the
> +       calendar. */
> +    void getUserAddresses(out PRUint32 aCount,
> +                          [array, size_is(aCount), retval] out wstring aAddresses);

Any specific reason for wstring instead of AUTF8String?

::: calendar/base/src/calItemBase.js
@@ +99,4 @@
>      /**
>       * @see calIItemBase
>       */
> +    get aclEntry() {

I was thinking how we could remove the mACLEntry code in the storage provider while keeping the attribute readonly in the IDL.

What do you think about inserting some logic in the getter like:

if (!this.mACLEntry && this.aclManager) {
   this.mACLEntry = this.aclManager.getItemEntry(this);
}

and then make sure that the acl manager returns null in cases where an acl entry isn't appropriate?

Would this work? Any sideeffects you could think of?

Comment 22

[Wolfgang Sourdeau]

[snip]

> @@ +179,5 @@
> > +const scriptLoadOrder = [
> > +    "calUtils.js"
> > +];
> > +
> > +function NSGetFactory(cid) {
> 
> We can go with even less code in this file. I'll fix this for you.

Thanks!

> 
> ::: calendar/base/content/dialogs/calendar-event-dialog.js
> @@ +2282,5 @@
> > +    // calendar-user-address-set. The organzerID is the owner of the calendar.
> > +    // If it's different, that is because someone is acting on behalf of
> > +    // the organizer.
> > +    if (item.organizer && item.calendar.aclEntry) {
> > +        let userAddresses = item.calendar.aclEntry.getUserAddresses({});
> 
> I tried this with my Mozilla caldav calendar and the function returned all
> email addresses from all identities I have set up. Is this wanted? I would
> have expected it should returns all the calendar-user-address-set emails and
> if thats not there then the email that is set up in the calendar properties?

That is indeed wanted and it's the expected behaviour. The calendar acl manager provided in the patch is a "dummy" acl manager, so it has no knowledge about caldav at all and is there to provide a best possible default behaviour, which is basically the same as without this patch. I still need to work on my caldav acl manager for SOGo, which should be compatible with any caldav server implementing the acl spec and which does provide what you mention.

> 
> ::: calendar/base/content/widgets/calendar-list-tree.xml
> @@ +1063,4 @@
> >      <handlers>
> >        <handler event="select"><![CDATA[
> >          this.compositeCalendar.defaultCalendar = this.getCalendar(this.tree.currentIndex);
> > +        calendarUpdateNewItemsCommand();
> 
> This needs to be
> 
> if ("calendarUpdateNewItemsCommand" in window) {
>   calendarUpdateNewItemsCommand();
> }
> 
> since the binding can potentially be used in dialogs that are not the main
> window. Too bad we have to do the updating right here, but I guess we can do
> that for now.
> 
> Ideally, an observer on the composite calendar should be updating the
> command when the calendar changes.

Is that possible with the current state of the code?

> 
> ::: calendar/base/public/calICalendarACLManager.idl
> @@ +82,5 @@
> > +
> > +    /* Returns the list of user ids matching the user accessing the
> > +       calendar. */
> > +    void getUserAddresses(out PRUint32 aCount,
> > +                          [array, size_is(aCount), retval] out wstring aAddresses);
> 
> Any specific reason for wstring instead of AUTF8String?

No, I found both mentionned in the idls but I didn't know one was preferred over the other. Is this documented?

> 
> ::: calendar/base/src/calItemBase.js
> @@ +99,4 @@
> >      /**
> >       * @see calIItemBase
> >       */
> > +    get aclEntry() {
> 
> I was thinking how we could remove the mACLEntry code in the storage
> provider while keeping the attribute readonly in the IDL.
> 
> What do you think about inserting some logic in the getter like:
> 
> if (!this.mACLEntry && this.aclManager) {
>    this.mACLEntry = this.aclManager.getItemEntry(this);
> }
> 
> and then make sure that the acl manager returns null in cases where an acl
> entry isn't appropriate?
> 
> Would this work? Any sideeffects you could think of?

It makes sense, especially since we need the acl entry onlyi in the views and only when the user is not the calendar owner, so it would avoid a lot of useless instantiations.

Note that the aclManager is attached to the item calendar, not to the item itself...

I am going to test the isse with the delete command and provide you with a small fix to apply on top of this patch, since you already have modified it a bit....

Thanks!

Comment 23

[Wolfgang Sourdeau]

Attachment: Fix the delete command

Indeed, there was a reversed condition.

Comment 24

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

(In reply to Wolfgang Sourdeau from comment #22)

> > I tried this with my Mozilla caldav calendar and the function returned all
> > email addresses from all identities I have set up. Is this wanted? I would
> > have expected it should returns all the calendar-user-address-set emails and
> > if thats not there then the email that is set up in the calendar properties?
> 
> That is indeed wanted and it's the expected behaviour. The calendar acl
> manager provided in the patch is a "dummy" acl manager, so it has no
> knowledge about caldav at all and is there to provide a best possible
> default behaviour, which is basically the same as without this patch. I
Isn't the default behavior to observe only the email address set in the properties?



> > since the binding can potentially be used in dialogs that are not the main
> > window. Too bad we have to do the updating right here, but I guess we can do
> > that for now.
> > 
> > Ideally, an observer on the composite calendar should be updating the
> > command when the calendar changes.
> 
> Is that possible with the current state of the code?

I see there is a calICompositeObserver in calendar-management.js. It is already updating commands when adding/removing a calendar, we could add the extra (and wrongfully missing) function onDefaultCalendarChanged() and also update all commands then. This should get rid of the need for updating in the calendar list binding, I'm not sure about the onLoad though. Maybe you can confirm that its not needed in the onLoad event either?

> > Any specific reason for wstring instead of AUTF8String?
> 
> No, I found both mentionned in the idls but I didn't know one was preferred
> over the other. Is this documented?
Not that I know of. I don't know whats best, but I've been using AUTF8String where I can. I believe AUTF8String makes it a nsCString in C++ instead of a wchar_t *.

> > What do you think about inserting some logic in the getter like:
> > 
> > if (!this.mACLEntry && this.aclManager) {
> >    this.mACLEntry = this.aclManager.getItemEntry(this);
> > }
> It makes sense, especially since we need the acl entry onlyi in the views
> and only when the user is not the calendar owner, so it would avoid a lot of
> useless instantiations.

I've changed this now, lets see. The way it is now, with the default acl manager the aclEntry will always return something. I guess its safe to check this in calling code again, but we could also just assure callers that it will always return non-null if you want.

Comment 25

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

(In reply to Philipp Kewisch [:Fallen] from comment #24)
> (In reply to Wolfgang Sourdeau from comment #22)
> 
> > > I tried this with my Mozilla caldav calendar and the function returned all
> > > email addresses from all identities I have set up. Is this wanted? I would
> > > have expected it should returns all the calendar-user-address-set emails and
> > > if thats not there then the email that is set up in the calendar properties?
> > 
> > That is indeed wanted and it's the expected behaviour. The calendar acl
> > manager provided in the patch is a "dummy" acl manager, so it has no
> > knowledge about caldav at all and is there to provide a best possible
> > default behaviour, which is basically the same as without this patch. I
> Isn't the default behavior to observe only the email address set in the
> properties?

With all the emails in there, I now see an ORGANIZER with SENT-BY the first email address in my list of identities. I don't have this email set in the properties for the account. I don't know how I managed to create this event, but I can imagine users don't want to expose unlinked email addresses.

Comment 26

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

Hmm I can't reproduce the issue I had, lets wait until it returns. Pushing the patch now.

Comment 27

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

Pushed to comm-central changeset 6a2ea51fb761

Comment 28

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

Backported to releases/comm-aurora changeset 2c80985a812d

Comment 29

[Ludovic Marcotte]

Target Milestone 1.3? Does this mean it won't be included in 1.2?

What has to be done so it's included in 1.2? Our users won't be too happy with running non-ESR versions of Thunderbird (ie., 10).

Comment 30

[Alessio]

I confirm, not too happy.

Comment 31

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

This should have made it to comm-beta, which would have changed the milestone to 1.2. Not sure why that didn't happen. I'll investigate on the machine I pushed this on soon.

Comment 32

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

Comment on attachment 583054 [details] [diff] [review]
Implementation of calendar acls through stubs (take 2)

r+ with the minor fixes I've applied. See the pushed changesets for a final patch if needed.

Comment 33

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

Backported to releases/comm-beta changeset 3b8a286fd1b0

Comment 34

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

Looks like I just closed my laptop a second to early, connection was reset. I've pushed to comm-beta now and will make sure 1.2b1 builds will be created asap.