GNOME Bugzilla – Bug 561297
Set a strut properly to prevent windows under the panel
Last modified: 2008-11-19 23:06:14 UTC
This set of patches for gnome-shell and metacity-clutter sets a strut so that windows don't appear under the panel. Review would be appreciated... it's actually quite straightforward (ignoring the very large set of gobject-introspection and gjs changes for struct support these changes depends upon...)
Created attachment 122902 [details] [review] Add meta_workspace_set_builtin_struts() Add a function to set a list of "built-in" struts for a workspace; these are struts that are combined with the struts of the windows in the workspace when determining the work area of the workspace. https://bugzilla.gnome.org/show_bug.cgi?id=561297
Created attachment 122903 [details] [review] Make MetaScreen and MetaWorkspace GObjects src/core/screen.c src/core/screen-private.h src/include/screen.h: Make MetaScreen a GObject. src/core/workspace.c src/core/workspace-private.h src/include/workspace.h: Make MetaWorkspace a GObject. Rename meta_workspace_free() to meta_workspace_remove(). https://bugzilla.gnome.org/show_bug.cgi?id=561297
Created attachment 122904 [details] [review] Fix return transfer annotations for a number of functions and add docs Add return transfer annotations (and while I was at it, docs), for public functions that return MetaDisplay, MetaWindow, and MetaScreen. https://bugzilla.gnome.org/show_bug.cgi?id=561297
Created attachment 122905 [details] [review] Add a 'n-workspaces' property to MetaScreen Add a 'n-workspaces' property to MetaScreen to allow for notification when the number of workspaces changes. https://bugzilla.gnome.org/show_bug.cgi?id=561297
Created attachment 122906 [details] [review] Patch for gnome-shell to add the strut This patch goes ahead and actually sets the struts for all workspaces to include the panel. It also adds a 'screen' property on ShellGlobal so that the javascript code can get access to the MetaScreen.
Please ignore the stray launcher.py change in the last patch.
Owen asked me to look at this, although I don't really know the metacity/mutter code that well... (In reply to comment #1) > Created an attachment (id=122902) [edit] > Add meta_workspace_set_builtin_struts() No real issues. Should there be a _get_builtin_struts() too? (In reply to comment #2) > Created an attachment (id=122903) [edit] > Make MetaScreen and MetaWorkspace GObjects The meta_screen_free() thing seems a little scary, since this means it's possible that references to the screen will remain after all of its data has already been freed. (Especially in a GCed environment.) I can't think of any concrete examples of problems that wouldn't already be there with the current non-refcounted MetaScreen though, so I guess this probably isn't an issue. (Likewise with meta_workspace_remove().) > /* This also removes the workspace from the screens list */ > - meta_workspace_free (workspace); > + meta_workspace_remove (workspace); This comment is now dumb. (In reply to comment #3) > Created an attachment (id=122904) [edit] > Fix return transfer annotations for a number of functions and add docs Hooray docs. (In reply to comment #4) > Created an attachment (id=122905) [edit] > Add a 'n-workspaces' property to MetaScreen This all looks right. (In reply to comment #5) > Created an attachment (id=122906) [edit] > Patch for gnome-shell to add the strut > + let me = this; I already had a "let panel = this;" earlier in Panel._init, for the new-tray-icon signal handler. So (a) you could just use that variable instead, and (b) this looks like it's going to be a recurring problem that we should have some cleverer way to deal with (or at least a standard pattern)... Otherwise looks good.
(In reply to comment #7) > (In reply to comment #2) > > Created an attachment (id=122903) [edit] > > Make MetaScreen and MetaWorkspace GObjects > > The meta_screen_free() thing seems a little scary, since this means it's > possible that references to the screen will remain after all of its data has > already been freed. (Especially in a GCed environment.) I can't think of any > concrete examples of problems that wouldn't already be there with the current > non-refcounted MetaScreen though, so I guess this probably isn't an issue. > (Likewise with meta_workspace_remove().) I originally started with a more extensive modification to e.g. split up workspace_remove() and workspace_finalize() and leave the workspace in a consistent state after remove() - no dangling pointers to freed memory, etc, but then I decided that that that was going to create a lot of merge problems and obscure the real motivation behind the change. There's certainly another step that could be done in addition to try and make new vs. init and "remove" vs. finalize sensible. > > /* This also removes the workspace from the screens list */ > > - meta_workspace_free (workspace); > > + meta_workspace_remove (workspace); > > This comment is now dumb. Indeed. > (In reply to comment #5) > > Created an attachment (id=122906) [edit] > > Patch for gnome-shell to add the strut > > > + let me = this; > > I already had a "let panel = this;" earlier in Panel._init, for the > new-tray-icon signal handler. So (a) you could just use that variable instead, > and (b) this looks like it's going to be a recurring problem that we should > have some cleverer way to deal with (or at least a standard pattern)... Good catch. I'm thinking we'll follow: http://svn.gnome.org/viewvc/gjs/trunk/doc/Style_Guide.txt?view=markup for the gnome-shell Javascript, which suggests "me" for this. Thanks for the review. I'll land these changes after the gjs structure support is committed.
gjs changes have landed; I've now committed the changes to metacity-clutter and gnome-shell.