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 642692 - Provide wnck_shutdown() (?)
Provide wnck_shutdown() (?)
Status: RESOLVED FIXED
Product: libwnck
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: libwnck maintainers
libwnck maintainers
Depends on:
Blocks:
 
 
Reported: 2011-02-18 17:09 UTC by Vincent Untz
Modified: 2012-01-30 14:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
first patch (8.72 KB, patch)
2012-01-13 18:15 UTC, Martin Pitt
needs-work Details | Review
simple test script (915 bytes, text/plain)
2012-01-18 15:53 UTC, Martin Pitt
  Details
Add wnck_shutdown() [buggy] (11.85 KB, patch)
2012-01-18 16:00 UTC, Martin Pitt
needs-work Details | Review
Add wnck_shutdown() (11.89 KB, patch)
2012-01-18 16:59 UTC, Martin Pitt
committed Details | Review

Description Vincent Untz 2011-02-18 17:09:20 UTC
Quoting bug 344137 comment 5:

Other point: I'm using libwnck to provide a window selection. Shouldn't it be possible to wnck_shutdown() when I'm done?
Comment 1 Martin Pitt 2012-01-13 18:15:23 UTC
Created attachment 205216 [details] [review]
first patch

I primarily need this to avoid unnecessary wakeups for each and every key and focus event, for the extended periods of time when the program does not need to follow state changes.

I did not find a clean and efficient way to do a global wnck_shutdown(), so I added a wnck_screen_stop_update() method which suspends updates. wnck_screen_force_update() then resumes event listening and state updates.

The original effect of eating CPU cycles can be shown with

GI_TYPELIB_PATH=libwnck LD_LIBRARY_PATH=libwnck/.libs/ strace python -c 'from gi.repository import GLib, Gdk, Wnck; s = Wnck.Screen.get_default();  GLib.MainLoop().run()'

This shows read/poll activity with every key press or focus change. Using the new method we can temporarily suspend it:

GI_TYPELIB_PATH=libwnck LD_LIBRARY_PATH=libwnck/.libs/ strace python -c 'from gi.repository import GLib, Gdk, Wnck; s = Wnck.Screen.get_default(); s.force_update(); s.stop_update(); GLib.MainLoop().run()'

-> no wakeups any more. The force_update() is necessary as otherwise the internal state only gets initialized in the main loop, which happens after stop_update(). This is mostly an effect of this rather crude test code.

And to reenable it:

GI_TYPELIB_PATH=libwnck LD_LIBRARY_PATH=libwnck/.libs/ strace python -c 'from gi.repository import GLib, Gdk, Wnck; s = Wnck.Screen.get_default(); s.force_update(); s.stop_update(); s.force_update(); GLib.MainLoop().run()'

-> wakes up on both events again and updates state.

Please let me know if you think this is generally ok, or you'd like a different approach.
Comment 2 Martin Pitt 2012-01-13 18:16:30 UTC
Whoops, there is a remaining printf() debugging leftover in the patch. But it will most likely not be the final version anyway, so might just as well leave it in for now to verify correct masks.
Comment 3 Vincent Untz 2012-01-16 08:13:16 UTC
(In reply to comment #1)
> I did not find a clean and efficient way to do a global wnck_shutdown(), so I
> added a wnck_screen_stop_update() method which suspends updates.

I'm not sure I understand why there's no clean and efficient way to do the global shutdown? Unless you want to keep around references to WnckScreen objects, maybe?
Comment 4 Martin Pitt 2012-01-16 08:40:42 UTC
Well, "I found no way" != "There is no way" :-)

Mostly, a global shutdown would mean to also tear down all WnckScreen data structures and reinit/build them each time you resume. I think that would be much less efficient than just stopping and restarting listening to events.

I actually did try to create such a thing at first, but I didn't succeed in coming up with something that properly cleans up everything and yet was able to reinitialize the state and event handling properly on resuming. That's mostly due to my inexperience with libwnck, and doesn't mean that it's generally impossible. It just seems that the general design of the library is pretty much like "init once, destroy never" (there is not even a public API for destroying screens).

BTW, if you like this more, I can also add a wnck_screen_restart_update() and decouple this from wnck_screen_force_update().

Thanks,

Martin
Comment 5 Vincent Untz 2012-01-16 09:10:30 UTC
(In reply to comment #4)
> Well, "I found no way" != "There is no way" :-)
> 
> Mostly, a global shutdown would mean to also tear down all WnckScreen data
> structures and reinit/build them each time you resume. I think that would be
> much less efficient than just stopping and restarting listening to events.

