GNOME Bugzilla – Bug 164474
migrate the window selector applet code into libwnck
Last modified: 2007-05-18 13:50:32 UTC
Bug #156917 was opened to make the "window selector" applet from gnome-panel use the window-menu code present in libwnck. This is to make the applet consistent with the window list applet, that uses the tasklist code from libwnck. This bug is to track the changes in libwnck required for migrating the applet.
Created attachment 36185 [details] [review] patch to move the widget code to libwnck Here is the patch to move all the widget code from the panel's window-selector applet to libwnck. I tried to follow the window list exemple : The window list applet uses the "tasklist" widget from libwnck. So with this patch the window selector applet would use the "selector" widget from libwnck. Thus I renamed libwnck's "WnckWindowMenu" to "WnckSelector". So I renamed the file accordingly (we don't lose anything as the file window-menu.c in libwnck was never used anywhere).
Note also that in the current state of things, the window selector applet does not use code from libwnck, and the window-menu code in libwnck is not used anywhere. So this patch alone does not change anything in the way the desktop works. But it is required to apply the patch in bug #156917.
Comment on attachment 36185 [details] [review] patch to move the widget code to libwnck It looks like the selector.[ch] copyright is wrong, since I didn't write the code in gnome-panel. Formatting here is weird, some of the items: struct braces should be in column 0 GObject *object vs. GObject * object egtk-format-protos on the static forward decls. I'm not really reading selector.c beyond formatting stuff since I assume it's at least as good as whatever was in the panel before. It might be nice to use the new GObject private data field instead of having all that stuff in the WnckSelector struct directly. Anyhow, if you clean up the formatting to be libwnck-consistent it's fine to commit and someone can fix any other stuff later.
Created attachment 36451 [details] [review] new patch This new patch corrects the copyright, fixes indentation problems and uses the g_type_class_add_private stuff. About egtk-format-protos : I've been looking at gtk stuff for a long time now, and I've never even heard of egtk.el anywhere before... It wouldn't hurt to put this file somewhere in a prominent place like d.g.o ;-) Ok to commit ?
I didn't see in your last comment that you said it was fine to commit. I will commit ASAP. Thanks !
Just a question : should the files window-menu.c and window-menu.h be removed from libwnck ? They serve no purpose now and were never updated after their initial commit, so we wouldn't lose anything.
there's a typo on line 58 of selector.c static void wnck_selector_init (WnckSelector *tasklist); the line should be static void wnck_selector_init (WnckSelector *selector); It's not really important though.
Vincent: I find it weird that we have to create the selector with a WnckScreen argument, but that there's no way to update this screen. Maybe you should add a _set_screen() function? Or remove the WnckScreen argument...
Vincent : yeah, it bothers me too. The only reason we pass a WnckScreen argument is for the first time the menu is filled up, during the widget creation. The widget needs to know the screen it is on, to get the list of windows for that screen. At that point, the widget is not realized yet, and calling gtk_widget_get_screen does not work. That's the only time we use the screen passed by the caller, from then on we just call gtk_widget_get_screen on the widget. So priv->screen is actually only used once. Do you have any smart insight on how to get rid of the thing altogether ?
Created attachment 36552 [details] [review] tentative patch This patch at least gets rid of the priv->screen element, but the WnckScreen argument is still there. I know it's weird, but it seems like the whole wnck_selector_setup_menu function is not really useful :) Connecting to the actual screen is enough to make the widget do the right thing.
Created attachment 36553 [details] [review] better patch that removes the argument This patch may do the trick. It connects a function to the realize signal that connects the widget to the right screen. The function is disconnected on its first call. Thus there's no need for a WnckScreen argument anymore. Is there a more elegant way to disconnect the signal than keep the handler around in a global variable ?
Moving to the right component. Sorry for the spam.
Comment on attachment 36553 [details] [review] better patch that removes the argument the handler ID should go in the selector object, or in g_object_set_data() object data on it. Generally speaking anything you do in realize should be undone in unrealize; i.e. you should be able to realize a widget on screen 1, unrealize it, then realize it again on screen 2. The patch looks good in overall concept.
Comment on attachment 36451 [details] [review] new patch I believe this was committed, right? Marking it as such to get it off the patch diligence report...
The patch in attachment 36451 [details] [review] was committed, but not the subsequent changes... I didn't have time to work on the patch to include Havoc's comments, sorry :(
Created attachment 47108 [details] [review] new patch that removes the argument and the global variable Here is an updated version of the patch, that includes Havoc's comments (in comment #13). The handler ID is now in the selector object, and the signal is reconnected on unrealize.
Comment on attachment 47108 [details] [review] new patch that removes the argument and the global variable Looks plausible to me if you guys think it's right. I didn't review that carefully.
- Remove wnck_selector_get_screen() altogether. Its a GtkWidget, so users should just use gtk_widget_get_screen() - Hook up to the realize() and unrealize() handlers in class_init() (chaining up to the parent) and connect/disconnect to/from the screen there. - No reason to add the #include <glib.h>
Mark : - wnck_selector_get_screen returns a WnckScreen (not a GdkScreen). I may be wrong, but I thought it's not the same thing... - if I hook up the realize/unrealize handlers in class_init, how do I get the signal handler ? When you say "connect to/from the screen there", what do you mean by "there" ? (sorry if I'm a little dense here, I'm not used to meddling with widgets like that :P)
I fixed this in trunk.