GNOME Bugzilla – Bug 439327
gtk_recent_manager_new_...
Last modified: 2007-06-19 11:05:04 UTC
The API for recent mangers is wrong. The base API seems to be gtk_recent_manager_get_for_screen. 1. That does not work for command line programs. 2. A multi-head program ends up with several recent managers. As the documentation states repeatedly, recent mangers are somewhat expensive, so having several managers is bad. And it is just plain silly as the screen isn't used for anything Solution: 1. Have gtk_recent_manager_get_default create a global object the first time it is called and return that. MAybe set up some unref-at-exit magic. 2. Deprecate gtk_recent_manager_get_for_screen and make it call gtk_recent_manager_get_default.
the only reason why GtkRecentManager is attached to the screen is to provide a way to unload it cleanly and without losing data, instead of letting the kernel clean up at the end of the process. I agree, in hindsight, that gtk_recent_manager_get_for_screen() shouldn't have been public API. if you can come up with a patch for the unref-at-exit magic, and without having a global pointer laying around, then I'll gladly review it.
You do realize that gtk_recent_manager_finalize isn't called, right? Attaching to the screen doesn't actually give you that cleanup.
(In reply to comment #2) > You do realize that gtk_recent_manager_finalize isn't called, right? the recent manager is unreffed when the display closes, if it was the screen singleton. see the display_closed() function.
(In reply to comment #1) > the only reason why GtkRecentManager is attached to the screen is to provide a > way to unload it cleanly and without losing data, instead of letting the kernel > clean up at the end of the process. Do you mean applications are losing data now? Because neither finalize() nor display_closed() are called.
(In reply to comment #4) > (In reply to comment #1) > > the only reason why GtkRecentManager is attached to the screen is to provide a > > way to unload it cleanly and without losing data, instead of letting the kernel > > clean up at the end of the process. Morten and I had a discussion on IRC some time ago; I forgot to paste the log here for reference. <gmorten> I seem to recall that displays are not closed. <gmorten> (i.e., deliberately leaked) <mclasen> but you _can_ close them nowadays <mclasen> thanks to mitch <mitch> mclasen: it was painful enough ;) <gmorten> ebassi: but in any case, gtk_quit_add with a main_level of 0 seems to be the way to go. * ebassi would like a GtkContext so that process specific stuff could be added and gutted on gtk_main_quit() <ebassi> private, obviously <gmorten> ebassi: that's more or less what gtk_quit_add does. <gmorten> if I read the code right, that is. * gmorten notes that exiting level==0 is special already. <gmorten> if (gtk_main_loop_level == 0) _gtk_clipboard_store_all (); <gmorten> ebassi: the default manager could be valid until someone exits the top-level main loop. I.e., document behaviour. so, a way to solve this in time for gtk+ 2.12 could be: move the recent manager singleton as a static pointer in gtkrecentmanager.c; add a private function called _gtk_recent_manager_sync_and_close() (or whatever name) that's called when the main loop reaches 0 and that will save, if in dirty state, and will finalize the manager singleton. gtk_recent_manager_set_for_screen() will be deprecated and will just return. applications creating their own recent manager instance with gtk_recent_manager_new() already have to manage it themselves, so they won't be affected by this. > Do you mean applications are losing data now? Because neither finalize() nor > display_closed() are called. no: every save is atomic, so applications are not losing data right now.
Created attachment 89741 [details] [review] [PATCH] Change the recent manager singleton memory management Since relying on the GdkDisplay being closed is not enough to guarantee a correct memory management of the GtkRecentManager singleton instance, the singleton is now a static pointer; gtk_recent_manager_get_default() will return that pointer, without adding any reference on it, while the gtk_recent_manager_get_for_screen() and gtk_recent_manager_set_screen() API are deprecated (the former will call get_default() internally). A private function has been added, which when called will finalize the recent manager singleton. --- gtk/gtk.symbols | 2 + gtk/gtkrecentmanager.c | 130 ++++++++++++++---------------------------------- gtk/gtkrecentmanager.h | 7 ++- 3 files changed, 45 insertions(+), 94 deletions(-)
Created attachment 89742 [details] [review] [PATCH] Remove the recent manager singleton at depth level 0 When the main loop depth is zero, synchronize and remove the recent manager singleton. If the user then restarts the main loop and calls gtk_recent_manager_get_default(), a new singleton will be created, thus no undefined behaviour is present. --- gtk/gtkmain.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)
Created attachment 89743 [details] [review] [PATCH] Hold a weak reference on the recent manager GtkRecentChooserDefault might be embedded inside a dialog, at that dialog can be destroyed after the main loop has finished its run, deallocating the recent manager singleton. With a weak reference on the recent manager we can nullify the pointer that GtkRecentChooserDefault holds and not trigger any warning about null instances. GtkRecentChooserMenu does not need this, because a menu will be destroyed before the recent manager singleton is finalized. --- gtk/gtkrecentchooserdefault.c | 37 +++++++++++++++++++++++++++++++------ 1 files changed, 31 insertions(+), 6 deletions(-)
Created attachment 89747 [details] [review] [PATCH] Add note inside README Notify the behaviour change and the functions deprecation inside the README for gtk+. --- README.in | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
Comment on attachment 89742 [details] [review] [PATCH] Remove the recent manager singleton at depth level 0 Is there any real need to remove the manager though ? If we only synced at depth zero, wouldn't that remove some of the concerns about restarting mainloops and dialogs being destroyed late ?
(In reply to comment #10) > (From update of attachment 89742 [details] [review] [edit]) > Is there any real need to remove the manager though ? > > If we only synced at depth zero, wouldn't that remove some of the concerns > about restarting mainloops and dialogs being destroyed late ? yeah, it should. we would leak the object and would have the kernel to clean it up, but if you don't care it's perfectly fine for me too.
the next three patches are incremental based on attachment 89743 [details] [review]. the first one forces a synchronisation when the main loop level reaches 0 but keeps the recent manager singleton instance around; the second one removes the weak reference held by the GtkRecentChooser default implementation, since the recent manager instances cannot disappear before the recent chooser widgets. the third one is not directly related to this bug but I'd like to get it in, so a review would be great: it bumps the required GLib version to 2.13.5 in order to depend on g_timeout_add_seconds_full() and uses that instead of gdk_threads_add_timeout(), because of the multi-seconds timeout used by the recent manager for polling the storage file.
Created attachment 90233 [details] [review] [PATCH] Synchronise recent manager singleton Force a synchronisation of the GtkRecentManager singleton, if any, instead of just cleaning up (let the kernel pick up the pieces). --- gtk/gtkmain.c | 4 ++-- gtk/gtkrecentmanager.c | 12 ++++++------ gtk/gtkrecentmanager.h | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-)
Created attachment 90234 [details] [review] [PATCH] Remove the weak reference on the manager Since the recent manager singleton is not unreferenced anymore, remove the weak reference on the manager that the default GtkRecentChooser widget holds. --- gtk/gtkrecentchooserdefault.c | 20 -------------------- 1 files changed, 0 insertions(+), 20 deletions(-)
Created attachment 90235 [details] [review] [PATCH] Use approximate timeouts inside the recent files manager GtkRecentManager polls the recent files storage file with a multi-second timeout. This patch switches the timeout source to the approximate timeout source added in GLib 2.13, using g_timeout_add_seconds_full() to make sure that the polling is done with the right granularity and under the GDK lock. The required GLib version is bumped accordingly. --- configure.in | 2 +- gtk/gtkrecentmanager.c | 65 ++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 58 insertions(+), 9 deletions(-)
Looks all good to me; do you want to commit this whole patch sequence ?
yes, I'll commit the singleton management in a single commit and the timeout patch in another.
committed the singleton management in revision 18184 and the timeout in revision 18185