That's more or less what I had in mind; I don't think it's that less efficient as forcing an update more or less does all the work too (except for memory allocations, of course, but even that part is debatable) since we have to check that all the information we have in cache is still valid.

Also, one thing to be careful about with the current approach is that I'm not sure calling wnck_screen_force_update() will result in the data of each WnckWindow to be updated. A quick test for this is to stop the updates, move a window, make it lose focus, restart the update and see with wnckprop if the window has the right coordinates or the old ones.

> It just seems that the general design of the library is pretty much
> like "init once, destroy never" (there is not even a public API for
> destroying screens).

Yeah, that's the design. All objects are actually owned by the library, not its users. But if we provide a shutdown() API, then we can tell the library "kthxbye" -- it's just a big destroy button the library users can press when they're done with the library, even if it's temporary.

That being said, I do see a point in your approach: if you have a pointer to a WnckWindow, you'd like to keep it valid if the window is still there next time...

So for your approach, I'd just propose two new APIs: wnck_freeze_updates() and wnck_thaw_updates().

I'll comment on some things in the patch too.
Comment 6 Vincent Untz 2012-01-16 09:18:24 UTC
Review of attachment 205216 [details] [review]:

::: libwnck/window.c
@@ +79,3 @@
   Window group_leader;
   Window transient_for;
+  int event_mask;

I don't think we need to keep event_mask around: it's always WNCK_APP_WINDOW_EVENT_MASK or nothing, isn't it?

@@ +520,3 @@
   /* Hash now owns one ref, caller gets none */
 
+  _wnck_window_resume_update (window);

Detail: I would call that "resume" when we start it for the first time :-) Maybe freeze/thaw names would work (is it fine to thaw something the first time?)

@@ +574,3 @@
   g_return_if_fail (wnck_window_get (window->priv->xwindow) == NULL);
 
+  _wnck_window_stop_update (window);

This should not be needed, and might even be wrong: the WnckWindow object is destroyed only when the X window isn't there anymore, so the XSelectInput call will even result in a BadWindow error, I guess.

