After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 557818 - Code cleanup: Centralize ESourceSelector drag-n-drop handling
Code cleanup: Centralize ESourceSelector drag-n-drop handling
Status: RESOLVED FIXED
Product: evolution-data-server
Classification: Platform
Component: general
2.24.x (obsolete)
Other Linux
: Normal minor
: ---
Assigned To: Evolution Shell Maintainers Team
Evolution QA team
Depends on:
Blocks:
 
 
Reported: 2008-10-24 21:46 UTC by Matthew Barnes
Modified: 2008-11-17 21:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch for Evolution-Data-Server (with soname bump) (8.72 KB, patch)
2008-10-24 21:51 UTC, Matthew Barnes
committed Details | Review
Patch for Evolution (24.09 KB, patch)
2008-10-24 21:53 UTC, Matthew Barnes
committed Details | Review

Description Matthew Barnes 2008-10-24 21:46:34 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.
Comment 1 Matthew Barnes 2008-10-24 21:51:00 UTC
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.
Comment 2 Matthew Barnes 2008-10-24 21:53:25 UTC
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.
Comment 3 Milan Crha 2008-11-07 22:18:10 UTC
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.
Comment 4 Matthew Barnes 2008-11-08 02:46:18 UTC
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.
Comment 5 Matthew Barnes 2008-11-08 02:51:35 UTC
The move behavior you noticed was reported three years ago.  See bug #318003.
Comment 6 Srinivasa Ragavan 2008-11-10 05:04:46 UTC
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.
Comment 7 Matthew Barnes 2008-11-10 12:17:40 UTC
The clock applet in gnome-panel has a call e_passwords_get_password().

I'll post a notice to evolution-hackers.
Comment 8 Srinivasa Ragavan 2008-11-11 02:49:49 UTC
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.
Comment 9 Matthew Barnes 2008-11-11 03:02:05 UTC
Ahh, very sneaky.  I like it!  >:D

Somehow I doubt I'm gonna get much response to my mailing list posting.
Comment 10 Michael Meeks 2008-11-11 12:25:17 UTC
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.
Comment 11 Matthew Barnes 2008-11-11 13:06:24 UTC
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.
Comment 12 Matthew Barnes 2008-11-15 18:40:24 UTC
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.
Comment 13 Srinivasa Ragavan 2008-11-17 03:42:48 UTC
Matt to trunk, go ahead. 
Comment 14 Matthew Barnes 2008-11-17 21:11:13 UTC
Patches committed, along with padding and variable initialization:

Evolution revision 36793, E-D-S revision 9760.