GNOME Bugzilla – Bug 705178
Provide sorted contact lists with a cursor API
Last modified: 2013-10-24 16:41:50 UTC
This patch set introduces a cursor API for the addressbook, allowing one to retrieve ordered results from the addressbook and navigate through a potentially large contact list without loading many contacts into memory. The cursor implementation (and API) feature rich locale sensitive sorting as well as interaction with the alphabet appropriate for the user's locale. Along with the locale sensitive sorting features, we use the org.freedesktop.locale1 interface on the system bus to listen for notifications of dynamic system wide locale changes, allowing sort order and active alphabets to be reloaded when locales change. I can attach patches, but there are currently 17 of them related to the cursor implementation (including tests and example code). I've squashed everything into a staging branch which can be found here: https://git.gnome.org/browse/evolution-data-server/log/?h=cursor-staging The first five patches on the branch are actually the patches for bug 701260.
I cannot build the branch out of the box (at commit 7ee3389), I get this error, which I will fix locally): e-book-backend.c: In function 'book_backend_dispatch_next_operation': e-book-backend.c:222:2: warning: 'g_io_scheduler_push_job' is deprecated (declared at /build/test-cli/include/glib-2.0/gio/gioscheduler.h:36): Use '"GThreadPool or g_task_run_in_thread"' instead [-Wdeprecated-declarations] g_io_scheduler_push_job ( ^ e-data-book.c: In function 'data_book_interpret_locale': e-data-book.c:1688:3: error: implicit declaration of function 'setlocale' [-Werror=implicit-function-declaration] const gchar *system_locale = setlocale (LC_COLLATE, NULL); ^ e-data-book.c:1688:3: warning: nested extern declaration of 'setlocale' [-Wnested-externs] e-data-book.c:1688:43: error: 'LC_COLLATE' undeclared (first use in this function) const gchar *system_locale = setlocale (LC_COLLATE, NULL); ^ e-data-book.c:1688:43: note: each undeclared identifier is reported only once for each function it appears in e-data-book.c: In function 'data_book_constructed': e-data-book.c:2009:17: error: 'LC_COLLATE' undeclared (first use in this function) setlocale (LC_COLLATE, NULL)); ^ cc1: some warnings being treated as errors
Oops, it was commit a4ea66f4fe8 (the previous one is from evo's webkit-composer branch) :)
Related warnings from a build: cursor-data.c: In function ‘cursor_load_data’: cursor-data.c:111:21: warning: unused variable ‘cursor’ [-Wunused-variable] EBookClientCursor *cursor = NULL; ^ cursor-data.c:110:30: warning: unused variable ‘setup’ [-Wunused-variable] ESourceBackendSummarySetup *setup = NULL; ^ cursor-data.c: At top level: cursor-data.c:262:1: warning: ‘get_addressbook_directory’ defined but not used [-Wunused-function] get_addressbook_directory (ESourceRegistry *registry, ^ cursor-example.c: In function ‘cursor_example_update_view’: cursor-example.c:416:3: warning: implicit declaration of function ‘cursor_slot_set_from_contact’ [-Wimplicit-function-declaration] cursor_slot_set_from_contact (priv->slots[i], contact); ^ cursor-example.c: In function ‘cursor_example_update_status’: cursor-example.c:485:24: warning: unused variable ‘error’ [-Wunused-variable] GError *error = NULL; ^ cursor-example.c: At top level: cursor-example.c:80:24: warning: ‘cursor_example_update_sensitivity’ declared ‘static’ but never defined [-Wunused-function] static void cursor_example_update_sensitivity (CursorExample *example); ^ cursor-navigator.c: In function ‘cursor_navigator_constructed’: cursor-navigator.c:102:26: warning: unused variable ‘priv’ [-Wunused-variable] CursorNavigatorPrivate *priv = navigator->priv; ^
How do I change the locale of an EBookClient? I see e_book_client_get_locale() (its definition in e-book-client.h suffers of coding style issues: a) spaces instead of tabs; b) 'const gchar *\t", not 'const gchar *"), but no _set_locale(), which I'd expect to be available - or at least something like that on the EBookClientCursor. I tried to run the addressbook factory with other LANG, even LC_COLLATE, but no luck, the test application still shows "Cursor example started in locale: en_US.UTF-8". Imagine that the backend runs in some system/session locale, but I want to run any client application in a different local. I would want to get results in order based on that local. This might be difficult, no doubt, when it comes to multi-client environment, where the factory has only one EBookBackend for all D-Bus clients (that's why it seems like the EBookClientCursor property).
I have created (copied) a book and if I search its content for "cer" I get this runtime warning on the cursor-test console (actual email is changed): > Gtk-WARNING **: Failed to set text from markup due to error parsing markup: > Error on line 1 char 67: 'user@example.com' is not a valid name: '@' and this user contact if missing from the list.
Scrolling by one, in the cursor-example, by holding down one of the scroll arrows makes quite large of CPU usage on the factory side. I hope you'll look on it too. My filtered list has 70 rows (out of ~21K) and it's eating one core during the button down time.
Review: a) book_backend_file_set_locale() calls cursors_locale_changed() before the backend's local private variable is updated, which can cause some "read of old value" in case any of the cursors will need to get a locale of the backend during the e_data_book_cursor_load_locale() call. You also do not lock the priv->cursors variable usage, but the public functions can be called from lultiple threads, thus you should add some thread safety on that code ( b) missing space at '(GDestroyNotify)g_object_unref', but the g_object_unref already conforms to the GDestroyNotify prototype, thus you can use it without the cast. c) "/* Now that we've modified the contact(s), notify cursors of the changes", doing modify with remove & add is to flash the UI part, as it can be seen in evolution. Wouldn't it be better to have only one notify for the modified contacts? d) book_backend_file_delete_cursor() what if you are asked to delete a cursor which you do not own anymore? e) inside e_book_backend_file_bump_revision() you removed the call to e_book_backend_notify_property_changed(). How is the client notified about revision change now? f) typos (some of them are there multiple times): - in "changing it's active locale setting" s/it's/its/ - e_book_client_cursor_get_contact_alpabetic_index s/alpabetic/alphabetic/ - in "reset it's cursor values internally" s/it's/its/ - s/synchonously/synchronously/ - s/knowlege/knowledge/ - "gchar *constraints = NULL;" (two spaces) - "location to story any error" s/story/store/ - "%TRUE on Success" - capital 'S' (multiple times) - "addto_vcard_list_cb , &local_results," extra space before comma - s/EbSdbCurorOrigin/EbSdbCursorOrigin/ - s/a %E_CLIENT_ERROR_NOT_SUPPORTED/an %E_CLIENT_ERROR_NOT_SUPPORTED/ - s/atomically commit/atomically committed/ - s/which stores it's contacts/which stores its contacts/ - s/in it's result list/in its result list/ - s/doesnt/doesn't/ - s/in it's place to recalculate/in its place to recalculate/ - s/updating it's current cursor/updating its current cursor/ - s/with the the cursor/with the cursor/ - s/Initial refresh the locale/Initial refresh of the locale/ - s/used by by/used by/ - s/and it's associated resources/and its associated resources/ g) I would rename EBookSortType to EBookCursorSortType. Its name is too generic. h) at book_client_cursor_set_client() you get a reference to current_client, but you unref it only if the client is different, while you claim it cannot change. I suppose you expect the current_client being always NULL and the 'client' being non-NULL, but the code as such seems to indicate a possible leak on a brief reading. What about removing the g_object_unref (current_client); and replace it with g_clear_object (¤t_client); at the end of the function? i) you should use main_context_lock in book_client_cursor_set_context() too j) I believe there should be added some property_mutex-es, to add some thread safety in functions like book_client_cursor_set_locale() (basically all get/set functions), because these can be called any time in any thread (I know, some properties are construct only, where it should be fine, same as you've the functions in "private mutators" section, it only feels like some of the functions can be called in any thread; if you disagree, then feel free to ignore this point). k) in move_by_sync_internal(), I would be paranoid, if the e_contact_new_from_vcard() fails, then do not add it into the GSlist; if there is not set out_contacts, then do not allocate the list and so on, just skip all that, when you free it at the end of the function anyway l) I'm confused from the e_book_client_cursor_move_by() API. I see that the move_by does more than it is named, it moves by and receives contacts. What if I want to move by 10K items, but read only 23 items? The move_by API doc sounds like I can do this only if I call the function twice, once for real move_by(), the other time to move_by & get 23 items. This feels odd. The move_by might be just move_by, then a new get_contacts (...,count, out_contacts,...) function might be added, top not oveload move_by. m) are you sure on this: /* Get the main context before e_async_closure_new () steps on it */ It's in e_book_client_get_cursor_sync() and other functions as well. You create an EAsyncClosure there, which puts its own main context on top of the current (which you wrote a comment about it in the code) and I'm wondering, whether it can cause a deadlock, because the EAsyncClosure prevents delivery of the signal (or basically anything) to the original thread default context, like if you run the sync call in the main thread. n) devel-docs comment at e_book_client_get_locale() - what if I'm not interested in current local sorting, but I want to get consistent order based on my (client side) local, instead of a local of the D-Bus process (book factory) (related to comment #4) o) comment ending with "See discussion in bug 689622", you are going to commit this to upstream, thus does the overall comment make any sense? p) the first summary field is skipped at "create_contacts_table (EBookBackendSqliteDB *ebsdb," "for (i = 1; i < ebsdb->priv->n_summary_fields && success; i++) {" q) in insert_stmt_from_contact() the test 'if (i > 0)' is not correct, if the first column in the summary_fields[] is E_TYPE_CONTACT_ATTR_LIST r) some of the 'return' conditions in e_book_backend_sqlitedb_set_locale are missing UNLOCK_MUTEX (&ebsdb->priv->lock); call s) why is e_book_backend_sqlitedb_get_locale() calling sqlitedb_set_locale_internal()? (doing _set in a _get function looks misleading, not talking about lost GError) t) please, do not use g_assert()/g_critical() - it's too easy to crash all backends (factories) due to incorrect programming in a 3rd party backends/implementations this way u) in e_book_backend_sqlitedb_cursor_move_by(), would it make sense to call 'local_results = g_slist_reverse (local_results);' only after the cursor state update? You can save one walk-though-the-list that way, and if the /* Execute the query */ returns also count of added items, then even two. Basically the reverse is interesting only if you save them to *results, and you are not interested in all returned items, if the results is NULL. You can save couple cycles and memory allocations, if you'll not use addto_vcard_list_cb here, but make the callback smarter. v) I would add some locking around 'cursor' members, just to make sure it's thread-safe to read/write those values. Currently, for example in e_book_backend_sqlitedb_cursor_calculate(), you read its members before obtaining a summary lock. Being this function called from multiplethreads simultaneously, you get an inconsistent result w) why does the e_book_backend_sqlitedb_cursor_calculate() use transactions? It's executing two "SELECT count(*)" statements, where it doesn't look like a correct place for a transaction. x) the e_book_backend_sqlitedb_cursor_compare() sounds like you want to compare two cursors, thus what about name it e_book_backend_sqlitedb_cursor_compare_with_contact() (or even without the "_with"). Its devel doc is missing "Since: 3.12" notation. y) localize the error string(s), for example at e_book_backend_create_cursor() (please check all g_set_error() you added. When talking about errors, please use g_set_error_literal(), when you are not using %X in strings. z) at e_data_book_cursor_sqlite_move_by(), do not pass 'local_results' into e_book_backend_sqlitedb_cursor_move_by() when the 'results' is NULL? Same as not coverting the result when nobody is interested. aa) "sqlitedb_origin = 0" in e_data_book_cursor_sqlite_move_by(), please use the actual enum value, instead of the '0'. ab) the e_data_book_cursor_sqlite_new() is missing s "Since: " notation in the devel doc comment ac) the note "Note that if you need to use the cursor API from EDS you should" at e-data-book-cursor.c doesn't make sense to me (I know what you wanted to tell, but I do not see it there) ad) /* If we ran off the end of the total query, reset to 0 */ Does it make sense, if you try to read more than the current content is, to get to the beginning? I'd say the more expected will be to stay at the end, rather than to jump at the beginning. ae) at e_data_book_cursor_set_sexp(), what if the .sex_exp() fails; are you supposed to call e_data_book_cursor_recalculate() anyway? af) there is no need to typecast g_free()/g_object_unref() to GDestroyNotify ag) counld you add an example of @locale to devel-docs of e_data_book_cursor_set_alphabetic_index() and other similar places, please? Even the @index is not much obvious to write there, is it an 'E' or the index into an array returned from the e_book_client_cursor_get_alphabet() function? When talking about e_book_client_cursor_get_alphabet(), I'm not sure of the @undeflow, @inflow, @oveflow indexes, what is the meaning/expected-values of all the 'gint' arrays there? ah) at e_data_book_cursor_load_locale() you call 'g_clear_object (&cursor);' after !e_data_book_cursor_recalculate. I suppose it's an error, same as using 'error' variable in the call to e_data_book_cursor_recalculate(), instead of '&local_error' ai) at data_book_interpret_locale() you use strncmp(), whic is fine, maybe it doesn't mkae sense to check case insensitively, but the more interesting is, that you do not comapre the result to '== 0', thus you check the locale value against everything but LC_COLLATE and LANG aj) is it intended to call EDBusLocale1 for e-dbus-localed.h/.c? The '1' at the type name looks weird ak) in e_collator_ref()/e_collator_unref(), wouldn't it be safer to use g_atomic_int_inc()/g_atomic_int_dec_and_test() inside the functions? al) I didn't review the cursor-example much, I only see that the .h files use different "#pragma once" notation, like '#ifndef __CURSOR_DATA_H__', which is not it should be. zz) Please fix indentation (coding style) of tests/libedataserver/e-collator-test.c, same as things like "const gchar *locale" (eds code usually do not indent parameter/variable names). Alternatively, Matthew can run his script to normalize your code changes to something close to eds coding style (as I see more places where the coding style doesn't conform). And, of course, change all 3.10 to 3.12 in API docs. I didn't review the tests/ changes much, rather not at all, I only run "make check" and all tests passed properly here, thus I guess it's OK. We can deal with some points later, but some might be fixed before the first commit. If you are not sure, feel free to ping me.
(Just to make it clear, definitely do not commit to master before it's branched for 3.10. I still expect you pinging me before you'll commit to master, with a list of things you changed/corrected before the commit.)
Spotted one memory leak on the factory side (cursor-example is clean): ==3922== 110,592 (672 direct, 109,920 indirect) bytes in 42 blocks are definitely lost in loss record 7,664 of 7,669 ==3922== at 0x4A06409: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==3922== by 0x6F311EA: g_malloc (gmem.c:104) ==3922== by 0x6F49D20: g_slice_alloc (gslice.c:1016) ==3922== by 0x6F4B7D6: g_slist_prepend (gslist.c:267) ==3922== by 0x4C41624: e_data_book_cursor_sqlite_move_by (e-data-book-cursor-sqlite.c:342) ==3922== by 0x4C401B0: e_data_book_cursor_move_by (e-data-book-cursor.c:781) ==3922== by 0x4C3FAB1: data_book_cursor_handle_move_by (e-data-book-cursor.c:510)
(In reply to comment #4) Milan, Thanks for the detailed review ! I have been on vacation for a week so had not replied yet... I'll answer all that I can right now before making changes. > How do I change the locale of an EBookClient? I see e_book_client_get_locale() > (its definition in e-book-client.h suffers of coding style issues: a) spaces > instead of tabs; b) 'const gchar *\t", not 'const gchar *"), but no > _set_locale(), which I'd expect to be available - or at least something like > that on the EBookClientCursor. I tried to run the addressbook factory with > other LANG, even LC_COLLATE, but no luck, the test application still shows > "Cursor example started in locale: en_US.UTF-8". > > Imagine that the backend runs in some system/session locale, but I want to run > any client application in a different local. I would want to get results in > order based on that local. This might be difficult, no doubt, when it comes to > multi-client environment, where the factory has only one EBookBackend for all > D-Bus clients (that's why it seems like the EBookClientCursor property). This is by design, the addressbook itself is set into a locale and reports contacts in the system wide locale. Note that in order to report ordered results efficiently, the sort keys for contact fields must be already stored in SQLite, this way we can navigate the content using only fully indexed columns. Any solution where contacts can be listed in various locale orders would be relatively slow, and probably comparative to just listing all of the existing contacts and then manually sorting them on the client side on top of EDS. To answer your initial question, the locale is set by the 'org.freedesktop.locale1' interface on the system bus. This interface is a part of systemd, which is not exactly standard but seemed to be the most obvious interface to use in order to listen to the system wide locale setting: http://www.freedesktop.org/wiki/Software/systemd/localed (In reply to comment #7) > Review: > a) book_backend_file_set_locale() calls cursors_locale_changed() before the > backend's local private variable is updated, which can cause some "read of old > value" in case any of the cursors will need to get a locale of the backend > during the e_data_book_cursor_load_locale() call. You also do not lock the > priv->cursors variable usage, but the public functions can be called from > lultiple threads, thus you should add some thread safety on that code ( Good points, the same can be said for the priv->locale variable I think. > > b) missing space at '(GDestroyNotify)g_object_unref', but the g_object_unref > already conforms to the GDestroyNotify prototype, thus you can use it without > the cast. > > c) "/* Now that we've modified the contact(s), notify cursors of the changes", > doing modify with remove & add is to flash the UI part, as it can be seen in > evolution. Wouldn't it be better to have only one notify for the modified > contacts? I think this should be fine the way it is. Calling e_data_book_cursor_contact_added/removed() causes the "total" and "position" cursor properties to be updated (quite different than with an EDataBookView). The result here is that the D-Bus property is repeatedly set during a batch of modifications, the D-Bus properties will be pushed in the normal, lazy way (i.e. only once the thread is idle, will the final position/total value be sent to the client). This might cause some inconsistencies in the position/total whilst the batch modification is being made (perhaps a scrollbar might jump unnecessarily), which is not all that bad... even so, it's true that using a sort of e_data_book_cursor_freeze()/thaw() semantic would make that air tight. FWIW, the reason we do this with removed/added is because we use EDataBookCursorClass.compare() to compare the added/removed contact with the current cursor position, this allows us to drive the position and total values without querying SQLite for the full contact count every time something is modified. > > d) book_backend_file_delete_cursor() what if you are asked to delete a cursor > which you do not own anymore? Certainly nothing good, I suppose this should come with a GError to report such a scenario. > e) inside e_book_backend_file_bump_revision() you removed the call to > e_book_backend_notify_property_changed(). How is the client notified about > revision change now? I think you might not be looking at the right copy of the sources for this comment... see: https://git.gnome.org/browse/evolution-data-server/tree/addressbook/backends/file/e-book-backend-file.c?h=cursor-staging#n636 > f) typos (some of them are there multiple times): > - in "changing it's active locale setting" s/it's/its/ > - e_book_client_cursor_get_contact_alpabetic_index s/alpabetic/alphabetic/ > - in "reset it's cursor values internally" s/it's/its/ > - s/synchonously/synchronously/ > - s/knowlege/knowledge/ > - "gchar *constraints = NULL;" (two spaces) > - "location to story any error" s/story/store/ > - "%TRUE on Success" - capital 'S' (multiple times) > - "addto_vcard_list_cb , &local_results," extra space before comma > - s/EbSdbCurorOrigin/EbSdbCursorOrigin/ > - s/a %E_CLIENT_ERROR_NOT_SUPPORTED/an %E_CLIENT_ERROR_NOT_SUPPORTED/ > - s/atomically commit/atomically committed/ > - s/which stores it's contacts/which stores its contacts/ > - s/in it's result list/in its result list/ > - s/doesnt/doesn't/ > - s/in it's place to recalculate/in its place to recalculate/ > - s/updating it's current cursor/updating its current cursor/ > - s/with the the cursor/with the cursor/ > - s/Initial refresh the locale/Initial refresh of the locale/ > - s/used by by/used by/ > - s/and it's associated resources/and its associated resources/ > > g) I would rename EBookSortType to EBookCursorSortType. Its name is too > generic. > > h) at book_client_cursor_set_client() you get a reference to current_client, > but you unref it only if the client is different, while you claim it cannot > change. I suppose you expect the current_client being always NULL and the > 'client' being non-NULL, but the code as such seems to indicate a possible leak > on a brief reading. What about removing the g_object_unref (current_client); > and replace it with g_clear_object (¤t_client); at the end of the > function? Sure, I used isolated mutator functions for consistency, so that I could simply call them at dispose() time, I'll go back and clean that up a bit more. > > i) you should use main_context_lock in book_client_cursor_set_context() too Nod, that bit I copy/pasted from EBookClient... I'll give it more attention. > > j) I believe there should be added some property_mutex-es, to add some thread > safety in functions like book_client_cursor_set_locale() (basically all get/set > functions), because these can be called any time in any thread (I know, some > properties are construct only, where it should be fine, same as you've the > functions in "private mutators" section, it only feels like some of the > functions can be called in any thread; if you disagree, then feel free to > ignore this point). Sure, I'll double check and come back with a more complete explanation, if you read through e-book-client-cursor.c (the sections and comment blocks), I think it's pretty clear that some things are ensured to execute in a specific thread... Specifically the mutators all execute in the main context in which the EBookClientCursor was created, this is also to ensure that the EBookClientCursor only fires g_object_notify() in that thread. > > k) in move_by_sync_internal(), I would be paranoid, if the > e_contact_new_from_vcard() fails, then do not add it into the GSlist; if there > is not set out_contacts, then do not allocate the list and so on, just skip all > that, when you free it at the end of the function anyway > > l) I'm confused from the e_book_client_cursor_move_by() API. I see that the > move_by does more than it is named, it moves by and receives contacts. What if > I want to move by 10K items, but read only 23 items? The move_by API doc sounds > like I can do this only if I call the function twice, once for real move_by(), > the other time to move_by & get 23 items. This feels odd. The move_by might be > just move_by, then a new get_contacts (...,count, out_contacts,...) function > might be added, top not oveload move_by. The ideal API I think (suggested by Patrick before) would have been: e_book_client_cursor_step() With a flags argument including the flags: o REPORT_RESULTS o MOVE_CURSOR_POSITION I.e. stepping through the contacts comes at a cost, it's most desirable to write your code in such a way that you always do both in one step (hence why the original name _move_by(), trying to encourage the user to write code that does both). After experimentation, it's become clear that you also need to move the cursor without fetching contacts sometimes, also sometimes you want to fetch contacts without effecting the current cursor position. > > m) are you sure on this: /* Get the main context before e_async_closure_new () > steps on it */ It's in e_book_client_get_cursor_sync() and other functions as > well. You create an EAsyncClosure there, which puts its own main context on top > of the current (which you wrote a comment about it in the code) and I'm > wondering, whether it can cause a deadlock, because the EAsyncClosure prevents > delivery of the signal (or basically anything) to the original thread default > context, like if you run the sync call in the main thread. I'm sure of this bit yes, it was important to fetch the real main context which the caller used to ask the EBookClient for a cursor, otherwise you will accidentally get the GMainContext which is temporarily created for the duration of e_book_client_get_cursor(). > n) devel-docs comment at e_book_client_get_locale() - what if I'm not > interested in current local sorting, but I want to get consistent order based > on my (client side) local, instead of a local of the D-Bus process (book > factory) (related to comment #4) As in my first comment, an addressbook is always sorted in the order of the locale of the device/system, not a locale of the client's choosing. > o) comment ending with "See discussion in bug 689622", you are going to commit > this to upstream, thus does the overall comment make any sense? Err, certainly I missed that, I'll track it down, it's not like me to leave such comments in source code. > > p) the first summary field is skipped at "create_contacts_table > (EBookBackendSqliteDB *ebsdb," "for (i = 1; i < ebsdb->priv->n_summary_fields > && success; i++) {" Because the first summary field is always the UID... the code does this in several places > > q) in insert_stmt_from_contact() the test 'if (i > 0)' is not correct, if the > first column in the summary_fields[] is E_TYPE_CONTACT_ATTR_LIST As above, the first summary field is always the UID > > r) some of the 'return' conditions in e_book_backend_sqlitedb_set_locale are > missing UNLOCK_MUTEX (&ebsdb->priv->lock); call I'll double check this... > > s) why is e_book_backend_sqlitedb_get_locale() calling > sqlitedb_set_locale_internal()? (doing _set in a _get function looks > misleading, not talking about lost GError) I'll try to clean that up... the reason is because when connected in Direct Read Access the client will load the current locale, and if the current locale has changed, the DRA backend instance needs a new ECollator for sorting purposes. Ok... I'm running out of internet... I will come back and reply the remaining questions a bit later ;-) > > t) please, do not use g_assert()/g_critical() - it's too easy to crash all > backends (factories) due to incorrect programming in a 3rd party > backends/implementations this way > > u) in e_book_backend_sqlitedb_cursor_move_by(), would it make sense to call > 'local_results = g_slist_reverse (local_results);' only after the cursor state > update? You can save one walk-though-the-list that way, and if the /* Execute > the query */ returns also count of added items, then even two. Basically the > reverse is interesting only if you save them to *results, and you are not > interested in all returned items, if the results is NULL. You can save couple > cycles and memory allocations, if you'll not use addto_vcard_list_cb here, but > make the callback smarter. > > v) I would add some locking around 'cursor' members, just to make sure it's > thread-safe to read/write those values. Currently, for example in > e_book_backend_sqlitedb_cursor_calculate(), you read its members before > obtaining a summary lock. Being this function called from multiplethreads > simultaneously, you get an inconsistent result > > w) why does the e_book_backend_sqlitedb_cursor_calculate() use transactions? > It's executing two "SELECT count(*)" statements, where it doesn't look like a > correct place for a transaction. > > x) the e_book_backend_sqlitedb_cursor_compare() sounds like you want to compare > two cursors, thus what about name it > e_book_backend_sqlitedb_cursor_compare_with_contact() (or even without the > "_with"). Its devel doc is missing "Since: 3.12" notation. > > y) localize the error string(s), for example at e_book_backend_create_cursor() > (please check all g_set_error() you added. When talking about errors, please > use g_set_error_literal(), when you are not using %X in strings. > > z) at e_data_book_cursor_sqlite_move_by(), do not pass 'local_results' into > e_book_backend_sqlitedb_cursor_move_by() when the 'results' is NULL? Same as > not coverting the result when nobody is interested. > > aa) "sqlitedb_origin = 0" in e_data_book_cursor_sqlite_move_by(), please use > the actual enum value, instead of the '0'. > > ab) the e_data_book_cursor_sqlite_new() is missing s "Since: " notation in the > devel doc comment > > ac) the note "Note that if you need to use the cursor API from EDS you should" > at e-data-book-cursor.c doesn't make sense to me (I know what you wanted to > tell, but I do not see it there) > > ad) /* If we ran off the end of the total query, reset to 0 */ Does it make > sense, if you try to read more than the current content is, to get to the > beginning? I'd say the more expected will be to stay at the end, rather than to > jump at the beginning. > > ae) at e_data_book_cursor_set_sexp(), what if the .sex_exp() fails; are you > supposed to call e_data_book_cursor_recalculate() anyway? > > af) there is no need to typecast g_free()/g_object_unref() to GDestroyNotify > > ag) counld you add an example of @locale to devel-docs of > e_data_book_cursor_set_alphabetic_index() and other similar places, please? > Even the @index is not much obvious to write there, is it an 'E' or the index > into an array returned from the e_book_client_cursor_get_alphabet() function? > When talking about e_book_client_cursor_get_alphabet(), I'm not sure of the > @undeflow, @inflow, @oveflow indexes, what is the meaning/expected-values of > all the 'gint' arrays there? > > ah) at e_data_book_cursor_load_locale() you call 'g_clear_object (&cursor);' > after !e_data_book_cursor_recalculate. I suppose it's an error, same as using > 'error' variable in the call to e_data_book_cursor_recalculate(), instead of > '&local_error' > > ai) at data_book_interpret_locale() you use strncmp(), whic is fine, maybe it > doesn't mkae sense to check case insensitively, but the more interesting is, > that you do not comapre the result to '== 0', thus you check the locale value > against everything but LC_COLLATE and LANG > > aj) is it intended to call EDBusLocale1 for e-dbus-localed.h/.c? The '1' at the > type name looks weird > > ak) in e_collator_ref()/e_collator_unref(), wouldn't it be safer to use > g_atomic_int_inc()/g_atomic_int_dec_and_test() inside the functions? > > al) I didn't review the cursor-example much, I only see that the .h files use > different "#pragma once" notation, like '#ifndef __CURSOR_DATA_H__', which is > not it should be. > > zz) Please fix indentation (coding style) of > tests/libedataserver/e-collator-test.c, same as things like "const gchar > *locale" (eds code usually do not indent parameter/variable names). > Alternatively, Matthew can run his script to normalize your code changes to > something close to eds coding style (as I see more places where the coding > style doesn't conform). > > And, of course, change all 3.10 to 3.12 in API docs. > > I didn't review the tests/ changes much, rather not at all, I only run "make > check" and all tests passed properly here, thus I guess it's OK. > > We can deal with some points later, but some might be fixed before the first > commit. If you are not sure, feel free to ping me.
Ok lets pick up where I left off... (In reply to comment #7) > Review: [...] > t) please, do not use g_assert()/g_critical() - it's too easy to crash all > backends (factories) due to incorrect programming in a 3rd party > backends/implementations this way Got it... I'll take a look at where I may have added those... > > u) in e_book_backend_sqlitedb_cursor_move_by(), would it make sense to call > 'local_results = g_slist_reverse (local_results);' only after the cursor state > update? You can save one walk-though-the-list that way, and if the /* Execute > the query */ returns also count of added items, then even two. Basically the > reverse is interesting only if you save them to *results, and you are not > interested in all returned items, if the results is NULL. You can save couple > cycles and memory allocations, if you'll not use addto_vcard_list_cb here, but > make the callback smarter. Good point, I don't think this is the bottle neck but I'll try to make that part a little smarter. > > v) I would add some locking around 'cursor' members, just to make sure it's > thread-safe to read/write those values. Currently, for example in > e_book_backend_sqlitedb_cursor_calculate(), you read its members before > obtaining a summary lock. Being this function called from multiplethreads > simultaneously, you get an inconsistent result Alright, I'll work on improving thread safety of cursors, perhaps the cursor itself deserves a separate mutex. > w) why does the e_book_backend_sqlitedb_cursor_calculate() use transactions? > It's executing two "SELECT count(*)" statements, where it doesn't look like a > correct place for a transaction. This is important actually, as specially for Direct Read Access mode we rely on SQLite transaction atomicity, i.e. if the addressbook is modified while calculating the cursor position in DRA mode, we want to be sure to get a consistent "position" and "total" value (i.e. the value before or after the modification, but not in between). > > x) the e_book_backend_sqlitedb_cursor_compare() sounds like you want to compare > two cursors, thus what about name it > e_book_backend_sqlitedb_cursor_compare_with_contact() (or even without the > "_with"). Its devel doc is missing "Since: 3.12" notation. Sure, we can rename that. Also I think I have everything currently marked as Since: 3.10 so I should change all of those... > y) localize the error string(s), for example at e_book_backend_create_cursor() > (please check all g_set_error() you added. When talking about errors, please > use g_set_error_literal(), when you are not using %X in strings. Got it. > > z) at e_data_book_cursor_sqlite_move_by(), do not pass 'local_results' into > e_book_backend_sqlitedb_cursor_move_by() when the 'results' is NULL? Same as > not coverting the result when nobody is interested. Yes this looks tricky, I should at least add a comment there. The reason is that, even if the caller did not want the results, the EDataBookCursor needs to know by how many contacts that the cursor was moved by. This is so that the current "position" property can be calculated after a move_by() without having to scan the whole SQLite again to derive the current position. > > aa) "sqlitedb_origin = 0" in e_data_book_cursor_sqlite_move_by(), please use > the actual enum value, instead of the '0'. Sure. > > ab) the e_data_book_cursor_sqlite_new() is missing s "Since: " notation in the > devel doc comment > > ac) the note "Note that if you need to use the cursor API from EDS you should" > at e-data-book-cursor.c doesn't make sense to me (I know what you wanted to > tell, but I do not see it there) The full text: "Note that if you need to use the cursor API from EDS you should be using the user facing APIs from #EBookClientCursor instead." Should this be rephrased ? How about this: <note><para>EDataBookCursor is an implementation detail for backends who wish to implement cursors. If you need to use the cursor API in Evolution Data Server you should be using #EBookClientCursor instead. </para></note> > > ad) /* If we ran off the end of the total query, reset to 0 */ Does it make > sense, if you try to read more than the current content is, to get to the > beginning? I'd say the more expected will be to stay at the end, rather than to > jump at the beginning. When the query runs off the end of the list we end up with 'NULL' 0 position. The 0 position is not only before all contacts but also after all contacts, depending on how you iterate next (iterating backwards starts at the end, and forwards starts at the beginning). I personally like it this way and not really sure it will make more sense the other way around. I.e. what if you are on the last contact and you move forward by 1 (or by 100), what do you then expect ? that it still stays on the last contact ? or that it moves forward by one and becomes the "0" position ? Remember also that the last contact might not be the last contact twice in a row (because another process might have inserted a contact which ends up after the currently last cursor position). > ae) at e_data_book_cursor_set_sexp(), what if the .sex_exp() fails; are you > supposed to call e_data_book_cursor_recalculate() anyway? My mistake, I'll fix it (it's not harmful the way it is, but better to change it). > af) there is no need to typecast g_free()/g_object_unref() to GDestroyNotify > > ag) counld you add an example of @locale to devel-docs of > e_data_book_cursor_set_alphabetic_index() and other similar places, please? > Even the @index is not much obvious to write there, is it an 'E' or the index > into an array returned from the e_book_client_cursor_get_alphabet() function? > When talking about e_book_client_cursor_get_alphabet(), I'm not sure of the > @undeflow, @inflow, @oveflow indexes, what is the meaning/expected-values of > all the 'gint' arrays there? Those are integer return parameters (not input arrays). Actually I documented this extensively in the user facing EBookClientCursor documentation... please notice that e_book_client_cursor_set_alphabetic_index() refers to a whole section I added on that topic (at the beginning of EBookClientCursor docs). Perhaps it will be enough to just link to that documentation from EDataBookCursor and EBookBackendSqliteDB... > > ah) at e_data_book_cursor_load_locale() you call 'g_clear_object (&cursor);' > after !e_data_book_cursor_recalculate. I suppose it's an error, same as using > 'error' variable in the call to e_data_book_cursor_recalculate(), instead of > '&local_error' Eeeek, indeed. > > ai) at data_book_interpret_locale() you use strncmp(), whic is fine, maybe it > doesn't mkae sense to check case insensitively, but the more interesting is, > that you do not comapre the result to '== 0', thus you check the locale value > against everything but LC_COLLATE and LANG Eeek, thanks. > > aj) is it intended to call EDBusLocale1 for e-dbus-localed.h/.c? The '1' at the > type name looks weird Eh... I'll try to change it... it generated that way because the interface is org.freedesktop.locale1... surely I can force gdbus-codegen to use a nicer name. > > ak) in e_collator_ref()/e_collator_unref(), wouldn't it be safer to use > g_atomic_int_inc()/g_atomic_int_dec_and_test() inside the functions? Yes, I was meaning to do this too but didn't get around to it, thanks for reminding me. > > al) I didn't review the cursor-example much, I only see that the .h files use > different "#pragma once" notation, like '#ifndef __CURSOR_DATA_H__', which is > not it should be. Sure, they are not all that important except to show an example of how to use the cursor... the cursor-data.[ch] is the messiest, most irrelevant part for the reader... I'll make sure to fix the warnings. > zz) Please fix indentation (coding style) of > tests/libedataserver/e-collator-test.c, same as things like "const gchar > *locale" (eds code usually do not indent parameter/variable names). > Alternatively, Matthew can run his script to normalize your code changes to > something close to eds coding style (as I see more places where the coding > style doesn't conform). > > And, of course, change all 3.10 to 3.12 in API docs. > > I didn't review the tests/ changes much, rather not at all, I only run "make > check" and all tests passed properly here, thus I guess it's OK. > > We can deal with some points later, but some might be fixed before the first > commit. If you are not sure, feel free to ping me. Thanks again for your detailed review... I should have another patch in a few days... I'm also putting together a regression test for data migrations from previous SQLite schema versions (the tests arent complete/ready yet, and I found a bug with the new cursor schema upgrading from EDS 3.6 w/BDB).
(In reply to comment #10) > I.e. stepping through the contacts comes at a cost, it's most desirable > to write your code in such a way that you always do both in one step > (hence why the original name _move_by(), trying to encourage the user > to write code that does both). OK, then feel free to keep it this way. > > p) the first summary field is skipped at "create_contacts_table > > (EBookBackendSqliteDB *ebsdb," "for (i = 1; i < ebsdb->priv->n_summary_fields > > && success; i++) {" > > Because the first summary field is always the UID... the code does this in > several places Write a comment about it, please, thus it;s obvious why you skip the first column. > > q) in insert_stmt_from_contact() the test 'if (i > 0)' is not correct, if the > > first column in the summary_fields[] is E_TYPE_CONTACT_ATTR_LIST > > As above, the first summary field is always the UID OK, let's say I define my own summary fields. It means that the summary code will add UID as the first, always, and skip it in my list, if there on other position. Sounds good. What if my second, third and fourth columns are E_TYPE_CONTACT_ATTR_LIST? That might, theoretically, break construction of the SQL statement, no? (I didn't test it, I only think aloud.)
(In reply to comment #11) > Also I think I have everything currently marked as Since: 3.10 so I should > change all of those... True, see the end of comment #7. :) > Yes this looks tricky, I should at least add a comment there. > > The reason is that, even if the caller did not want the results, the > EDataBookCursor needs to know by how many contacts that the cursor was > moved by. My point is that the counting itself doesn't require all that memory copying/allocating. I even think that if you only need one contact, the last, then you can have a GSlice memory for it, and replace it during the walk-through the list. (GSlice to make the allocations quicker.) > <note><para>EDataBookCursor is an implementation detail for backends who wish > to implement cursors. If you need to use the cursor API in Evolution Data > Server you should be using #EBookClientCursor instead. > </para></note> Yes, looks better. The "in Evolution Data Server" sounds a bit inaccurate, shouldn't it be "on a client side"? (where eds is server side). > I.e. what if you are on the last contact and you move forward by 1 (or by 100), > what do you then expect ? that it still stays on the last contact ? or that it > moves forward by one and becomes the "0" position ? I imagine it as FILE stream. Once you read EOF, you stay at EOF (moving forward), not jump to beginning. Maybe I just miss the "position 0" interaction. It's just that it doesn't seem to be able to cover both "before start" and "after end" without addition flag to me. An example: I open a cursor; reading backward it gets before start; then moving forward by 10 reads 10 first records; moving after the end (in forward) keeps me at the end; moving backward by 10 gives me 10 last contacts. If it works this way, then great. It only seemed to me that it doesn't, when I was reading the code. > Perhaps it will be enough to just link to that documentation from > EDataBookCursor and EBookBackendSqliteDB... Yup, please do. > Thanks again for your detailed review... I should have another patch in a few > days... I'm also putting together a regression test for data migrations from > previous SQLite schema versions (the tests arent complete/ready yet, and I > found a bug with the new cursor schema upgrading from EDS 3.6 w/BDB). I also realized that the SQLiteDBSummary is not clever enough. The test book which was created by the cursor-example cannot be opened on current master, claiming "Error introspecting unknown summary field 'full_name_localized'". The code might be more forgiving, I believe, because right now it's like data loss, I cannot open the book at all.
(In reply to comment #13) > (In reply to comment #11) > > Also I think I have everything currently marked as Since: 3.10 so I should > > change all of those... > > True, see the end of comment #7. :) > > > Yes this looks tricky, I should at least add a comment there. > > > > The reason is that, even if the caller did not want the results, the > > EDataBookCursor needs to know by how many contacts that the cursor was > > moved by. > > My point is that the counting itself doesn't require all that memory > copying/allocating. I even think that if you only need one contact, the last, > then you can have a GSlice memory for it, and replace it during the > walk-through the list. (GSlice to make the allocations quicker.) > > > <note><para>EDataBookCursor is an implementation detail for backends who wish > > to implement cursors. If you need to use the cursor API in Evolution Data > > Server you should be using #EBookClientCursor instead. > > </para></note> > > Yes, looks better. The "in Evolution Data Server" sounds a bit inaccurate, > shouldn't it be "on a client side"? (where eds is server side). > > > I.e. what if you are on the last contact and you move forward by 1 (or by 100), > > what do you then expect ? that it still stays on the last contact ? or that it > > moves forward by one and becomes the "0" position ? > > I imagine it as FILE stream. Once you read EOF, you stay at EOF (moving > forward), not jump to beginning. Maybe I just miss the "position 0" > interaction. It's just that it doesn't seem to be able to cover both "before > start" and "after end" without addition flag to me. > > An example: I open a cursor; reading backward it gets before start; then moving > forward by 10 reads 10 first records; moving after the end (in forward) keeps > me at the end; moving backward by 10 gives me 10 last contacts. > > If it works this way, then great. It only seemed to me that it doesn't, when I > was reading the code. Just to reply to this particular comment... Yes, it does work exactly as you describe. In addition to the behaviour you describe... if you reach the end of the contact list (and hit the "0" position as a result), and then you read 10 contacts forward again... then you will get the the first 10 contacts. There are 2 indications that you have reached the end of the list: a.) e_book_client_cursor_move_by() returns less contacts than was requested b.) the "position" property becomes 0 after a call to e_book_client_cursor_move_by() Also, the move_by() documentation specifies that the position value will be synchronously updated, provided that the API is called with the same thread default GMainContext which was default at cursor creation time. > > > Perhaps it will be enough to just link to that documentation from > > EDataBookCursor and EBookBackendSqliteDB... > > Yup, please do. > > > Thanks again for your detailed review... I should have another patch in a few > > days... I'm also putting together a regression test for data migrations from > > previous SQLite schema versions (the tests arent complete/ready yet, and I > > found a bug with the new cursor schema upgrading from EDS 3.6 w/BDB). > > I also realized that the SQLiteDBSummary is not clever enough. The test book > which was created by the cursor-example cannot be opened on current master, > claiming "Error introspecting unknown summary field 'full_name_localized'". The > code might be more forgiving, I believe, because right now it's like data loss, > I cannot open the book at all. And this one... this, if I understand correctly, is very much expected. When we make changes to the SQLite schema, we provide some code which ensures that it can deal with the migration from a previous version. When you run code from the cursor-example branch (which is technically in the future of master), then master has no idea how to handle a database which comes from a future version. Essentially this means that you cannot upgrade from Evolution 3.2 to Evolution 3.6 and then change your mind, you can no longer open your 3.6 compliant addressbook data using 3.2 software (I don't believe this was possible at any point, though).
(In reply to comment #14) > In addition to the behaviour you describe... if you reach the end of the > contact list (and hit the "0" position as a result), and then you read > 10 contacts forward again... then you will get the the first 10 contacts. I do not like it. Once I reach "the current" end, I do not want to get to the beginning, it's highly unexpected. Checking for two options is also suboptimal. The expected is that the cursor API will reject with an error move behind boundaries (in both directions) if there is no more contact to be read, aka it's fine if it reads less than requested contacts, the error will be returned only if it reads none, because it stays at the end already. It's a common practice, consider the FILE streams. > When you run code from the cursor-example branch (which is technically > in the future of master), then master has no idea how to handle a database > which comes from a future version. Right, having forward compatibility is not trivial. The error as such, "unknown column", seemed to me like easy to ignore, with no side-effects, instead of giving up on the summary completely (think of not only forward compatibility, but also about misconfiguration between the summary extension and actual summary database, like when the change in extension wasn't properly propagated into the summary for some reason. Hmm, is the summary extension change-able on the fly, or only before book creation? Maybe I do it overcomplicated, let's call this a side-note, and do not bother much with it).
Update: The cursor staging branch at this point has been updated for most of the (many) details outlined in the comments above. Remaining unaddressed issues: o Some coding style issues, I think it would perhaps be easier to just run the script on the code after committing. o Some comments were made about thread safety regarding EBookClientCursor variables, these are designed in such a way that they should only ever be accessed from the same thread where EBookClientCursor was created in. This approach is not fail safe, however we are still unclear about threading guarantees on the client side, suggest that we don't block on this and instead reconsider our client side threading policies / guarantees. o API issues, regarding the '0' position being simultaneously the beginning and ending of the list; Milan suggests that the list should be linear and not circular, this way avoiding the possibility that an application accidentally read the first contacts after the end of the list. Since there are some things I'd like to change in the API, I think that changing this detail of the '0' position justifies that we rethink and perfect the API one last time before merging this. /* Instead of a RESET origin, we use BEGIN and END origins */ typedef enum { ORIGIN_BEGIN, ORIGIN_END, ORIGIN_CURRENT, ORIGIN_PREVIOUS, /* Do we need to keep PREVIOUS ? */ } EBookCursorOrigin; /* A new argument to e_book_client_cursor_step() */ typedef enum { CURSOR_STEP_MOVE_POSITION = (1 << 0), /* Whether the position should move */ CURSOR_STEP_FETCH_RESULTS = (1 << 1), /* Whether results should be reported */ } EBookCursorStepFlags; /** * e_book_client_cursor_step_sync: * @cursor: A cursor * @origin: Whence to move the cursor from * @flags: Whether to move the cursor or fetch results * @count: The amount of contacts to iterate over, negative values * to iterate backwards through the list * @... out_contacts, cancellable, error * * Steps the cursor through the sorted contact list * * Returns: The number of contacts the cursor has stepped over if successfull, * otherwise -1 is returned and @error is set. */ gint e_book_client_cursor_step_sync (EBookClientCursor *cursor, EBookCursorOrigin origin, EBookCursorStepFlags flags, gint count, GSList **out_contacts, GCancellable *cancellable, GError **error); A note on the ORIGIN_PREVIOUS cursor origin, this was added for convenience in the move_by() API so that one could "refresh" the currently loaded page of contacts in the case the addressbook was modified. With the old API it is impossible to fetch results without moving the cursor position, so we added ORIGIN_PREVIOUS and the implementation now holds 2 cursor states to remember where the cursor last was. I think we should remove the PREVIOUS origin type with the new API, this make it easier to understand the API, and also simplify some code in the backend which now has to maintain 2 cursor states for this purpose. Thoughts ?
(In reply to comment #16) > o API issues, regarding the '0' position being simultaneously > the beginning and ending of the list; Milan suggests that the > list should be linear and not circular, this way avoiding > the possibility that an application accidentally read the > first contacts after the end of the list. I agree that "wrapping around" is unexpected. Please avoid it. > Since there are some things I'd like to change in the API, I think > that changing this detail of the '0' position justifies that we > rethink and perfect the API one last time before merging this. > > /* Instead of a RESET origin, we use BEGIN and END origins */ > typedef enum { > ORIGIN_BEGIN, > ORIGIN_END, > ORIGIN_CURRENT, > ORIGIN_PREVIOUS, /* Do we need to keep PREVIOUS ? */ > } EBookCursorOrigin; > > /* A new argument to e_book_client_cursor_step() */ > typedef enum { > CURSOR_STEP_MOVE_POSITION = (1 << 0), /* Whether the position should move */ > CURSOR_STEP_FETCH_RESULTS = (1 << 1), /* Whether results should be reported > */ > } EBookCursorStepFlags; I still like that better than the implicit moving of the cursor, so +1 from me for this change. > I think we should remove the PREVIOUS origin type with the new API, > this make it easier to understand the API, and also simplify some > code in the backend which now has to maintain 2 cursor states for this > purpose. Agreed.
(In reply to comment #16) > o Some coding style issues, I think it would > perhaps be easier to just run the script on > the code after committing. Sure, make a deal with Matthew on this, I believe he'll even share the script(s) with you, thus you would be able to do the code cleanup before actual commit. > Since there are some things I'd like to change in the API, I think > that changing this detail of the '0' position justifies that we > rethink and perfect the API one last time before merging this. Definitely, the clearer/cleaner the API will be, the better. > /* Instead of a RESET origin, we use BEGIN and END origins */ > typedef enum { > ORIGIN_BEGIN, > ORIGIN_END, > ORIGIN_CURRENT, > ORIGIN_PREVIOUS, /* Do we need to keep PREVIOUS ? */ no comma after the last enum name (in both cases). :) > * @flags: Whether to move the cursor or fetch results The enum seems like you can do one, the other, or both (move _or_ fetch), is it that, or you can do only one or the other? Or does the fetch also imply move? I'd prefer that the fetch doesn't imply move, in which case you can avoid ORIGIN_PREVIOUS, you get it for free. > successfull, one less 'l' in the word. > I think we should remove the PREVIOUS origin type with the new API, > this make it easier to understand the API, and also simplify some > code in the backend which now has to maintain 2 cursor states for this > purpose. I agree (see above, a comment beside the @flags argument). One related question: what if the cursor changes in a way that there are added new contacts before current position (in the sort order of the cursor). Will the re-fetch (supposing it'll not imply also move) read from the same position like before, thus the same contacts, or will you get different contacts on re-read. In numbers, I stay at position 100, reading contacts 100-110. there will be an addressbook update, which adds 10 contacts at position < 90. Either the cursor::position will move to 110, and a fetch will return the same contacts; or the cursor::position will stay at 100, and a fetch will return (in pre-update state) contacts 90-100. You should write somewhere how this will work, to avoid confusion. (If I recall correctly my review, you move the cursor::position based on the updates, which is fine).
(In reply to comment #18) [...] > > * @flags: Whether to move the cursor or fetch results > > The enum seems like you can do one, the other, or both (move _or_ fetch), is it > that, or you can do only one or the other? Or does the fetch also imply move? > I'd prefer that the fetch doesn't imply move, in which case you can avoid > ORIGIN_PREVIOUS, you get it for free. No, fetch does not imply move. You can move the cursor, fetch results, or do both. [...] > One related question: what if the cursor changes in a way that there are added > new contacts before current position (in the sort order of the cursor). Will > the re-fetch (supposing it'll not imply also move) read from the same position > like before, thus the same contacts, or will you get different contacts on > re-read. In numbers, I stay at position 100, reading contacts 100-110. there > will be an addressbook update, which adds 10 contacts at position < 90. Either > the cursor::position will move to 110, and a fetch will return the same > contacts; or the cursor::position will stay at 100, and a fetch will return (in > pre-update state) contacts 90-100. You should write somewhere how this will > work, to avoid confusion. (If I recall correctly my review, you move the > cursor::position based on the updates, which is fine). Yes, I'm surprised that that's not in the docs of EBookClientCursor. I'll make sure to add some text specifically about that. And you remember correctly; the cursor state is defined by contact data and it's position in relation to other contacts. The cursor position "Maurice" will always be after the contact "Alice" and before "Zack", no matter how many contacts are added.
Hi Milan, I was hoping to catch you on IRC earlier this week, but since that's not very easy with our time zones let's try to wrap this up in bugzilla. The API changes discussed above have been performed in the cursor staging branch[0]. I've been refining the API and documentation, the new documentation is (I hope) much more clear... and includes a few diagrams to help visualize how the cursor states and positions work. Regarding the coding style, Matthew says he has a collection of scripts and stuff that he does, but nothing he can easily hand over to me. Do we agree that the only thing blocking this branch now is the coding style issue ? (i.e. mostly just an issue of some lines > 80 chars wide and some spaces instead of tabs or such). If we can agree on this, then I can work this detail out with Matthew. [0]:https://git.gnome.org/browse/evolution-data-server/log/?h=cursor-staging [1]:https://people.gnome.org/~tvb/libebook/EBookClientCursor.html
The 'cursor-staging' branch has landed today in master. See announcement mail: https://mail.gnome.org/archives/evolution-hackers/2013-October/msg00013.html Closing bug.