Closed Bug 370146 Opened 17 years ago Closed 17 years ago

API enhancement: Searching for calendars

Categories

(Calendar :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: dbo, Assigned: dbo)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch adding searchForCalendars (obsolete) — Splinter Review
Proposing search capability for calICalendarProvider.
Attachment #254803 - Flags: first-review?(dmose)
Assignee: nobody → daniel.boelzle
Relies on calIRequest proposal from bug 329034.
Depends on: 329034
The existing methods on calICalendarProvider all pass in a "location" in one way or another (either as a URI or calICalendar object) to tell the provider exactly where to do the deletion/creation/get.  Given that searchCalendars doesn't seem to have such a parameter, it's not clear to me how any given provider would know where to search.  Does this need such a parameter?  If not, does the search method maybe belong on a "session" interface?  Otherwise it seems like the request is telling the provider to search "all calendars in the world" or maybe "all calendars that you have any knowledge of", and I don't see how either of those makes sense in the general case.

Also, what exactly are the "properties" referred to in the flags in the patch?  X- properties on the VCALENDAR itself?
(In reply to comment #2)

It helps me to sum up my understanding of the existing API, so please correct me if I am wrong.

1. We are having a (static) program id scheme for provider components. That way we could create a provider of some type (explicitly: without the need to actually pass a url to it). However, due to the fact that one cannot really use the provider instance without setting an url, it is IMO a design flaw to implement both calICalendar and calICalendarProvider at the same instance.

2. We are having an API to create calICalendar instances that have been registered at the (local) app, i.e. you call the calendar manager which creates the provider instance of demanded type, then sets its url.

3. We are having calICalendarProvider which is (optionally?) implemented by the same provider instances to 
  a) create/delete calendars
     (real ones, not the registered ones of the calendar manager)
  b) get the displayName of the provider type (static name)
  c) get calICalendar instances passing a url
  d) configure a calendar of that type.

It's still not clear to me how 3d) ought to work. How does the chrome get the actual calendar instance (url) that is to be configured by it?

Because of this mixed up situation, I currently assume provider instances always have an url and intuitively added searching to calICalendarProvider.

You are right, searching needs to have scope, i.e. a url/session/... If calICalendarProvider is meant to cleanly work without a url, then I have to move it, e.g. to a separate interface (like free-busy).

However, how would searching be performed? There is no unified world-wide search framework, so a calendar search could only take every registered calendar into account. The calendars have been registered by the user, so this is the least clue.

My favourite model for all this stuff is:
- Providers just implement an interface similar to calICalendarProvider (as service, single instance), so the calendar manager uses an indirection (clean factory). That factory object creates account objects (e.g. of interface calIAccount).
- calIAccount objects hold the url and all session and credential like data (user name, session tickets etc.), refers to the set of subscribed calendars, ...
- calIAccount objects create calICalendar objects (default calendar, subscribed ones)
- calIAccount objects can search for calendars (scope).
- calendar configuration capabilities (chrome) is divided into
  - calIAccount: account configuration (credentials etc)
  - calICalendar: calendar configuration (access rights, name)

