GNOME Bugzilla – Bug 652171
EBookClientView: Avoid initial notifications
Last modified: 2013-09-14 16:55:42 UTC
EDS currently sends all data when a view is requested, after e_book_view_start() http://developer.gnome.org/libebook/stable/EBookView.html#e-book-view-start though that is not documented. This is unnecessary when the client is only interested in change notifications. An extension to the EDS query language (EBookQuery) could optionally suppresses that sending of existing data when the view is opened, to be implemented only for the local contacts and calendar backends. This would allow easier integration with the QContact API. For instance, "X-EDS-QUERY-FLAG-SKIP-DATA-DUMP". We plan to provide a patch for this soon, but please let us know if it sounds acceptable.
Ok I've added an e_book_view_set_flags() api along with the underlying gdbus machinery to 'openismus-work' branch. Currently I have it setup with a new EBookViewFlags type and a single field which is E_BOOK_VIEW_NOTIFY_INITIAL. The E_BOOK_VIEW_NOTIFY_INITIAL is ON by default so to skip the initial notifications you need to call e_book_view_set_flags (view, 0) before calling e_book_view_start(). Currently it's only implemented by the local backends/file backend. Also, currently it still sends the "complete" message even if no initial notifications are sent, this is unnecessary but a.) was convenient for my testing and b.) is a decent convention if we have some backends honoring the flag and others not (i.e. whether a backend honors the flag or not, one can assume the "complete" notification will fire). I've added addressbook/tests/ebook/test-ebook-suppress-initial-notifications.c which demonstrates the code in action. Some caveats: o It would be good if EDataBookView sported a "flags" property so that backends could get notified of changes in the flags configuration (right now thats not needed since we set the flag and then start the view). o The api is not very complete, one might consider adding apis like _add_bits(), _remove_bits() and at least _get_flags() (currently the api is write-only). o I didn't modify the xml files in libedata-book/ it seems they are unused but intended for gdbus-codegen...
A link to the openismus-work branch for browsing: http://git.gnome.org/browse/evolution-data-server/log/?h=openismus-work
> I didn't modify the xml files in libedata-book/ it seems they are unused but intended for gdbus-codegen... Yes, no need to edit those, they are no longer used. Is the patch ready are are you still planning changes? Should I merge it on meego-eds already?
I think it's ready the way it is yes (Looks like it's already merged into meego-eds).
Yes, I uploaded this to MeeGo yesterday already and made use of the functionality in QtContacts-EDS. It seems to work fine, Thanks!
(In reply to comment #3) > > I didn't modify the xml files in libedata-book/ it seems they are unused but intended for gdbus-codegen... > Yes, no need to edit those, they are no longer used. If they are really never used then can't they just be removed from git?
This functionality seems to introduce a bug. If I call e_book_view_set_flags (m_ebookview, (EBookViewFlags)0); on the view, then it sometimes occurs that I get a ContactAdded signal instead of a ContactChanged one whenever I update (commit) a contact.
Created attachment 190626 [details] [review] Add EBookClientViewFlags to EBookClientView and counterparts [master] This patch is a retake on the work that went into 'meego-eds' but targeted at EDS master. Also, I've setup another branch which I will regularly rebase against master called 'openismus-work-master' (the other 'openisumus-work' branch is regularly rebased off of 'meego-eds'). Unfortunately I'm still not able to get the test cases to run in master so I can't yet say the attached patch actually works yet. The relevant commit in 'openismus-work-master' is: commit 4400a0f89e955f3a1c595f8eedb278a2669d58db Author: Tristan Van Berkom <tristan.van.berkom@gmail.com> Date: Fri Jun 24 18:25:24 2011 -0400 Added e_book_client_view_set_flags() This commit adds a EBookClientViewFlags to EBookClientView with an initial flag value E_BOOK_CLIENT_VIEW_NOTIFY_INITIAL to control whether the initial contact notifications are sent. The default is to send notifications, if the flag is unset then only future notifications are issued.
I added a test to 'openismus-work-master' for e_book_client_view_set_flags() In fact I was mislead into following the old deprecated code paths in tests/libebook/ but the new EClient api tests are indeed working in tests/libebook/client. So, currently 'openismus-work-master' branch is ready for review and has just 3 commits. The commit mentioned above ended up working for the most part, I needed to make a minor change: commit dc4a418b051674ca24c04128b742b4415a697b12 Author: Tristan Van Berkom <tristan.van.berkom@gmail.com> Date: Fri Jun 24 21:48:27 2011 -0400 Fixed addressbook code run properly without the E_BOOK_CLIENT_VIEW_NOTIFY_INITIAL flag set. It seems we were indeed sending all the initial notifications in a bundle at 'complete' time, this patch adjusts the code to not accumulate the notifications but to only mark the contacts as valid in the id table. And the other commit is an added test case: commit 44d782fe0e294b0ed61862a986cd66e529cdd961 Author: Tristan Van Berkom <tristan.van.berkom@gmail.com> Date: Fri Jun 24 21:50:14 2011 -0400 Added test-client-suppress-notifications.c test Added a test that asserts e_book_client_view_set_flags() apis are working and that the backend bahaves properly when the default E_BOOK_CLIENT_VIEW_NOTIFY_INITIAL flag is unset.
(In reply to comment #4) > I think it's ready the way it is yes > (Looks like it's already merged into meego-eds). When I apply my test-fixing patch to the openismus-work branch, the new test does not succeed, though others do: http://mail.gnome.org/archives/evolution-hackers/2011-June/msg00039.html ** ERROR **: Expected no notifications while loading the view aborting... Aborted FAIL: test-ebook-suppress-initial-notifications I can commit my patch to openismus-work, if you like. I'm porting it to master now too.
(In reply to comment #7) > This functionality seems to introduce a bug. > > If I call > e_book_view_set_flags (m_ebookview, (EBookViewFlags)0); > on the view, then it sometimes occurs that I get a ContactAdded signal instead > of a ContactChanged one whenever I update (commit) a contact. For the record, this is fixed by Chris in meego-eds and openismus-work by this commit: commit a66d6bcc8883fe0203d1117f3d0364244941fb68 Author: Christophe Dumez <christophe.dumez@intel.com> Date: Tue Jun 21 16:26:58 2011 +0300 Fix "avoid initial notifications" functionality The file backend does need to notify the view so that it can update its hash table of ids. The filtering needs to be done in EDataBookView instead, AFTER the hash table is updated. This way, the functionality does not require any change in the backend and works for all backends. Bug report: https://bugzilla.gnome.org/show_bug.cgi?id=652171 I based the equivalent patch on openismus-work-master off of the same fixed code by Chris (however I changed a detail which maybe should go into meego-eds branch also). Chris, could be review this minor change ? In openismus-work-master in e-data-book-view.c:notify_add() function I changed your code to not accumulate the notifications in the send array and really only update the hash table while sending initial notifications (if flagged to disable them). I needed to do this in openismus-work-master in order to avoid all the accumulated contacts to be sent at 'complete' time right before finally sending the 'complete' signal. Also it seems I had not pushed my openismus-work-master tip properly to git... doing that now...
I merged Tristan's fix to meego-eds branch. Thanks for notifying me.
(In reply to comment #12) > I merged Tristan's fix to meego-eds branch. Thanks for notifying me. Thanks I just noticed that (I was in the process of backporting it to openismus-work branch ;-)) (In reply to comment #10) > (In reply to comment #4) > > I think it's ready the way it is yes > > (Looks like it's already merged into meego-eds). > > When I apply my test-fixing patch to the openismus-work branch, the new test > does not succeed, though others do: > http://mail.gnome.org/archives/evolution-hackers/2011-June/msg00039.html > > ** ERROR **: Expected no notifications while loading the view > aborting... > Aborted > FAIL: test-ebook-suppress-initial-notifications > > I can commit my patch to openismus-work, if you like. I'm porting it to master > now too. Murray, yes please do, I just tested the backported patch along with your test case modifications and now the said test passes.
(In reply to comment #13) > Murray, yes please do, I just tested the backported patch along with > your test case modifications and now the said test passes. Done. (I've pushed my tests fix.) But it's still failing for me. What commit should have fixed it in the openismus-work branch?
The commit is: commit 5e4dce7b256326b3d555a4ca90e4b3b78bfdcc63 Author: Tristan Van Berkom <tristan.van.berkom@gmail.com> Date: Tue Jun 28 10:10:15 2011 -0700 It seems we were indeed sending all the initial notifications in a bundle at 'complete' time, this patch adjusts the code to not accumulate the notifications but to only mark the contacts as valid in the id table. Which Chris backported from openismus-work-master into meego-eds, (I just now rebased openismus-work on meego-eds and pushed the new tip so the commit is now on openismus-work branch too).
(In reply to comment #15) > the commit is now on openismus-work branch too). Confirmed. The test passes for me now too. Thanks.
(In reply to comment #8) > Created an attachment (id=190626) [details] [review] > Add EBookClientViewFlags to EBookClientView and counterparts [master] So is this just waiting for review by a maintainer of the master branch, or does it need more work?
(In reply to comment #17) > (In reply to comment #8) > > Created an attachment (id=190626) [details] [review] [details] [review] > > Add EBookClientViewFlags to EBookClientView and counterparts [master] > > So is this just waiting for review by a maintainer of the master branch, or > does it need more work? Yes this patch for master is ready for review. For clarification sake, while writing the patch for master I found a bug, the original patch for master already contains the fix. And the chit-chat on the bug above is about back porting the said fix back to meego-eds branch.
Created attachment 193288 [details] [review] Add EBookClientViewFlags to EBookClientView and counterparts [master, take 2] Actually I found that on the branch there was an initial commit and a fix commit following up. Just to make sure the attached patch is right... I squashed the commits into one legible patch and updating the patch here. The new commit on 'openismus-work-master' is: commit 79d3bfc7c4ffca7cb8bd7eba3ce560efdac2287e Author: Tristan Van Berkom <tristan.van.berkom@gmail.com> Date: Fri Jun 24 18:25:24 2011 -0400 Added e_book_client_view_set_flags() This commit adds a EBookClientViewFlags to EBookClientView with an initial flag value E_BOOK_CLIENT_VIEW_NOTIFY_INITIAL to control whether the initial contact notifications are sent. The default is to send notifications, if the flag is unset then only future notifications are issued.
Created attachment 198954 [details] [review] Add EBookClientViewFlags to EBookClientView and counterparts [master, take 3] Updating patches for huge rebase of 'openismus-work-master' branch off of current master. Attaching new patch for the implementation (the test case is also on the branch as a separate commit). The new relevant commits for this patch are: commit 836b56e8a5f0adcaac24340d906bc850cf844aad Author: Tristan Van Berkom <tristan.van.berkom@gmail.com> Date: Fri Jun 24 21:50:14 2011 -0400 Added test-client-suppress-notifications.c test Added a test that asserts e_book_client_view_set_flags() apis are working and that the backend bahaves properly when the default E_BOOK_CLIENT_VIEW_NOTIFY_INITIAL flag is unset. commit ac969f68158a6903f6d6d2490742976baa1f0d04 Author: Tristan Van Berkom <tristan.van.berkom@gmail.com> Date: Fri Jun 24 18:25:24 2011 -0400 Added e_book_client_view_set_flags() This commit adds a EBookClientViewFlags to EBookClientView with an initial flag value E_BOOK_CLIENT_VIEW_NOTIFY_INITIAL to control whether the initial contact notifications are sent. The default is to send notifications, if the flag is unset then only future notifications are issued.
Created attachment 198960 [details] [review] Add EBookClientViewFlags to EBookClientView and counterparts [master, take 4] Oops, I broke the build with the last patch. This is the fixed version. commit 8403dd5df4df261d780ecf2425e3d6b2ce711898 Author: Tristan Van Berkom <tristan.van.berkom@gmail.com> Date: Fri Jun 24 18:25:24 2011 -0400 Added e_book_client_view_set_flags()
Is this patch acceptable upstream now? This is a nice optimization and we need it for MeeGo/Tizen.
In e_book_client_view_set_flags() do not localize the error string, they were removed from localization recently. Call E_BOOK_CLIENT_VIEW_NOTIFY_INITIAL as E_BOOK_CLIENT_VIEW_FLAGS_NOTIFY_INITIAL and add a new value, E_BOOK_CLIENT_VIEW_FLAGS_NONE too. I would not define E_BOOK_CLIENT_VIEW_DEFAULT_FLAGS. In e-book-client-view.h you are adding unnecessary empty lines, evolution's code is having one empty line per definitions, usually, not two. Make sure you'll make it Since 3.4 there as well. Here: void e_book_client_view_set_flags (EBookClientView *view, EBookClientViewFlags flags, GError **error); might be used tabs, not spaces :) This might not be there: +#include <libebook/e-book-view.h> Why did you reindent those in e_gdbus_book_view_default_init()? Just add there the new and make it align with others, no line breaks are needed there. Note of a new style of function prototype and a parameter-per-line definition in e-gdbus-book-view.c, you "reverted it for e_gdbus_book_view_call_dispose() and made it the old way for the set_flags method. Also make sure you'll update the LIBEDATABOOK_CURRENT and LIBEBOOK_CURRENT in configure.ac. Otherwise looks good, even I didn't compile it yet.
Created attachment 200308 [details] [review] Add EBookClientViewFlags to EBookClientView and counterparts [master, take 5] This patch addresses all of the issues pointed out by Milan. For some of the indentation cases, I put them back how they seem to exactly be but they still seem to generate some small diff (somewhere there is a trailing whitespace or an evil leading tab instead of whitespace or something). For the libtool versioning, I didnt see a note in the configure.ac so I went by what I usually do, I have a comment in some projects (I dont know where it originally came from actually), anyway I think I followed this rule properly (usually I would increment them only before doing a release I think), it might not be right... # If the source code has changed at all, increment PROJECT_REVISION # If any interfaces have been added, removed, or changed, increment PROJECT_CURRENT, and set PROJECT_REVISION to 0. # If any interfaces have been added since the last public release, then increment PROJECT_AGE. # If any interfaces have been removed since the last public release, then set PROJECT_AGE to 0. # Reference: http://www.gnu.org/software/libtool/manual.html#Versioning
(In reply to comment #24) > This patch addresses all of the issues pointed out by Milan. Hmm, I do not see FALGS rename, neither the FLAGS_NONE addition in the above patch. Neither "Since: 3.2" had been removed. Are you sure you attached correct patch, please? > For some of the indentation cases, I put them back how they seem > to exactly be but they still seem to generate some small diff (somewhere > there is a trailing whitespace or an evil leading tab instead of whitespace > or something). It's because you are using spaces, where there were used tabulators. They are preferred in evolution sources, I think. > For the libtool versioning, I didnt see a note in the configure.ac so > I went by what I usually do, I have a comment in some projects (I dont > know where it originally came from actually), anyway I think I followed > this rule properly (usually I would increment them only before doing > a release I think), it might not be right... I agree, the increase should be done not more than once per release, thus once it is increased, one may not repeat the increment (I asked for a version increment in bug #652178 too). Thinking of it, it changes DBus API too, thus I forgot of ADDRESS_BOOK_DBUS_SERVICE_NAME, please increase the tailed number as well.
Created attachment 200463 [details] [review] Add EBookClientViewFlags to EBookClientView and counterparts [master, take 6] Ok I can see I was in a hurry and I missed adding Since 3.4 in some additional places and completely overlooked that you wanted the flags to be renamed. Sorry for that. I tried again to get rid of the whitespace changes, by copying the diff output from what was there before into the new source... but still no luck, there remains this whitespace diff: - E_INIT_GDBUS_METHOD_VOID (EGdbusBookViewIface, "start", start, __START_METHOD) - E_INIT_GDBUS_METHOD_VOID (EGdbusBookViewIface, "stop", stop, __STOP_METHOD) - E_INIT_GDBUS_METHOD_VOID (EGdbusBookViewIface, "dispose", dispose, __DISPOSE_METHOD) + E_INIT_GDBUS_METHOD_VOID (EGdbusBookViewIface, "start", start, __START_METHOD) + E_INIT_GDBUS_METHOD_VOID (EGdbusBookViewIface, "stop", stop, __STOP_METHOD) + E_INIT_GDBUS_METHOD_VOID (EGdbusBookViewIface, "dispose", dispose, __DISPOSE_METHOD) + E_INIT_GDBUS_METHOD_UINT (EGdbusBookViewIface, "set_flags", set_flags, __SET_FLAGS_METHOD) Regarding versioning numbers, as I incremented the LIBEBOOK_CURRENT in bug 652178 as you suggested so this patch now has an increment to LIBEDATABOOK_CURRENT and the GDBus api tail number is pushed to 3.
(In reply to comment #26) > Created an attachment (id=200463) [details] [review] > Add EBookClientViewFlags to EBookClientView and counterparts [master, take 6] > > Ok I can see I was in a hurry and I missed adding Since 3.4 in some > additional places and completely overlooked that you wanted the flags > to be renamed. > > Sorry for that. > > I tried again to get rid of the whitespace changes, by copying the diff output > from what was there before into the new source... but still no luck, there > remains this whitespace diff: Maybe something with your editor, as it has spaces instead of tab in the middle of the line. > Regarding versioning numbers, as I incremented the LIBEBOOK_CURRENT > in bug 652178 as you suggested so this patch now has an increment > to LIBEDATABOOK_CURRENT and the GDBus api tail number is pushed to 3. Correct, thanks for it. By the way, the commit you did is dated 2011-07-23, which is kinda misleading (using gitk) and 2011-07-22 using (git log). Neither is near the current time for some reason (I prefer git pull && patch -p1 <... && make && make install && test-it && git add -u && git commit && git push to be sure everything was applied as expected - and it writes correct commit times as well). ----------------------------------------------------------------------------- Patch looks good, just make the commit message: Bug #652171 - Add e_book_client_view_set_flags() as the bug number is usually useful, if the change was discussed on any bug.
Tristan, please push this.
Pushed.