Re: [Evolution-hackers] e_cal_remove_object_with_mod() + empty rid: semantic?
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
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
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
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
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
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
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
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
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