IMO it wouldn't make sense to model credentials, access rights and that kind of stuff into IDL, because I doubt that we can generalize this kind of data. I fear there will always be a calendar system that doesn't fit into that IDL schema. Thus calIAccount should only attribute its url and have methods to get calICalendar instances, access to the user's default calendar instance and the set of subscribed ones.
I am still unsure whether we should expose an explicit calISession or the like. Currently it seems sufficient to implicitly create a session if demanded and save the user's credentials for single signon (in case of lost/timed-out sessions).
Maybe it would make sense to unify authentication; WCAP (as well as gdata) currently only support plain user/password prompts and password manager lookup. I can think of system pam (pluggable authentication modules), smart card authentication and the like. I haven't dipped deeper into mozilla's platform capabilities in this field: maybe someone can give good advice what's possible and how. Then, we could decide whether it makes sense to model that in IDL (calIAccount, calISession?) or better leave it out.
(In reply to comment #2)
> Also, what exactly are the "properties" referred to in the flags in the patch? 
> X- properties on the VCALENDAR itself?

The information that is taken into account while searching; the term "properties" is obviously misleading. I should fix the comment.
(In reply to comment #3)
> 
> 1. We are having a (static) program id scheme for provider components. That way
> we could create a provider of some type (explicitly: without the need to
> actually pass a url to it). 

Correct.

> However, due to the fact that one cannot really use the provider instance
> without setting an url, it is IMO a design flaw to implement both
> calICalendar and calICalendarProvider at the same instance.

You're right that allowing a single object to implement both of those interfaces is conceptually a little strange in the sense that there could exist multiple objects implementing the provider interface for a single provider.  However, it's not clear to me that that causes any particular problems in practice; at some level those methods are essentially acting as class-static methods would in other languages.  At one point, Joey had me convinced that we should in fact simply merge all the calICalendarProvider methods into calICalendar.  

Are you aware of any particular problems that the current design is likely to cause?

Unfortunately, I have to head out for the evening, so I'll get back to this tomorrow.
(In reply to comment #3)
>
> 2. We are having an API to create calICalendar instances that have been
> registered at the (local) app, i.e. you call the calendar manager which creates
> the provider instance of demanded type, then sets its url.

Right.
 
> 3. We are having calICalendarProvider which is (optionally?) implemented by the
> same provider instances to 
>   a) create/delete calendars
>      (real ones, not the registered ones of the calendar manager)
>   b) get the displayName of the provider type (static name)
>   c) get calICalendar instances passing a url
>   d) configure a calendar of that type.

calICalendarProvider is a bit weird in the sense that the implementation and design were only partially done.   I put together a straw-man version of the interface, started to do the implementation work, and then got pulled off onto other things.  The result is that the interface itself is checked in, but the calendar manager doesn't really use it in all the places that it should.  Bug 297721 has some of the history here.

> It's still not clear to me how 3d) ought to work. How does the chrome get the
> actual calendar instance (url) that is to be configured by it?

The theory was that the calendar manager would cause the calICalendarProvider instance to create and return a calendar instance, and the manager would return that to the (chrome) caller.  Does this answer your questions?

> You are right, searching needs to have scope, i.e. a url/session/... If
> calICalendarProvider is meant to cleanly work without a url, then I have to
> move it, e.g. to a separate interface (like free-busy).

Part of the issue here is that calICalendarProvider was intended to represent 
the protocol or format implementation itself.  For example, even if a user were to have two WCAP accounts on separate servers, there would theoretically only need to be a single implementation of calICalendarProvider, so there would be no place for the second URL.

It seems like it might be helpful to step back from the stuff here and get the calICalendarProvider strategy (i.e. bug 297721) sorted out.

> However, how would searching be performed? There is no unified world-wide
> search framework, so a calendar search could only take every registered
> calendar into account. The calendars have been registered by the user, so this
> is the least clue.

As I understand it, the proposal is to search all accounts containing calendars that have been registered by a user (i.e. you should be able to find a calendar owned by a coworker so that you can in fact subscribe/register).

> My favourite model for all this stuff is:
>
> [...]

That sounds workable to me on the surface, but I don't really feel like I have enough of a feel for the model proposed by CalDAV, gData, etc. to really provide good feedback.  I've added some folks who might be able to provide more feedback; perhaps they can comment...

> 
> IMO it wouldn't make sense to model credentials, access rights and that kind of
> stuff into IDL, because I doubt that we can generalize this kind of data. 

That sounds plausible.

> I am still unsure whether we should expose an explicit calISession or the like.
> Currently it seems sufficient to implicitly create a session if demanded and
> save the user's credentials for single signon (in case of lost/timed-out
> sessions).

Sounds reasonable.  Note that for (at least) HTTP-based protocols, we can probably get necko and satchel to do a lot of the heavy lifting.

