[mkcal] Add a search method.

[dcaliste]

This new method is loading incidences
matching a string pattern in summary, description
or location. Incidences are added to the calendar
and the list of matching instance identifiers is
return to avoid searching in the calendar for match in client code.

@pvuorela, to add search capability to the calendar application, there are two possibilities : either we load all incidences and then search inside the memory calendar for matching incidences, or we delegate the search function to the database and load only matching incidences. The first one does not scale well for large number of incidences. So I went for the second option. But doing so, one needs to know what the database has loaded, to avoid searching again later in the memory calendar. But such information of loaded incidences does not exist at the moment for existing loading routines.

It exists some functions though like insertedIncidences() that poke the database and return in a separated list, matching incidences. So I may have added the search routine with an API such as the one of insertedIncidences(). But this has the issue that the notebook information is lost in such API (you don't know to which notebook returned incidences belong to), and anyway, the incidences will have to be loaded in the calendar to display the results.

That's why I choose this hybrid API for the search routine : it is a loading routine, but it also returns the list of loaded incidences (as instance identifiers to be thread safe, in case we will go between thread boundaries since Incidence::List is not thread safe).

[pvuorela]

Something here should filter out disabled notebooks?

[dcaliste]

I'm not sure filtering should be done at DB level, even if it would simplify things. Maybe more at nemo-qml-plugin-calendar level… Because load methods don't care about disabled calendars. The date range method for instance is loading everything in range. The filtering out is done later in calendarworker.

[dcaliste]

I've hardened a bit the test to check that SQlite wildcard charaters (_ and %) can be passed as literal values in the query without returning false positives.

[pvuorela]

Actually is it maybe a bit strange when search() method which is defined to look up from the storage also loads some data, which it doesn't promise to do? Should this have more of selectIncidences() type of handling? Compare to deleted/modified/insertedIncidences() methods.

A bit also thinking alternatives of nemo-calendar just checking the loaded incidences, or if here there should be limits/control on how distant events should be returned.

The ordering is now an implementation detail, but pondering should there be some control over that too.

[dcaliste]

Actually is it maybe a bit strange when search() method which is defined to look up from the storage also loads some data, which it doesn't promise to do?

Well, it can be discussed. I agree that it's loading more than necessary for the search, but the true results of the search that is identifiers are only containing matching incidences. The fact that it is loading more (it can be mentined in the documentation) is a shortcut for the main (only?) usecase:

• you want to search for incidences with foo, so you call search() which loads nothing and get the identifiers of incidences matching foo,
• the identifiers give you no information, so you need to load the incidences anyway. For each identifier, you call loadIncidenceInstance() which will actually load more than the instance you are interested in, by design to ensure a consistant view of the calendar.

I don't see a usecase where the search() call is not followed by a load of the instances. You may have already some of the matching identifiers in memory, so the load of these is not necessary. I agree that could be a minor optimisation.

Having the search() API following the current selectIncidences() API is not an option I think for two reasons:

• this API is broken, because it is trashing the notebook belonging. Indeed, it is returning a list of incidences, but without notebook information. The norebook information is stored (for good or bad) by a calendar holding these incidences. But since they are presented only as a list and not inside a MemoryCalendar for instance, the notebook information is lost.
• if we adjust a bit the current API to keep the notebook association (like returning a QHash<QString, Incidence::Ptr> instead of a list or returning a MemoryCalendar), the fact that we're going to use these incidences, means that we need to add them to the calendar associated to the storage. So it means to add the returned incidences to the mCalendar. So why not directly do it in the search() call ?

The problem of limit is valid. It could be added to the search() API, saying that we load only the first N matching incidences. Currently, I've played with it searching for stupid strings like a, returning 1500 matches from my database. It's not slow : around 2 seconds to get the list displayed. But I agree that it's useless to get a list of more than 20-50 matches when you look for something… So let's add a int limit argument that UI will hardcode to 50 or something like that.

The same for ordering, we can still add it later as an optional argument if necessary. But I don't plan to add search options to the API. Calendars are simpler than emails in my opinion and less populated. So searching with the most recent on top should cover most of the cases (like when is this doctor appointment ?).

[pvuorela]

Yea, guess since the events would need to get loaded to show anything useful, and the loading already gets done here, better then just to add to the calendar. In that case could just consider to note on the method definition that it will load what it finds.

For the ordering, if we allow to adjust that, I guess apps would like to then also define a starting point. Otherwise if the order is reversed to show the oldest ones first, there might be quite pointless ones in the beginning. Something like "start, end, order" might fit needs. But maybe good enough for starters with just what there is currently.

[dcaliste]

In that case could just consider to note on the method definition that it will load what it finds.

Good suggestion. I've amended the API doc string to highlight the fact that the function will actually load matching incidences into the memory calendar.

Something like "start, end, order" might fit needs.

Maybe, at one point, we may want to add such arguments. In my opinion, for the moment, the use case may be simple enough just to return matching incidence with a limit to avoid clogging memory.

[pvuorela]

The nemo plugin is now passing a limit for this, but it's missing here?

[dcaliste]

Well, I'm a bit confused. I guess I forgot to push the changes and then I lost them in a recent rebase… Anyway, I've added the limit argument again and properly pushed !

I've also documented the fact that the ::search() routine is actually loading into memory search results.