Re: [Evolution-hackers] e_cal_remove_object_with_mod() + empty rid: semantic?

2011-05-17 Thread Patrick Ohly
On Mo, 2011-05-16 at 18:06 +0200, Patrick Ohly wrote:
 I'll do it as uid[\nrid] so that entries without an rid continue to
 look exactly like the current ones.

Looks good. I ran my KCal-EDS test program which adds, modifies and
removes events, including parent and child (= detached recurrence) in
various orders.

I am now seeing all ECalView signals that I expect and need.

I also ran Evolution in parallel to the test and saw it react to all
signals, but not quite as I would have liked.

Evolution seems to interpret uid + empty rid as all recurrences
removed, which IMHO is wrong. It should mean parent removed, because
otherwise there is no way of expressing that change.

For child removed, parent not updated (= no EXDATE added) I would
expect Evolution to show the original, unmodified recurrence again.
Instead it only removes the recurrence (as if EXDATE had been added).

I consider this an Evolution bug which can and should be fixed
separately. Please understand that it is currently lower priority for
me, my main goal has to be to get EDS working as intended in EKCal-EDS,
without Evolution.

Please have a look at the attached patch series. They were tested with
the gnome-2-32 branch. Except for the very last one, all apply to
master. Does this look okay? May I submit to master (after fixing
the last patch), gnome-3-0 and gnome-2-32?

-- 
Bye, Patrick Ohly
--  
patrick.o...@gmx.de
http://www.estamos.de/

From 2e128c63a4a9dd64a3a327262498deb869a119f7 Mon Sep 17 00:00:00 2001
From: Patrick Ohly patrick.o...@intel.com
Date: Wed, 11 May 2011 16:59:51 +0200
Subject: [PATCH 1/8] calendar file backend: support removing parent event with CALOBJ_MOD_THIS

It was possible to create a meeting series with just a detached event
(RECURRENCE-ID set) by importing a meeting invitation for that single
recurrence. It was not possible to arrive at that same state after
adding the parent event (the one with the RRULE) because
e_cal_remove_object_with_mod() removed all instances for
CALOBJ_MOD_THIS and empty rid.

This contradicts the intended semantic of e_cal_remove_object_with_mod():
 By using a combination of the @uid, @rid and @mod
 arguments, you can remove specific instances. If what you want
 is to remove all instances, use e_cal_remove_object instead.

This patch implements the desired semantic:
- e_cal_backend_file_remove_object(CALOBJ_MOD_THIS) now always
  calls remove_instance().
- remove_instance() was extended to also work for the parent
  event.
- That call removes the complete object if nothing is left
  after removing the instance. This case must be handled by
  the caller. The return value is the original object (if
  it still exists) and NULL if not.
- Because the uid pointer into the object may become invalid
  as part of the removal, a more permanent pointer has to
  be provided by the caller.
---
 calendar/backends/file/e-cal-backend-file.c |  134 ++-
 1 files changed, 88 insertions(+), 46 deletions(-)

diff --git a/calendar/backends/file/e-cal-backend-file.c b/calendar/backends/file/e-cal-backend-file.c
index 33bab50..7a8c450 100644
--- a/calendar/backends/file/e-cal-backend-file.c
+++ b/calendar/backends/file/e-cal-backend-file.c
@@ -2629,47 +2629,94 @@ e_cal_backend_file_modify_object (ECalBackendSync *backend, EDataCal *cal, const
 	g_static_rec_mutex_unlock (priv-idle_save_rmutex);
 }
 
