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 439327 - gtk_recent_manager_new_...
gtk_recent_manager_new_...
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Class: GtkRecent
2.10.x
Other All
: Normal enhancement
: Small API
Assigned To: gtk-bugs
Emmanuele Bassi (:ebassi)
Depends on:
Blocks:
 
 
Reported: 2007-05-18 00:24 UTC by Morten Welinder
Modified: 2007-06-19 11:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH] Change the recent manager singleton memory management (7.78 KB, patch)
2007-06-11 12:59 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
[PATCH] Remove the recent manager singleton at depth level 0 (758 bytes, patch)
2007-06-11 12:59 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
[PATCH] Hold a weak reference on the recent manager (2.25 KB, patch)
2007-06-11 12:59 UTC, Emmanuele Bassi (:ebassi)
rejected Details | Review
[PATCH] Add note inside README (736 bytes, patch)
2007-06-11 13:13 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
[PATCH] Synchronise recent manager singleton (1.77 KB, patch)
2007-06-18 20:21 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
[PATCH] Remove the weak reference on the manager (1.35 KB, patch)
2007-06-18 20:21 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
[PATCH] Use approximate timeouts inside the recent files manager (4.01 KB, patch)
2007-06-18 20:21 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review

Description Morten Welinder 2007-05-18 00:24:26 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.
Comment 1 Emmanuele Bassi (:ebassi) 2007-05-18 09:12:42 UTC
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.
Comment 2 Morten Welinder 2007-05-18 15:18:30 UTC
You do realize that gtk_recent_manager_finalize isn't called, right?
Attaching to the screen doesn't actually give you that cleanup.
Comment 3 Emmanuele Bassi (:ebassi) 2007-05-18 15:23:02 UTC
(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.

Comment 4 Yevgen Muntyan 2007-06-10 23:12:03 UTC
(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.
Comment 5 Emmanuele Bassi (:ebassi) 2007-06-11 08:05:31 UTC
(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.
Comment 6 Emmanuele Bassi (:ebassi) 2007-06-11 12:59:41 UTC
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(-)
Comment 7 Emmanuele Bassi (:ebassi) 2007-06-11 12:59:44 UTC
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(-)
Comment 8 Emmanuele Bassi (:ebassi) 2007-06-11 12:59:47 UTC
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(-)
Comment 9 Emmanuele Bassi (:ebassi) 2007-06-11 13:13:54 UTC
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 10 Matthias Clasen 2007-06-11 17:37:23 UTC
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 ?
Comment 11 Emmanuele Bassi (:ebassi) 2007-06-11 19:09:29 UTC
(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.
Comment 12 Emmanuele Bassi (:ebassi) 2007-06-18 20:21:05 UTC
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.
Comment 13 Emmanuele Bassi (:ebassi) 2007-06-18 20:21:20 UTC
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(-)
Comment 14 Emmanuele Bassi (:ebassi) 2007-06-18 20:21:23 UTC
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(-)
Comment 15 Emmanuele Bassi (:ebassi) 2007-06-18 20:21:26 UTC
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(-)
Comment 16 Matthias Clasen 2007-06-19 03:02:37 UTC
Looks all good to me; do you want to commit this whole patch sequence ?
Comment 17 Emmanuele Bassi (:ebassi) 2007-06-19 09:52:40 UTC
yes, I'll commit the singleton management in a single commit and the timeout patch in another.
Comment 18 Emmanuele Bassi (:ebassi) 2007-06-19 11:05:04 UTC
committed the singleton management in revision 18184 and the timeout in revision 18185