GNOME Bugzilla – Bug 399403
borked Gdk::DragContext::get_targets()
Last modified: 2007-06-10 10:01:37 UTC
Please describe the problem: The following code: Glib::ArrayHandle<Glib::ustring> test() { std::list<Glib::ustring> liste; liste.push_back("test1"), liste.push_back("test2"), liste.push_back("test3"); return liste; } int main(int argc, char *argv[]) { std::list<Glib::ustring> liste=test(); for (std::list<Glib::ustring>::const_iterator it=liste.begin(); it!=liste.end(); it++) std::cout << *it << " "; return 0; } leads to the following output : test1 test2 test1 instead of : test1 test2 test3 It seems that this due to the fact that the object liste in the function test is local to the function. Gtk::DragContext::get_targets() uses similar code and don't run correctly. Steps to reproduce: 1. 2. 3. Actual results: Expected results: Does this happen every time? Other information:
Your test case is broken; it is not valid to return a Glib::ArrayHandle<> that was constructed from data with local scope. It's a wonder that it doesn't simply crash. If as you say Gtk::DragContext::get_targets() does that, it is broken. Therefore I'll reassign the bug to gtkmm. Thanks for your report. Doh. It's called "Handle" for a reason, really.
So I guess we need to allocate copies of the items? A patch would be welcome.
I just had a look at the code, and it is indeed broken. The proper solution would be to define new traits for conversion from GdkAtom to strings, but that would break API/ABI. By the way, GDK_POINTER_TO_ATOM() should rather be used instead of a direct cast. I think we should do either a) Convert each GList element to a string, as in: node->data = g_atom_name(G_POINTER_TO_ATOM(node->data)) and then return a Glib::ListHandle<Glib::ustring> with OWNERSHIP_DEEP or b) Define the appropriate traits struct. Come to think of it, maybe it even exists already. Dunno. Hmm, I just realized that a) break API/ABI too, since it is a list, not an array. That could be hacked around by creating a temporary array instead. However, in fact it doesn't matter if we break this API/ABI since it is broken to begin with. Nobody can possibly be using it at the moment. So I'd vote for b).
Yeah, b) sounds fine. Thanks.
Created attachment 82275 [details] [review] gtkmm_get_targets.patch Could someone test this patch, please?
Aah, nice to see that apparently the traits struct really does exist already. The patch seemed a bit strange at first, since essentially it relies on the conversion static_cast<GdkAtom>(node->data); // node->data has type void* to work, and I had trouble imagining how this could compile at all. But then I figured that GdkAtom is actually a pointer: typedef struct _GdkAtom *GdkAtom; That's okay then. We're stretching the API guarantees of GTK+ a little since it isn't really public, but I think that's acceptable here. So the patch looks fine to me. However: * gtk/src/selectiondata.ccg: Added a commented-out alternative implementation of get_targets(), using ArrayHandle_AtomString. We can not actually use this because it breaks API of a function that apparently does work. No way, Gtk::SelectionData::get_targets() can't possibly work. Unless you count accessing already freed memory as working. The bug will only be apparent if the caller actually accesses the strings.
> static_cast<GdkAtom>(node->data); // node->data has type void* > > to work, and I had trouble imagining how this could compile at all. But then I > figured that GdkAtom is actually a pointer: Presumably that's what's done in C with this GList, so it's not really our problem. If applications do this then we can too. > No way, Gtk::SelectionData::get_targets() can't possibly work. Unless you count > accessing already freed memory as working. The bug will only be apparent if the > caller actually accesses the strings. I'm not convinced. I think you're talking about this code: //Convert the atom to a string: gchar* const atom_name = gdk_atom_name(targets[i]); Glib::ustring target; if(atom_name) target = Glib::ScopedPtr<char>(atom_name).get(); //This frees the gchar*. listTargets.push_back(target); gdk_atom_name() returns a new string, which is then copied into the ustring, and then freed by the ScopedPtr. Only the copy is put in the container and returned from the function. Or does something bad happen after that?
The container is allocated locally, and the ListHandle doesn't copy the elements but only calls c_str() on each. And no, I wouldn't want to change that -- the handles have never been intended to replace containers. Making a shallow copy only is perfectly fine for their use case.
> Presumably that's what's done in C with this GList, so it's not really our > problem. If applications do this then we can too. Applications are supposed to use GDK_POINTER_TO_ATOM() on the list elements.
OK, so I dont' see a way to fix Gtk::SelectionData::get_targets() either without breaking API. It's unpleasant that our traits become part of the API, but I've never liked these intermediate types anyway. I'm tempted to leave this for gtkmm 2.12, just in case it disturbs anything slightly. But 2.12.0 could be six months away.
GTK+ and gtkmm 2.12 will be ready for GNOME 2.20, so I have committed this. I'm not very happy about it, but I guess we will find out if it disturbs people.