Created attachment 193280 [details] [review]
Patch to add e_cal_get_revision() [targetting meego-eds branch]

This patch adds e_cal_get_revision() much like e_book_get_revision()
from bug 652175.

Note this patch is targeting the 'meego-eds' branch and not master
(patch for master still needs to be cooked).

In the local calendar backend in this case, the actual file modification
time is used and the file is saved to disk if needed before generating
the revision timestamp.

On 'openismus-work' as usual you will find an accompanying test case:
   calendar/tests/ecal/test-ecal-get-revision.c

Commit on 'openismus-work' branch is:

commit a780f922ddb4c60b90fab141953d2d3894c42524
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Thu Aug 4 14:18:46 2011 -0400

    Added e_cal_get_revision().
    
    Adds e_cal_get_revision() async/sync api to access an
    opaque revision string which will be NULL for a backend that
    does not implement it. Adds all the needed gdbus machinery and
    implements a revision in the file backend.
    
    This is implemented by checking the calendar file timestamp and
    maintaining a modification counter to ensure there are no races
    when multiple revisions happen in the same second. When the backend
    is asked for it's revision and it's currently dirty... the calendar
    file is forcefully saved to ensure a correct revision string.
    
    Bug: https://bugzilla.gnome.org/show_bug.cgi?id=652177

Created attachment 193289 [details] [review]
Patch to add e_cal_get_revision() [targetting master branch]

And this is the ported version of the patch to master branch.

The test case on 'openismus-work-master' branch is:
  tests/libecal/client/test-client-get-revision.c

and the commit for this patch on 'openismus-work-master' is:

commit ff86182250463f0af07e63b812e3ef1395db62bc
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Thu Aug 4 21:30:35 2011 -0400

Note to reviewer.

There is one thing I'm not entirely sure about, the code is short and
comment inline so I'll just post it here:

static void
e_cal_backend_file_get_revision (ECalBackend *backend, EDataCal *cal, guint32 opid, GCancellable *cancellable)
{
	ECalBackendFile *cbfile = E_CAL_BACKEND_FILE (backend);
	gchar *revision;

	/* Sync the file right now because we need a timestamp to
	 * make a revision
	 */
	if (cbfile->priv->is_dirty) {
		if (cbfile->priv->dirty_idle_id) {
			g_source_remove (cbfile->priv->dirty_idle_id);
			cbfile->priv->dirty_idle_id = 0;
		}
		save_file_when_idle (cbfile);

		/* Do we need to reset 'refresh_skip' here ?? */
		/* priv->refresh_skip = 0; */
	}

	/* Notify with the revision */
	revision = get_revision_string (cbfile);
        ...
}

The above bit which forces the file to be saved 
right away if dirty is copied from 'refresh_thread_func()'
where it sets 'priv->refresh_skip' to 0, it's unclear to
me if that should be done also in this case.

Created attachment 198957 [details] [review]
Patch to add e_cal_get_revision() [targetting master branch, take 2]

Refreshing patches for a huge rebase of 'openismus-work-master' branch off of
the current master branch.

Attaching new patch for master.

New related commits on 'openismus-work-master' branch are:

commit 54c1452c0ff35de3c6990978527176a60c672b1c
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Thu Aug 4 21:32:09 2011 -0400

    Added test case for e_cal_get_revision().....

commit 874827860088477d3add9548aedeaffbd292d2ff
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Thu Aug 4 21:30:35 2011 -0400

    Added e_cal_get_revision()....

Similar comment like in bug #652175, please make it a backend property, without API changes.

For the implementation itself, I do not see the revision counter being saved anywhere, and the 'save' function is not the best one for the counter increment either, because the file is saved on open too. It means the revision string will differ right after opening - it's because check_dup_uid() call.

You can usually use a cache for this, but the file backend doesn't have it, it has everything in an iCal string. Try if you are able to add an X property on a toplevel VCALENDAR property, the one for revision. I suppose a icalcomponent_add_property() will work, for an X property "X-EVOLUTION-REVISION" (get inspired by icomp_x_prop_set(), icomp_x_prop_get() in http://git.gnome.org/browse/evolution-data-server/tree/calendar/backends/caldav/e-cal-backend-caldav.c )

Another issue in later versions of eds can be that the calendar backends would be freed when no longer needed (they are kept alive till the end of a factory process, where addressbook factory frees its backends immediately when no need for them is kept). I believe this difference will be avoided one day, in one or the other way. Who knows which. Just it can make difference in your current patch too.

Created attachment 201909 [details] [review]
Patch to add revision backend property to ECal

Ok here's the good patch.

Removed all the api overhead and implemented as a property (this time
re-added the common property definition to e-client.h).

This one uses g_get_current_time & g_time_val_to_isoSOME_NUMBER_STARTING_WITH_8()
to create the revision string (as requested in the other libebook bug...)

The revision string is stored as an "X property" of the main calendar component
and the revision is 'bumped' any time the calendar is modified.

The new commit on openismus-work-master:

commit 0c4285ba3e97137bef93e7a03d9cb655f5351ea6
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Tue Nov 22 17:58:30 2011 +0900

And the test case has also been updated:

commit bc043ce8c152ca4468cc80ae9fda29dde5f3aa53
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Thu Aug 4 21:32:09 2011 -0400


These are in fact the last 2 commits left on the branch...

Thanks, it looks good and is basically it, only with little issues:
a) change ECAL_REVISION_X_PROP to "X-EVOLUTION-DATA-REVISION", because it's
   the correct name for the X property and because otherwise you can get:
      CREATED;VALUE=X:2011-11-22T14:56:33.727731Z(1)
   instead of the revision property
b) typo in ensure_revision(), there should be ICAL_X_PROPERTY,
   instead of ICAL_X_COMPONENT
c) seeing how many times the bump_revision() is called, and always before
   the save() function, what about adding a gboolean parameter to the save()
   function, saying "do_bump_revision"?
d) I agree with moving CLIENT_BACKEND_PROPERTY_REVISION to e-client.h, but not
   to duplicate it in both e-cal-client.h and e-book-client.h. It's enough to
   have it in e-client.h only (same as the define for capabilities is not
   duplicated between EClient descendants).

And that's it, as you see, only a little polishing.

Created attachment 201980 [details] [review]
Patch to add revision backend property to ECal [take 2]

Same patch with the added polishing requested by Milan.

Consequently the patch touches tests/libebook/client/test-client-get-revision.c
to use the new CLIENT_BACKEND_PROPERTY_REVISION define.

New commit on 'openismus-work-master' branch:

commit 25ddd82dfdfffbfc36ae8eb11ff4711aa29db8bb
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Tue Nov 22 17:58:30 2011 +0900

That's it, thanks. Only replace those two spaces in front of bump_revision (cbfile); with one tab and commit to master.

Review of attachment 201980 [details] [review]:

Eeek I fixed the whitespace problem and pushed this almost a week ago and
forgot to close the bug...

Closing now sorry for the latency...

Committed as:

commit 9597173bbc19a81e53b2e353a7cf634ca2b1e11c
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Tue Nov 22 17:58:30 2011 +0900