GNOME Bugzilla – Bug 557818
Code cleanup: Centralize ESourceSelector drag-n-drop handling
Last modified: 2008-11-17 21:11:13 UTC
I noticed while working on my "kill-bonobo" branch that the drag-n-drop logic for ESourceSelector (the ones used in the sidebar) is repeated in no less than four different places in Evolution. The logic for handling a valid drop is unique for each case, but the logic for determining whether a drop is valid is exactly the same in all four cases. That latter part can be centralized into ESourceSelector itself. Applications would still have to tag the instance as a drag destination and supply a drag target list. But as for the drop logic, applications would only have to respond to a valid drop event by listening for a "data-dropped" signal from ESourceSelector. To implement this, I've added a method to ESourceSelectorClass: gboolean (*data_dropped) (ESourceSelector *selector, GtkSelectionData *data, ESource *destination, GdkDragAction action, guint target_info); This breaks the ABI of libedataserverui (the size of ESourceSelectorClass increases), but only gnome-panel and Evolution would be affected.
Created attachment 121311 [details] [review] Patch for Evolution-Data-Server (with soname bump) ESourceSelector handles "drag-motion", "drag-drop", "drag-data-received" and "drag-leave" events itself, and emits a "data-dropped" signal when it detects a valid drop. The signal carries all the necessary information for applications to process the drop event.
Created attachment 121313 [details] [review] Patch for Evolution A bunch of repetitive drag-n-drop logic falls out since we only have to handle "data-dropped" signals from ESourceSelector.
In eds patch, potentional call of 'g_object_unref' on uninitialized variable 'object' in 'source_selector_drag_motion'. Also there, I think 'action = 0' means something different than 'action = { 0 }', but I'm not so good in these little tinnies. Err, something doesn't work, when I try to move event from one local calendar to the other one, in a day view, then the item is copied, not moved.
Good catch on the uninitialized variable. I'll initialize it to NULL before committing the patch. The GdkDragAction variable is just an enumeration, so initializing it to zero is valid. As for moving calendar events, I see this behavior too but I also see it in Evolution 2.12 (on my Debian machine at home). So it's not a regression caused by this patch. If you compare the selector_tree_data_dropped() function in calendar-component.c to, say, tasks-component.c, you'll notice the move logic for calendar events is not implemented. Fixing that might be as simple as copying from Tasks, but let's handle that in a separate bug and have one of the Calendar guys review it. Sound good? Srini: Are you okay with an ABI break in libedataserverui for this? As I mentioned above, it would only impact Evolution and GNOME Panel.
The move behavior you noticed was reported three years ago. See bug #318003.
Matt, How GNOME Panel? I thought libecal/libedataserver is what Panel lives on. Also, if we have to do it, I would like to move it as a discussion on e-h before we do it. Atleast, this is what I promised when we broke last time.
The clock applet in gnome-panel has a call e_passwords_get_password(). I'll post a notice to evolution-hackers.
I was just wondering, if we don't bump the SO version, assuming that nothing apart from e-passwords gets used from libedataserverui externally (NON-EVO) and just go with it. Because, Panel would still work well, post commit, even if the version isnt bumped. We can let it assume that only e-passwords is the supported/public ABI/API thingy in libedataserverui. That way Panel would be happy and just refix evo/exchange. It might be crude, or ugly, but just a thought, to avoid unnecessary so bump to panel, which really doesn't need it.
Ahh, very sneaky. I like it! >:D Somehow I doubt I'm gonna get much response to my mailing list posting.
I read the patch; I love the code saving, and the bonobo removal :-) but I hate the ABI breakage :-( I like Srini's solution. I really dislike the way that when the last ABI flag-day happened we (apparently) failed to add: gpointer unused_slots[2]; or whatever to the end of each of these Classes - prolly worth adding a couple there now: the cost is negligable. Of course - for the paranoid I guess there are number of other possible solutions: including adding leaving the old cruft there, and adding a method to turn on the new signals instead of old, and adding the new signal without a slot in the vtable (AFAIR possible if not recommended) - since we have no 'default' version of the signal, that sounds like another option - if Srini's is not good enough :-) Thanks for the CC.
Okay, note to self to fix the uninitialized variable Milan pointed out in comment #3 and add some padding to the end of ESourceSelectorClass before committing. I did a reverse dependency search on Debian to see how widespread the effects would _really_ be. It came up with a couple more hits than Fedora: $ apt-cache rdepends libedataserverui1.2-8 libedataserverui1.2-dev lunar-applet libedataserverui1.2-dev gnome-panel evolution-plugins-experimental evolution-plugins evolution-jescs evolution-exchange evolution lunar-applet, like gnome-panel, only calls e_passwords_get_password(). evolution-jescs uses more of the password API, but that's it. That pretty much quells any paranoia I had. The breakage is kept entirely in the family, and we have versioned dependency tracking in the configure scripts now to avoid any problems for Evolution.
Any final concerns before I commit this? I think we've covered all the bases: - We will _not_ change the libedataserverui soname. - Evolution is the only known package that's affected, and we'll handle that by bumping the E-D-S requirement to 2.25.2. - ESourceSelectorClass will be padded to allow for future expansion.
Matt to trunk, go ahead.
Patches committed, along with padding and variable initialization: Evolution revision 36793, E-D-S revision 9760.