GNOME Bugzilla – Bug 692951
Detail saving times out if the data sent is the same as the one already in EDS
Last modified: 2013-02-14 17:41:09 UTC
When committing the details to EDS, currently the request is going to fail with a timeout, as this will trigger no property changed signals. EDS Addressbook's modify_contact() returns a boolean value determining the success of the operation, so this can be used instead. The attached patch changes the way success is determined to use the result of modify_contact().
(In reply to comment #0) > The attached patch changes the way success is determined to use the result of > modify_contact(). Did you forget to attach the patch? :-)
Created attachment 234978 [details] [review] patch fixing the problem
(In reply to comment #1) > (In reply to comment #0) > > The attached patch changes the way success is determined to use the result of > > modify_contact(). > > Did you forget to attach the patch? :-) Oups, just did it.
Review of attachment 234978 [details] [review]: The reason the code was written that way is because we require the E.BookClientView.objects_modified signal to be emitted before modify_contact() returns, since modify_contact() is meant to block until all relevant changes are complete in the backend (in this case, EDS). Unless EDS has changed its behaviour to ensure objects_modified is emitted before modify_contact() returns, I don’t think this patch can be applied. See: http://git.gnome.org/browse/folks/commit/backends/eds/lib/edsf-persona-store.vala?id=bc0141fb8317caa614e930cfecbb4cf437797bc2 Instead, we should just make sure that _commit_modified_property() is only ever called on *modified* properties. Which property was causing you problems?
Ok, got your point. Indeed the modify_contact returns before objects_modified is emitted. So, the case I was trying to solve is the following: when using qt-folks, when saving changes to a given contact evolution-addressbook-factory was stopping to respond. After some investigation I saw that qt-folks was setting all the details in one batch operation (calling all the async _set_ functions without even checking their results). So I changed it to save one detail at a time (waiting for the detail saving callback to return before saving the next one), but that was not working very well because of this timeout thing happening for details that were not really changed. Anyway, if this is how E.BookClient works, I'll probably have to follow your suggestion and modify qt-folks to only save details that were really changed. Thanks for the explanation.
(In reply to comment #5) > After some investigation I saw that qt-folks was setting all the details in one > batch operation (calling all the async _set_ functions without even checking > their results). So I changed it to save one detail at a time (waiting for the > detail saving callback to return before saving the next one), but that was not > working very well because of this timeout thing happening for details that were > not really changed. In the async change_*() functions, folks should be checking to see if a property value has changed, and returning immediately if it hasn’t. If it’s not doing that, there’s a bug in folks. Which property was causing the problem?
(In reply to comment #6) > In the async change_*() functions, folks should be checking to see if a > property value has changed, and returning immediately if it hasn’t. If it’s not > doing that, there’s a bug in folks. Which property was causing the problem? I tested it and got the timeout for the following properties: - postal-addresses - notes - phone-numbers - email-addresses - gender
Created attachment 235759 [details] [review] eds: Check property values have changed before committing them to EDS https://www.gitorious.org/folks/folks/commits/692951-eds-property-changes This should fix the problem by adding some missing checks for unchanged property values. It passes all the relevant tests in folks’ EDS test suite.
I can confirm that the problem exists (I discovered it for groups pretty much like Gustavo did) and furthermore, that the patch proposed by Philip solves the issue.
Review of attachment 235759 [details] [review]: Not only does this look fine, it appears to fix at one of our two currently-broken tests (the second fails even less often than normal, but that's probably a fluke). Commit it before something upsets the balance in the universe :)
Comment on attachment 235759 [details] [review] eds: Check property values have changed before committing them to EDS commit fcc7bc4421bf6071061acca427faf0ae4c63c91b Author: Philip Withnall <philip@tecnocode.co.uk> Date: Tue Feb 12 00:39:32 2013 +0000 eds: Check property values have changed before committing them to EDS This prevents timeouts when waiting for EDS to notify us of property changes since it won’t notify for non-changed properties. It also saves a D-Bus roun trip in the case the folks property setter is called with an unchanged value Closes: https://bugzilla.gnome.org/show_bug.cgi?id=692951 NEWS | 2 ++ backends/eds/lib/edsf-persona-store.vala | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+)