::: libwnck/xutils.c
@@ +1405,3 @@
+        {
+          /* we delete the mask; save and return the current one */
+          old_mask = attrs.your_event_mask;

old_mask should always be set to the previous value, not just when mask is 0. (Or we can just drop the return value, as it's not needed, see comment in window.c)
Comment 7 Martin Pitt 2012-01-18 13:06:42 UTC
(In reply to comment #6)

> I don't think we need to keep event_mask around: it's always
> WNCK_APP_WINDOW_EVENT_MASK or nothing, isn't it?

I kept it because if the application sets an event mask itself before calling wnck, then we need to restore that properly after resuming wnck updates. That's also why _wnck_select_input() reads the current mask and ORs it to WNCK_APP_WINDOW_EVENT_MASK instead of just setting it (also before my patch).

> @@ +520,3 @@
>    /* Hash now owns one ref, caller gets none */
> 
> +  _wnck_window_resume_update (window);
> 
> Detail: I would call that "resume" when we start it for the first time :-)
> Maybe freeze/thaw names would work (is it fine to thaw something the first
> time?)

_wnck_window_{stop,resume}() sounds fine for me. It's just an internal method anyway, so I'm not fussed at all about namings here.


> +  _wnck_window_stop_update (window);
> 
> This should not be needed, and might even be wrong: the WnckWindow object is
> destroyed only when the X window isn't there anymore

Ah, I was missing that. Will remove.

> ::: libwnck/xutils.c
> @@ +1405,3 @@
> +        {
> +          /* we delete the mask; save and return the current one */
> +          old_mask = attrs.your_event_mask;
> 
> old_mask should always be set to the previous value, not just when mask is 0.
> (Or we can just drop the return value, as it's not needed, see comment in
> window.c)

Alright.

I'll try to extend the patch to provide some global wnck_shutdown() method then which frees all screens and other objects.

Thanks for your comments!
Comment 8 Martin Pitt 2012-01-18 15:53:29 UTC
Created attachment 205534 [details]
simple test script

This is a quick test script which tests the new wnck_shutdown() and ensures that events are being picked up properly before and after, and not in between.

To run in source tree:

  GI_TYPELIB_PATH=libwnck LD_LIBRARY_PATH=libwnck/.libs/ python wncktest.py
Comment 9 Martin Pitt 2012-01-18 16:00:43 UTC
Created attachment 205535 [details] [review]
Add wnck_shutdown() [buggy]

As discussed, this patch now provides a global wnck_shutdown(), which frees all resources, global state, and event listeners. I verified with strace that all wakeups are gone after wnck_shutdown(), and that events are properly caught before shutdown/after reinitialization.

There is one wart, though: At _shutdown time I get a bunch of

Wnck-CRITICAL **: _wnck_window_destroy: assertion `WNCK_IS_WINDOW (window)' failed

(a new assertion that I added to _wnck_window_destroy()). It seems that the _WnckScreenPrivate->mapped_windows list sometimes gets some entries which are not a WnckWindow. I haven't tracked this down properly yet, need to continue on this tomorrow. Attaching this so that you can review the new structure, do you like this more now?

Note that I had to add a new argument to _wnck_select_input(). The previous version always ORed the current Gdk mask to the supplied mask, which is fine for enabling our wnck mask, but doesn't work for disabling it. So it now gets an "update" boolean parameter which causes an OR when TRUE (for enabling), and just sets the supplied mask when FALSE (for disabling).

I also simplified the orig_event_mask handling a bit: we just remember the value on enabling, and write it back (with update==FALSE) on shutdown.

Thanks,

Martin
Comment 10 Martin Pitt 2012-01-18 16:59:54 UTC
Created attachment 205541 [details] [review]
Add wnck_shutdown()

Hah, got it. Silly duplicate destroying of the windows. I also fixed the NULL segfaults in _wnck_application_shutdown_all()/_wnck_class_group_shutdown_all().

Now I got no criticals at all (with G_DEBUG=fatal_criticals) with several wncktest.py runs and some minimal tests:

  GI_TYPELIB_PATH=libwnck LD_LIBRARY_PATH=libwnck/.libs/ python -c 'from gi.repository import Gdk, Wnck; s = Wnck.Screen.get_default(); s.force_update();  Wnck.shutdown()'

GI_TYPELIB_PATH=libwnck LD_LIBRARY_PATH=libwnck/.libs/ python -c 'from gi.repository import Gdk, Wnck; s = Wnck.Screen.get_default(); s.force_update(); print s.get_number(); Wnck.shutdown(); s = Wnck.Screen.get_default(); GLib.MainLoop().run()
Comment 11 Vincent Untz 2012-01-27 17:51:07 UTC
Review of attachment 205541 [details] [review]:

::: libwnck/screen.c
@@ +2770,3 @@
+  for (item = screen->priv->mapped_windows; item != NULL; item = g_list_next (item))
+    _wnck_window_shutdown (WNCK_WINDOW (item->data));
+  wnck_screen_finalize (G_OBJECT (screen));

I've reworked the patch locally, and one thing I changed is not calling wnck_screen_finalize, but just unref'ing the screen object.

The issue with that, though, is that it breaks introspection bindings: the bindings get a ref on the object, and so the shutdown doesn't work.

That's really an issue with the gir, though. Our doc explicitly say that the objects are owned by libwnck and should never be referenced or unreferenced... I'll take a look if we can change the annotations.
Comment 12 Vincent Untz 2012-01-27 22:14:48 UTC
FWIW, after talking with Colin, the conclusion seems to be that we have several options:

 a) do not make wnck_shutdown() introspectable
 b) make it clear in documentation that if you use wnck_shutdown() from introspection, you need to explicitly stop referencing objects before
 c) rewrite part of libwnck

Option c is not going to happen.

I guess option b is relatively reasonable, especially as I don't expect many people to use wnck_shutdown()... Concretely, in your test case above, in means doing "screen = None" before the shutdown.
Comment 13 Martin Pitt 2012-01-28 10:51:58 UTC
>  b) make it clear in documentation that if you use wnck_shutdown() from
> introspection, you need to explicitly stop referencing objects before

This seems very reasonable to me, and better than disabling it from GI completely.

Thanks for the review!
Comment 14 Vincent Untz 2012-01-30 14:41:01 UTC
Comment on attachment 205541 [details] [review]
Add wnck_shutdown()

Thanks!
Comment 15 Vincent Untz 2012-01-30 14:41:34 UTC
I've cleaned things up a bit, and added a bit more docs.