> Maybe it would make sense to unify authentication; WCAP (as well as gdata)
> currently only support plain user/password prompts and password manager lookup.
> I can think of system pam (pluggable authentication modules), smart card
> authentication and the like. I haven't dipped deeper into mozilla's platform
> capabilities in this field: maybe someone can give good advice what's possible
> and how. 

I don't have a lot of experience with that code, but I think looking at nsIAuthModule is probably the place to start looking.

Since there's still more discussion to happen here, and this is my last day on the calendar project, I'm removing the r? flag. 
Attachment #254803 - Flags: first-review?(dmose)
Blocks: 376585
Attached file calICalendarSearchProvider.idl (obsolete) —
New approach: Similar to freebusy (bug 370148), a central search provider could serve for calendar searching.
Attachment #254803 - Attachment is obsolete: true
Attachment #283198 - Flags: review?(philipp)
Comment on attachment 283198 [details]
calICalendarSearchProvider.idl

>[scriptable, uuid(306DA1C9-DB54-4ef3-B27E-FEA709F638FF)]
>[scriptable, uuid(2F2055CA-F558-4dc8-A1D4-11384A00E85C)]
I would prefer the hex characters being lowercase, as with most interfaces

>    const unsigned long EXACT_MATCH = 1;
>    const unsigned long INCLUDE_DESCRIPTION = 1 << 4;
What if the provider doesn't support flags like this? i.e the provider only allows partial matches? If we have a search dialog that allows exact matches and the user recieves matches that only contain the words, then something is wrong. I noticed you added a comment below, but still there is no way for callers to find out which flags are supported for the provider, so there is no way to differ in the UI. We might need a field like |readonly attribute unsigned long supportedFlags| that the UI can query  to decide what elements to show. 

>    /**
>     * Specifies whether the search should include the name of the calendar.
>     */
>    const unsigned long INCLUDE_NAME = 1 << 1;
If I understand correctly, this is the oposite to EXACT_MATCH. If this is not true, it needs a more descripive comment. If so, then I think we should get rid of this flag, and add a comment for EXACT_MATCH, telling the developer that if EXACT_MATCH is not specified, partial matches will be returned. I think its better to not allow ambiguities like (EXACT_MATCH | INCLUDE_NAME) in the first place.


>     * Specifies whether the search should include the calendar's calId.
>     * Specifies whether the search should include the calendar's ownerId.
A bit more description as to what a calId or ownerId is would be nice, to make it easier for new developers.

>     * @param aString     the search string to match
>     * @param aOptions    the search options
"A Combination of the above constants" is more descriptive, also maybe call it aFlags.

>     * @param aMaxResults maximum number of results
>     *                    (0 denotes provider specific default maximum)
Drop the "default" to make clear that this is the proivder specific maximum, not the provider specific default value.

>     * @param aListener   called with an array of calICalendar objects
>     * @return            optional operation handle to track the operation
Minor nits above: Please use correct capitalization, at least in IDL files like this.