-static void
-remove_instance (ECalBackendFile *cbfile, ECalBackendFileObject *obj_data, const gchar *rid)
+/**
+ * Remove one and only one instance. The object may be empty
+ * afterwards, in which case it will be removed completely.
+ *
+ * @uidpointer to UID which must remain valid even if the object gets
+ * removed
+ * @ridNULL, , or non-empty string when manipulating a specific recurrence;
+ * also must remain valid
+ * @return modified object or NULL if it got removed
+ */
+static ECalBackendFileObject *
+remove_instance (ECalBackendFile *cbfile, ECalBackendFileObject *obj_data, const gchar *uid, const gchar *rid)
 {
 	gchar *hash_rid;
 	ECalComponent *comp;
 	struct icaltimetype current;
 
-	if (!rid || !*rid)
-		return;
+	/* only check for non-NULL below, empty string is detected here */
+	if (rid  !*rid)
+		rid = NULL;
 
-	if (g_hash_table_lookup_extended (obj_data-recurrences, rid, (gpointer *)hash_rid, (gpointer *)comp)) {
-		/* remove the component from our data */
+	if (rid) {
+		/* remove recurrence */
+		if (g_hash_table_lookup_extended (obj_data-recurrences, rid,
+		  (gpointer *)hash_rid, (gpointer *)comp)) {
+			/* remove the component from our data */
+			icalcomponent_remove_component (cbfile-priv-icalcomp,
+			e_cal_component_get_icalcomponent (comp));
+			cbfile-priv-comp = g_list_remove (cbfile-priv-comp, comp);
+			obj_data-recurrences_list = g_list_remove (obj_data-recurrences_list, comp);
+			g_hash_table_remove (obj_data-recurrences, rid);
+		} else {
+			/* not an error, only add EXDATE */
+		}
+		/* 

Re: [Evolution-hackers] 32 bit IDs in contact file backend

2011-05-17 Thread Patrick Ohly
On Do, 2011-04-28 at 15:16 +0200, Patrick Ohly wrote:
 Attached the resulting patch. Note that with the patch applied, all new
 contacts in a Berkley DB get the simpler IDs, unconditionally. Older
 contacts continue to use their existing IDs. Would something like this
 be acceptable upstream?

Any thoughts on this?

-- 
Bye, Patrick Ohly
--  
patrick.o...@gmx.de
http://www.estamos.de/


___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


Re: [Evolution-hackers] 32 bit IDs in contact file backend

2011-05-17 Thread David Woodhouse
On Thu, 2011-04-28 at 15:16 +0200, Patrick Ohly wrote:
 As part of wrapping QtContacts around EDS [1] I ran into the same issue
 that Nokia already encountered in their Maemo 5 [2] backend: EDS uses
 strings as ID, QtContacts 32 bit integers.
 
 Nokia solved that by setting up an in-memory hash which maps the UID
 string to its CRC-16. I don't see any checks for the (possible) hash
 collisions. IMHO a proper solution has to keep a permanent mapping on
 disk, otherwise the 32 bit IDs won't be stable.
 
 Overall not a nice solution. My attempt to make it nicer at least in
 combination with the file backend (the main goal for QtContacts-EDS)
 focused on simplifying the problem instead: with 32 bit IDs in the
 storage, the mapping becomes easy. 

While I understand what you're saying when you say 'nicer', this seems
to be a fundamentally *wrong* approach.

You're suggesting that a user of the EDS API should rely on internal
implementation details of a single back end, which don't even apply to
other back ends.

Even if we *didn't* have immediate plans to use other back ends like EWS
with this setup, that would be entirely the wrong thing to do, surely? 

Or is this hack planned to be *extremely* limited for MeeGo 1.2, and
dropped in some way (perhaps by *fixing* the QtContacts API) by 1.3? In
which case, perhaps it really would make more sense to do it with a
persistent mapping in your wrapper?

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation

___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


Re: [Evolution-hackers] 32 bit IDs in contact file backend

2011-05-17 Thread David Woodhouse
On Tue, 2011-05-17 at 14:51 +0200, Patrick Ohly wrote:
 On Di, 2011-05-17 at 13:27 +0100, David Woodhouse wrote:
  On Tue, 2011-05-17 at 14:04 +0200, Patrick Ohly wrote:
   On Di, 2011-05-17 at 12:38 +0100, David Woodhouse wrote:
Even if we *didn't* have immediate plans to use other back ends like EWS
with this setup, that would be entirely the wrong thing to do, surely?
   
   I'm not so sure. We are pitching EDS as an alternative for other storage
   solutions that are highly optimized (= limited!) for specific use cases.
   What you are suggesting is that any attempt to add optimizations for a
   specific combination of app + EDS + backend is wrong and should be
   avoided. My feeling is that EDS will simply not be used at all unless
   such optimization are acceptable.
  
  [EDS upstream]
  
  I have no objection to an *optimisation*. You seemed to be describing a
  *fix*, not an optimisation.
  
  An *optimisation* allows things to work faster or more efficiently, when
  they were already working before.
  
  So if you expose an extra '32bit-numeric-uid' in your static
  capabilities for the back end, and the user can make use of that to
  operate more efficiently by bypassing the permanent uidstring-integer
  mapping, then I'm happy with that.
 
 That was the plan.

In that case I have no objection to the EDS part in principle.

  But *only* if it really is an optimisation, and designed such that
   the code still works (via the mapping) without it.
 
 I can't promise that the code will work without it right away because
 the mapping hasn't been implemented yet due to lack of time. See also:
 http://lists.meego.com/pipermail/meego-dev/2011-May/483078.html

That needs to be fixed; I've responded to that message.

-- 
dwmw2

___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


Re: [Evolution-hackers] 32 bit IDs in contact file backend

2011-05-17 Thread Chenthill
On Tue, 2011-05-17 at 14:51 +0200, Patrick Ohly wrote:
 On Di, 2011-05-17 at 13:27 +0100, David Woodhouse wrote:
  On Tue, 2011-05-17 at 14:04 +0200, Patrick Ohly wrote:
   On Di, 2011-05-17 at 12:38 +0100, David Woodhouse wrote:
Even if we *didn't* have immediate plans to use other back ends like EWS
with this setup, that would be entirely the wrong thing to do, surely?
   
   I'm not so sure. We are pitching EDS as an alternative for other storage
   solutions that are highly optimized (= limited!) for specific use cases.
   What you are suggesting is that any attempt to add optimizations for a
   specific combination of app + EDS + backend is wrong and should be
   avoided. My feeling is that EDS will simply not be used at all unless
   such optimization are acceptable.
  
  [EDS upstream]
  
  I have no objection to an *optimisation*. You seemed to be describing a
  *fix*, not an optimisation.
  
  An *optimisation* allows things to work faster or more efficiently, when
  they were already working before.
  
  So if you expose an extra '32bit-numeric-uid' in your static
  capabilities for the back end, and the user can make use of that to
  operate more efficiently by bypassing the permanent uidstring-integer
  mapping, then I'm happy with that.
 
 That was the plan. From the original email:
 
 |  Further work if you agree in principle:
 |  * let clients query whether all contacts have the simplified ID -
 |could be done with the dynamic capabilities that I mentioned in
 |the e-client API thread; avoids reading all contact (UIDs)
This sounds like a better solution than adding constraints on eds end.
I was for a moment confused on this and david's reply was right on
target :)

One possibility which came to my mind [requires more thoughts] was to
bring the uid generation (e_book_backend_file_create_next_uid) function
into EBookBackend as a virtual function. Then you could have a external
backend for qtcontacts subclassing EBookFileBackend and over-riding the
create_next_uid function alone. 

I haven checked if other backend's would need this virtual function
though. Maybe webdav might use it ?

- Chenthill.
  But *only* if it really is an
  optimisation, and designed such that the code still works (via the
  mapping) without it.
 
 I can't promise that the code will work without it right away because
 the mapping hasn't been implemented yet due to lack of time. See also:
 http://lists.meego.com/pipermail/meego-dev/2011-May/483078.html
 
 



___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


Re: [Evolution-hackers] 32 bit IDs in contact file backend

2011-05-17 Thread Patrick Ohly
On Di, 2011-05-17 at 18:49 +0530, Chenthill wrote:
 On Tue, 2011-05-17 at 14:51 +0200, Patrick Ohly wrote:
  |  Further work if you agree in principle:
  |  * let clients query whether all contacts have the simplified ID -
  |could be done with the dynamic capabilities that I mentioned in
  |the e-client API thread; avoids reading all contact (UIDs)
 This sounds like a better solution than adding constraints on eds end.

I prefer to think of it as an additional promise to the libebook user,
not a constraint.

 One possibility which came to my mind [requires more thoughts] was to
 bring the uid generation (e_book_backend_file_create_next_uid) function
 into EBookBackend as a virtual function. Then you could have a external
 backend for qtcontacts subclassing EBookFileBackend and over-riding the
 create_next_uid function alone. 
 
 I haven checked if other backend's would need this virtual function
 though. Maybe webdav might use it ?

And then what? Build and maintain out-of-tree MeeGo versions of all
backends? Quite frankly, patching the existing backends sounds less
risky to me. Of course, including these changes upstream would be best.

-- 
Bye, Patrick Ohly
--  
patrick.o...@gmx.de
http://www.estamos.de/


___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


Re: [Evolution-hackers] 32 bit IDs in contact file backend

2011-05-17 Thread Milan Crha
On Tue, 2011-05-17 at 15:59 +0200, Patrick Ohly wrote:
  I haven checked if other backend's would need this virtual function
  though. Maybe webdav might use it ?
 
 And then what? Build and maintain out-of-tree MeeGo versions of all
 backends? Quite frankly, patching the existing backends sounds less
 risky to me. Of course, including these changes upstream would be
 best. 

Hi,
I'm not sure if I got it right, but such workarounds are just wrong from
my point of view. You cannot force servers to use certain types of IDs
because of constraints given by application which tries to connect to
it.

Your interface between QtContacts and eds may hide this QtContacts
implementation detail, to be able to support any book backend, current
or future. This might not be done in eds backend itself, from my point
of view.

That's my opinion, at least.
Bye,
Milan

___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


Re: [Evolution-hackers] 32 bit IDs in contact file backend

2011-05-17 Thread Patrick Ohly
On Di, 2011-05-17 at 16:25 +0200, Milan Crha wrote:
 On Tue, 2011-05-17 at 15:59 +0200, Patrick Ohly wrote:
 I'm not sure if I got it right, but such workarounds are just wrong from
 my point of view. You cannot force servers to use certain types of IDs
 because of constraints given by application which tries to connect to
 it.

We can't force a server. But we can adapt the file backend.

 Your interface between QtContacts and eds may hide this QtContacts
 implementation detail, to be able to support any book backend, current
 or future. This might not be done in eds backend itself, from my point
 of view.

might not or must not? 

Anyway, the direction is clear: add the mapping code to QtContacts-EDS,
as soon as time allows. We are juggling resources between various tasks
at the moment, so I'd say as long as we have a solution that works for
the time being, I'd rather see other problems addressed first.

While we do that, the 32 bit patch should remain outside upstream EDS.
Once we have the mapping code, we can revisit that question by doing a
direct comparison with and without it.

-- 
Bye, Patrick Ohly
--  
patrick.o...@gmx.de
http://www.estamos.de/


___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


Re: [Evolution-hackers] 32 bit IDs in contact file backend

2011-05-17 Thread Milan Crha
On Tue, 2011-05-17 at 17:19 +0200, Patrick Ohly wrote:
 On Di, 2011-05-17 at 16:25 +0200, Milan Crha wrote:
  On Tue, 2011-05-17 at 15:59 +0200, Patrick Ohly wrote:
  I'm not sure if I got it right, but such workarounds are just
  wrong from my point of view. You cannot force servers to use
  certain types of IDs because of constraints given by application
  which tries to connect to it.
 
 We can't force a server. But we can adapt the file backend.

That's what I didn't get fully from your conversation with David. What
is the gain to change file backend when there are all other backends
which will be unusable with QtContacts because they cannot do 32-bit int
UIDs? Was it just to have it done natively in the most common
backend and all the rest will use mappings in the QtContacts-EDS
interface? It might be harder to spot bugs in the first or the second
part of the code (with or without intermediate mappings), isn't it?
Maybe I'm just too confused.

  Your interface between QtContacts and eds may hide this QtContacts
  implementation detail, to be able to support any book backend,
  current or future. This might not be done in eds backend itself,
  from my point of view.
 
 might not or must not?

Well, I'm not a maintainer of eds, this is only my personal opinion on
the subject.
Bye,
Milan

___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers