GNOME Bugzilla – Bug 643959
Make mutter into a library
Last modified: 2011-03-07 23:36:29 UTC
as previously discussed in bug 641724 The early_initialize stuff is reverted both because it's now unnecessary, and because it actually gets in the way, since the plugin manager will believe it has already loaded the plugins before we get a chance to install ours. I called the library libmutter-wm, because it seemed odd to call it libmutter when we already had libmutter-private, since it would imply that libmutter was "public", when if anything, it's even more private. Alternatively, we could just get rid of libmutter-private, since it would presumably be possible to use libmutter(-wm) for anything you could do with libmutter-private. We may want to do something different with command-line handling; for the moment gnome-shell doesn't have any command-line options of its own, but that could change in the future. Also, currently the --mutter-plugins argument is still there even in gnome-shell, but it probably shouldn't be. So maybe we should have a meta_get_option_group(flags) method or something.
Created attachment 182554 [details] [review] Revert "plugins: Add early_initialize vfunc" This reverts commit f2158218bef0c51ea67c9f52b56227063d74ca62.
Created attachment 182555 [details] [review] Allow mutter to be used as a library Move all of the mutter code into a new libmutter-wm.so, split its main() method into meta_init() and meta_run(), add methods for using in-process plugins, and add libmutter-wm.pc pointing to the new library. The mutter binary is now just a tiny program that links against libmutter-wm.
Created attachment 182556 [details] [review] Move the installed includes to a subdir If mutter is going to be a "real" library, then it should install its includes so that users can do #include <meta/display.h> rather than #include <display.h> So rename the includedir accordingly, move src/include to src/meta, and fix up all internal references. There were a handful of header files in src/include that were not installed; this appears to have been part of a plan to keep core/, ui/, and compositor/ from looking at each others' private includes, but that wasn't really working anyway. So move all non-installed headers back into core/ or ui/.
Review of attachment 182554 [details] [review]: Probably should include a brief justification in the commit message
Review of attachment 182555 [details] [review]: Mostly looks good to me ::: src/Makefile.am @@ +201,3 @@ +mutter_SOURCES = core/mutter.c +mutter_LDADD = -lmutter-wm I'd expect libmutter-wm.la not -lmutter-wm here @@ +233,3 @@ --include=xlib-2.0 \ --include=xfixes-4.0 \ + --library=mutter-wm \ Think this should be libmutter-wm.la as well, mutter-wm we're asking g-ir-scanner to guess whether it should try to find a system library or use something built locally, and that only has the possibility to go wrong. (Other possiblity woudl be to switch over to Makefile.introspection since things are pretty much standard at this point.) @@ +241,3 @@ +Meta-$(api_version).typelib: Meta-$(api_version).gir + $(AM_V_GEN) $(G_IR_COMPILER) Meta-$(api_version).gir -o $@ Hmm, this is unrelated to this change - the hack is no longer needed because Colin fixed the problem in gobject-introspection in 2009: commit 6d510b8db845f0c368dddf9b8d15aaac71a8a676 Author: Colin Walters <walters@verbum.org> Date: Tue Aug 18 10:23:09 2009 -0400 [typelib] Clean up dlopen handling It's was busted that g_typelib_new_* does the dlopen() since that caused g-ir-compiler to load the modules even though it wasn't going to do anything with them. Instead, change things so that g_module_symbol does the dlopen on-demand. Remove the extra dlopen(NULL) inside girepository.c, we had another already in gtypelib.c. Thanks to Owen Taylor for suggesting this approach. not because of the binary => library change. Don't really care if it's slipstreamed in here though, just irrelevant ancient history. @@ +254,3 @@ +testboxes_LDADD = -lmutter-wm +testgradient_LDADD = -lmutter-wm +testasyncgetprop_LDADD = -lmutter-wm Again, use the .la files here ::: src/compositor/meta-plugin-manager.c @@ +165,3 @@ + g_list_free (plugin_mgr->plugins); + plugin_mgr->plugins = NULL; + return; Hmm, works, I guess. Should we just do a pre-patch that removes the GConf key and the reload functionality and requires plugins to be passed on the command line? @@ +238,3 @@ + { + plugin_mgr->plugins = g_list_copy (plugin_mgr->installed_plugins); + return TRUE; If we allowed passing plugins only on the command line and disallowed reloading, then you could remove the return TRUE here and avoid the "if you install one plugin the command line is ignored" oddity. Doesn't really matter - it's not like we expect multiple simultaneous plugins to actually work. ::: src/compositor/meta-plugin.c @@ +619,3 @@ + * @plugin: a #MetaPlugin + * + * Installs @plugin as a compositor plugin. If at least plugin is 'at least one' ::: src/core/mutter.c @@ +2,3 @@ + +/* + * Copyright (c) 2011 Red Hat, Inc. The '(c)' has no legal meaning. If you are including it for consistency with the rest of Metacity, then the rest of metacity uses an upper case '(C)'
Review of attachment 182556 [details] [review]: I agree with the changes, hard to *really* check the details, but hopefully if it works it works :-)
Created attachment 182622 [details] [review] Use standard introspection configure/Makefile bits This changes the introspection configure flag from --with/--without-introspection to --enable/--disable-introspection, and changes it so that trying to enable introspection when g-i is not installed results in an error, rather than being silently ignored.
Created attachment 182623 [details] [review] Remove some unused plugin functionality Revert the early_initialize changes (which get in the way in the "libmutter-wm" paradigm), remove the GConf key for setting plugins, and remove plugin "params", which weren't being used. Also remove all the logic for unloading and reloading plugins, since the list never changes after startup now.
Created attachment 182624 [details] [review] Allow mutter to be used as a library This now splits out command-line handling as well, so that we can have mutter-binary-specific (and gnome-shell-specific) command-line args
Created attachment 182625 [details] [review] Move the installed includes to a subdir mostly unchanged, but with a few minor fixes so it distchecks now
Review of attachment 182622 [details] [review]: Looks good (I would probably just make it a hard-dep if doing it from scratch at this point, but doesn't hurt to keep it optional.) ::: configure.in @@ +213,3 @@ +GOBJECT_INTROSPECTION_CHECK([$INTROSPECTION_VERSION]) + +if test x$found_introspection != xno; then hmm, depending on this seems like an internal detail, but it doesn't seem like the check allows for action-if-found/action-if-not-found, so I guess it's not an internal detail.
Review of attachment 182623 [details] [review]: Looks like it should work and as expected simplifies things a bunch. One left-over comment. ::: src/compositor/meta-plugin-manager.c @@ +177,3 @@ + plugin_type = (GType)GPOINTER_TO_SIZE (iter->data); + plugin = g_object_new (plugin_type, "screen", screen, NULL); + plugin_mgr->plugins = g_list_prepend (plugin_mgr->plugins, plugin); This doesn't really seem coherent to me, in that if additional plugins are installed after multiple screens are open, we don't add them to the additional screens, but just part of the whole mutter multiscreen pile of non-working that will be fixed up when someone cares, or, which is likely, if that never happens, never. ::: src/core/prefs.c @@ +1066,1 @@ /* We now initialize plugins so that they can override any preference locations */ Leftover comment
Review of attachment 182624 [details] [review]: Looks good to me, just a few comments ::: src/core/main.c @@ +249,3 @@ * + * Returns a #GOptionContext initialized with mutter-related options. + * Parse the command-line args with this before calling meta_init(). This is really confusing, I think what you mean is that you should add program-specific options ot this option context before calling meta_init()? @@ +573,3 @@ + +int +main (int argc, char **argv) What's this doing here?
Review of attachment 182625 [details] [review]: Assumming still good
(In reply to comment #12) > This doesn't really seem coherent to me, in that if additional plugins are > installed after multiple screens are open, we don't add them to the additional > screens Oh, yeah, I meant to document that you have to do all the loading/registering before meta_init(). Hm... which gnome-shell doesn't do. (In reply to comment #13) > + * Parse the command-line args with this before calling meta_init(). > > This is really confusing, I think what you mean is that you should add > program-specific options ot this option context before calling meta_init()? No, in the latest version, the option parsing is done by the program (mutter, gnome-shell), in between meta_get_option_context() and meta_init(). Otherwise you'd have to make all your command-line args be handled via callbacks, which could be complicated if some of them depended on other ones. > +int > +main (int argc, char **argv) > > What's this doing here? doh, that's the remnants of the old mutter main(). Apparently I hadn't dealt with meta_restart_after_quit yet. But that can just move into meta_run().
Comment on attachment 182555 [details] [review] Allow mutter to be used as a library belatedly obsoleting this
Created attachment 182715 [details] [review] Remove meta_restart() / "mutter-message restart" meta_restart() was broken, in that it restarted mutter with what was left of argv after GOption and Clutter had possibly modified it. Rather than try to fix this, just remove it. [To be rebased to before the libmutter-wm patch.]
Created attachment 182716 [details] [review] Minor updates to be squashed I would have added a g_return_if_fail() to meta_plugin_type_register(), but there was no immediately obvious thing to check. (There is a corresponding change to squash to gnome-shell, to call g_type_init() explicitly and then register the plugin type before calling meta_init().)
Review of attachment 182715 [details] [review]: Seems OK ::: src/core/main.c @@ -573,3 @@ -int -main (int argc, char **argv) This looks like it's in the wrong place - if this was rebased first, you couldn't just remove main()
Review of attachment 182716 [details] [review]: OK, so if the intent was that the caller parses the option group, then what you should do is remove the argc/argv from meta_init(), you can just pass NULL/NULL to clutter_init() - it doesn't even touch those arguments at all when clutter_get_option_group_without_init() has been called. Beyond that, my comment is that the gnome-shell plugin doesn't actually work in my current testing, even when I move the call to type_register() before the call to meta_init(), marking this patch needs-work since I think more fixup is needed, though everything in this patch seems OK to me, actually. ::: src/core/main.c @@ +210,3 @@ "replace", 0, 0, G_OPTION_ARG_NONE, &opt_replace_wm, + N_("Replace the running window manager"), string freeze break if goes in after today
Created attachment 182769 [details] [review] Set plugin features from constructed(), not init() Subclass vtables aren't available from init(), so look up plugin features in constructed(). This makes things work better for me; one of the real oddities of GObject is that object->klass is set to the parent classes vtable when the parent classes init function is called, never *quite* understood the justification for that.
squashed, rebased, pushed Attachment 182622 [details] pushed as a66ae4a - Use standard introspection configure/Makefile bits Attachment 182623 [details] pushed as 4c76791 - Remove some unused plugin functionality Attachment 182624 [details] pushed as bb50f65 - Allow mutter to be used as a library Attachment 182625 [details] pushed as c84da3c - Move the installed includes to a subdir Attachment 182715 [details] pushed as 2875271 - Remove meta_restart() / "mutter-message restart"