GNOME Bugzilla – Bug 590381
Gtk::RecentInfo::get_application_info() broken beyond repair
Last modified: 2009-08-25 10:57:46 UTC
The method Gtk::RecentInfo::get_application_info() gets the API completely wrong; see the patch for details. There is some other horrible breakage in the file, too. I think the ABI needs to be broken for get_application_info() at least. I don't think it could possibly have been used.
Created attachment 139633 [details] [review] Breakage neutralizer
What's the problem, without me having to read through the patch? Try to be nice.
This is what I get when I try to compile gtkmm-2-14 branch on Ubuntu: recentinfo.cc: In member function »bool Gtk::RecentInfo::get_application_info(const Glib::ustring&, Glib::StringArrayHandle&, guint&, time_t&) const«: recentinfo.cc:192: Fehler: ungültige Umwandlung von »const gchar**« in »gchar**« recentinfo.cc:192: Fehler: Argument 3 von »gboolean gtk_recent_info_get_application_info(GtkRecentInfo*, const gchar*, gchar**, guint*, time_t*)« wird initialisiert Daniel looked at it and told me that the whole file is broken.
Johanns, that compilation error was already fixed in the master and the 2.16 branches: http://git.gnome.org/cgit/gtkmm/commit/?id=3927a139db7529c2368194e6b2684f5b596e957d Feel free to apply it to 2.14 too. It was just caused by a change of constness in GTK+ 2.17/18. I doubt it has anything to do with whatever Daniel is talking about.
(In reply to comment #4) > Johanns, that compilation error was already fixed in the master and the 2.16 > branches: Yes, that made it compile. But it's still broken. > http://git.gnome.org/cgit/gtkmm/commit/?id=3927a139db7529c2368194e6b2684f5b596e957d > Feel free to apply it to 2.14 too. I suggested to Johannes to simply remove the method, which would have the same effect -- it will build but not work. > It was just caused by a change of constness in GTK+ 2.17/18. I doubt it has > anything to do with whatever Daniel is talking about. Well, it has something to do with it insofar as it made me look at the C API documentation for the method in question: http://library.gnome.org/devel/gtk/unstable/GtkRecentManager.html#gtk-recent-info-get-application-info The app_exec parameter is simply a string output argument. Thus, the use of an array handle in the wrapper method is misguided: bool get_application_info(const Glib::ustring& app_name, Glib::StringArrayHandle& app_exec, guint& count, time_t& time) const; The proper prototype would have been something much simpler: bool get_application_info(const Glib::ustring& app_name, std::string& app_exec, guint& count, time_t& time_) const; The implementation was misguided and broken as well, but that doesn't matter much at this point. The problem due to the constness change and the need to add another type conversion for this case should have been a warning sign. A non-const Glib::StringArrayHandle reference would have to be an output argument, which is in fact impossible since the handles do not even implement assignment. Well, the merged C API documentation in our reference documentation appears to have played a role in this breakage, because it is incorrect and has been changed upstream since then. Apart from the API problem, the patch also plugs a number of leaks and fixes other incorrect usages of ArrayHandle.
(In reply to comment #5) > http://library.gnome.org/devel/gtk/unstable/GtkRecentManager.html#gtk-recent-info-get-application-info > > The app_exec parameter is simply a string output argument. Thus, the use of an > array handle in the wrapper method is misguided: Ah, yes. Please make some effort to see if anyone is using this method, and then apply if appropriate.
(In reply to comment #4) > Johanns, that compilation error was already fixed in the master and the 2.16 > branches: > http://git.gnome.org/cgit/gtkmm/commit/?id=3927a139db7529c2368194e6b2684f5b596e957d > Feel free to apply it to 2.14 too. > > It was just caused by a change of constness in GTK+ 2.17/18. I doubt it has > anything to do with whatever Daniel is talking about. > I have applied this patch in gtkmm-2-14 branch but it still does not build with the same error.
Johannes, are you sure that the .h/.cc files were regenerated?
Yes, I made sure they were regenerated. First, I touched the corresponding files and afterwards I tried with a completely clean checkout and still got the problem.
OK, Johannes, I'm not sure what's happening, but I am confident that you can figure out a little constness compiler error.
Committed, with this ChangeLog/commit entry: 2009-08-25 Daniel Elstner <danielk@openismus.com> RecentInfo: Correct get_application_info() so it can actually be used. * gtk/src/recentinfo.[hg|ccg]: get_application_info(): Correct the app_exec parameter to a Glib::ustring& instead of a Glib::StringArrayHandle& app_exec. This method could not have been used before without crashing so this is not a real ABI break. equal(): Reimplement with _WRAP_METHOD() instead of hand-coding. get_applications(), get_length(): Make the implementations more robust by using the length parameter. Bug #590381 We'll need to keep an eye on this. If this stops any existing applications from starting due to unfound symbols, we might need to revert it.
I also commented out the (undocumented) deprecation of operator bool(). Do that separately if necessary, please.