calICalendarSearchService looks fine. r- for now, mostly to take care of the supported search parameters. This might also get interesting for calICalendarSearchService, since at some point with enough providers, supportedFlags will be pretty minimal.
Attachment #283198 - Flags: review?(philipp) → review-
(In reply to comment #8)
> >    const unsigned long EXACT_MATCH = 1;
> >    const unsigned long INCLUDE_DESCRIPTION = 1 << 4;
> What if the provider doesn't support flags like this? i.e the provider only
> allows partial matches? If we have a search dialog that allows exact matches
> and the user recieves matches that only contain the words, then something is
> wrong. I noticed you added a comment below, but still there is no way for
> callers to find out which flags are supported for the provider, so there is no
> way to differ in the UI. We might need a field like |readonly attribute
> unsigned long supportedFlags| that the UI can query  to decide what elements to
> show. 
The passed flags are not intended in a strong sense, but rather passed flags as 'hints'. Following your suggestion, the service's effective flags would be the least common denominator of all search providers, which renders such a dialog useless once we have a single simple search provider hooked in.

> >    /**
> >     * Specifies whether the search should include the name of the calendar.
> >     */
> >    const unsigned long INCLUDE_NAME = 1 << 1;
> If I understand correctly, this is the oposite to EXACT_MATCH. If this is not
> true, it needs a more descripive comment. If so, then I think we should get rid
> of this flag, and add a comment for EXACT_MATCH, telling the developer that if
> EXACT_MATCH is not specified, partial matches will be returned. I think its
> better to not allow ambiguities like (EXACT_MATCH | INCLUDE_NAME) in the first
> place.
EXACT_MATCH should indicate how the match should be performed, but INCLUDE_xyz what data should be incorporated into the search.

> >     * Specifies whether the search should include the calendar's calId.
> >     * Specifies whether the search should include the calendar's ownerId.
> A bit more description as to what a calId or ownerId is would be nice, to make
> it easier for new developers.
ok.

> >     * @param aString     the search string to match
> >     * @param aOptions    the search options
> "A Combination of the above constants" is more descriptive, also maybe call it
> aFlags.
> 
> >     * @param aMaxResults maximum number of results
> >     *                    (0 denotes provider specific default maximum)
> Drop the "default" to make clear that this is the proivder specific maximum,
> not the provider specific default value.
ok.

> >     * @param aListener   called with an array of calICalendar objects
> >     * @return            optional operation handle to track the operation
> Minor nits above: Please use correct capitalization, at least in IDL files like
> this.
which ones?

thanks for reviewing
(In reply to comment #9)
> The passed flags are not intended in a strong sense, but rather passed flags as
> 'hints'. Following your suggestion, the service's effective flags would be the
> least common denominator of all search providers, which renders such a dialog
> useless once we have a single simple search provider hooked in.
I'm just concerned, that an option for an "exact match", especially when only one provider is selected for searching, doesn't give the user the expected results, if the provider doesn't support exact matches. I guess we can postpone this until we create the UI, but I'd be against just ignoring the case. Maybe supportedFlags could just be useful for the certain providers, so that if only one provider is selected for search, that the exact match item is disabled.

> EXACT_MATCH should indicate how the match should be performed, but INCLUDE_xyz
> what data should be incorporated into the search.
I understand. In that case maybe something like "This flag is independant of the INCLUDE_xyz flags" will be sufficient.


> > >     * @param aString     the search string to match
> > >     * @param aOptions    the search options
> > >     * @param aMaxResults maximum number of results
> > >     *                    (0 denotes provider specific default maximum)
> > >     * @param aListener   called with an array of calICalendar objects
> > >     * @return            optional operation handle to track the operation
> > Minor nits above: Please use correct capitalization, at least in IDL files like
> > this.
> which ones?
Specifically: The, The, Maximum, Called, Optional
- simplifies calICalendarSearchProvider to just a HINT_EXACT_MATCH for now. We are free to add more hints coming in the future.
- implements calICalendarSearchService
- adopts WCAP code
Attachment #283198 - Attachment is obsolete: true
Attachment #286815 - Flags: review?(philipp)
Flags: wanted-calendar0.8+
Status: NEW → ASSIGNED
Some parts of comment 8 were not addressed. (i.e "provider specific default maximum", captialization,...). Any specific reason?
- I missed to remove the "default" maximum term, thanks for that.
- I don't like capitalization for parameters; moreover looking at other IDL, this is quite unusual.
- Anything else I've missed?
Comment on attachment 286815 [details] [diff] [review]
revised calICalendarSearchProvider API, service implementation, WCAP changes

I'm not sure you understand me. I just mean to capitalize the beginning of the description of the parameter (i.e "The search options" vs "the search options"), not the parameter itself.

I think that was it, with those changes r=philipp.
Attachment #286815 - Flags: review?(philipp) → review+
Checked in on HEAD and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Verified in Lightning build 2007111803 -> task is fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.