What I think would probably make sense is to:

 A) Have a XSetting Gtk/Modules that mapped to the
    gtk-modules GTK+ setting 

    (that would allow you to put gtk-modules = "foo"
     in your gtkrc)

 B) Say that the set of modules loaded is the set
    of modules requested for all screens

The hardest question that comes to mind is if you
want to unload a module if no screens specify it any
more ... it's probably right (and people can prevent
unloading with g_module_make_resident), but on the 
other hand, I doubt any existing module handles it
properly.

Created attachment 18284 [details] [review]
Module loading/unloading via XSETTING or rc file

Here's the patch for gtk+ HEAD.  It adds gtk/gtkmodules.[ch], which
clears all the module stuff out of gtkmain.c, and adds a "gtk-modules"
notify handler to gtksettings.c.

Commitable?

Various comments; this isn't really meant to be completely
comprehensive, but rather to cover the major issues, and
mention various small issues that I happened to notice along
the way. 

 - Please fix the copyright in gtkmodules.[ch]; just because
   you created the file, you didn't write all the code. Most all
   of the code you moved there is my code, but some of it's
   evolved versions of older code from Tim.

   Copyright 1998-2002 Tim Janik, Red Hat, Inc., and others.
   Copyright 2003 Alex Graveley

   is probably a good copyright notice

 - I think using gdatalist like this is a bit idiosynchratic
   and should be avoided. gdatalist is really meant for using
   the same keys in many places, not different strings 
   in one place.

   (And as a further sign that it should be avoided, your
   code using it is buggy - you make the assumption that
   g_datalist_init (&list) will result in a non-NULL
   list, but g_datalist_init (&list) is actually
   list = NULL.)

 - The comment in gtk_module_load_info 
   "Initialize multihead aware gtk modules..." is stale
   with respect to the code.

 - You are doing two entirely unrelated operations in
   foreach_modules_callback() based on whether the 
   display is NULL or not! Either use two different
   foreach functions or (better) leave the code as
   it was with an explicit iteration over the list.
  
   The comment you put in foreach_modules_callback()
   actually refers to the first test in 
   default_display_notify_cb.

 - You have various debug printfs left in your code

 - I'm not fond of the gboolean returning shutdown
   function. It seems like an unnecessary extra 
   complication.

 - There is some question in my mind whether the behavior
   is correct when the mapping from 'name specified in list of 
   modules' to 'loaded file name' changes due to 
   changes in what modules are on disk.

   I think it might be better to, for each loaded module,
   keep a list of all names from the list for which it
   is found, and check that before hunting on disk. So,
   if we earlier loaded a module for 'gail', we never
   search again on disk for 'gail' again until the first
   module is unloaded.

   The way it works now, if the same module is specified
   in multiple different GtkSettings, or in GTK_MODULES
   and in a GtkSettings, then you can have different 
   shared objects loaded for that.

 - settings_update_modules() migth as well be directly
   in gtkmodules.c 

 - I don't see a reason for the if (gdk_screen_get_number (screen) == 0),
   we already know how to merge different lists of modules,
   we might as well merge the lists for all screens on the display.

 - The fact that you don't call _gtk_modules_init() unless
   either GTK_MODULES or --gtk-modules is set doesn't look
   right to me; how is the notification on newly opened
   displays established?
 
 - gtk_argc/gtk_argv should be removed from gtkmain.c
   and should be static in gtkmodules.c

this is cool, we should log an RFE against at-spi for dynamic loading
of accessibility support (or perhaps log against libgnome).  Thanks Alex!

Sorry for the lapse, been away from the internet for a month. 
Comments on your points:

#1) doh.

#2) i don't think this is a big deal, but i'll switch it to a
hashtable anyways.

#6) is it okay to modify the module API like this?

#7) the thinking here was that if you somehow specify different
physical modules with the same name, then you know what you are doing
and they should probably both be loaded.  its your call.

#9) seems like a waste as screens in the same display all share the
same xsettings... err i thought they did anyways?

#10) looks like a bug, ya.

#11) this is done already.

Also, thanks for taking the time to review this patch owen :)

Bill, Just FYI I've already got patches to all the a11y and
control-center stuff to get loading on the fly, but I'm waiting for
this patch to get okay'd before doing anything with them.

Alex: how is this setting getting turned on/off?  Is g-s-d setting the
XSETTING according to the gconf value? I think that might be the
requirement, since the gconf value is exposed API at the moment (i.e.
documented via gconftool-2, etc.)

By the way, Alex: we'd _much_ prefer if the XSETTING were 'G_MODULES'
or some such, instead of 'GTK_MODULES', since the a11y preload
mechanism will need to be implementable by non-gtk+ applications in
the future.  Since we're revising, we ought to keep the gtk-isms to a
minimum here if possible.

For example, KDE is planning to reuse at-spi stuff, via bridging to
ATK.  Ideally they could load g_modules (they have accepted the glib
dependency in ATK), but using gtk+ API (even if it's just the
gtk_module wrapper around gmodule) would obviously be a problem.

These patches don't effect the atk modules at all, it is just a
mechanism to get them loaded into a gtk app at runtime.  This doesn't
apply to a KDE app, and KDE will have to implement its own mechanism.

* I don't understand:

> #6) is it okay to modify the module API like this?

* GtkSettings are in fact per-screen (think DPI, font size
  for why)

* As far as I can see, the cases where multiple different
  "gails" got loaded aren't from intention, but rather
  from installing a new version in a different prefix
  while the app was running or things like that.

I hadn't realized this was still waiting at me. Unfortunately,
without final patch at this point, it's going to have to wait 
until GTK+-2.6.

This is going to keep apps from dropping the libgnomeui dependency, if that is
still a GTK 2.6 goal

Here is a new version of Alex' patch, which applies to 2.5.2, and incorporates 
Owens comments above. It compiles, but needs to be tested using xsettings-manager
and some dummy modules...

Created attachment 31114 [details] [review]
new patch

Looks good in broad terms. Various comments, most trivial, some not.
(There's a double free when reloading, the init functions aren't
called exactly at the right time.)

===
--- gtk/gtkmain.c	9 Aug 2004 16:59:52 -0000	1.240
+++ gtk/gtkmain.c	31 Aug 2004 04:24:23 -0000
@@ -34,10 +34,11 @@
 #include <libintl.h>
 #endif
 
+#include <glib.h>
+

 #include <stdio.h>
====

* Preferred ordering is system includes, then library includes, then local includes.

====
-		  if (gtk_modules_string)
-		    g_string_append_c (gtk_modules_string, G_SEARCHPATH_SEPARATOR);
-		  else
-		    gtk_modules_string = g_string_new (NULL);
-
+		  g_string_append_c (gtk_modules_string, G_SEARCHPATH_SEPARATOR);

 		  g_string_append (gtk_modules_string, module_name);
===

I don't think the rewrite of the handling of gtk_modules_string in this
function is correct ... you can end up with a leading ":" on the string.


===
+  /* load gtk modules */
+  _gtk_modules_init (*argc, *argv, gtk_modules_string->str);
+  g_string_free (gtk_modules_string, TRUE);
===

 gtk_parse_args (NULL, NULL) is supposed to work.

====
--- gtk/gtkmain.h	13 Jul 2004 14:49:02 -0000	1.54
+++ gtk/gtkmain.h	31 Aug 2004 04:24:23 -0000
@@ -57,9 +57,6 @@ extern "C" {
 
 #endif /* GTK_DISABLE_DEPRECATED */
 
-typedef void	(*GtkModuleInitFunc)        (gint	  *argc,
-				             gchar      ***argv);
-typedef void	(*GtkModuleDisplayInitFunc) (GdkDisplay   *display);
====

I think you should include a #include <gtk/gtkmodules.h> to preserve
compatibility when moving these declarations. They aren't used very
much (if at all) by apps, but they are in a public header.

====
+      for (l = gtk_modules; l; l = l->next)
+	{
+	  info = l->data;
+	  if (g_slist_find_custom (info->names, 
+				   (gconstpointer) name, 
+				   (GCompareFunc)strcmp))
====

The (gconstpointer) cast isn't needed.

====
+	      info = (GtkModuleInfo *) g_slist_find (gtk_modules, 
+						     (gconstpointer) module);
====		  

This can't be right ... gtk_modules holds GtkModuleInfo while module is a
GtkModule.

====
+		  if (default_display_opened && !info->display_init_func)
+		    (* info->init_func) (&gtk_argc, &gtk_argv);
+
+		  /* iterate opened displays for modules supporting display init */
+		  if (info->display_init_func) 
====

The comment should be above the !info->display_init_func and explain that modules
without a display_init_func are not multihead aware and are initialized once
when the default display is first set, while modules with a display_init_func
are multihead aware have their init_func called immediately always and 
their display_init_func called when displays are open.

And because that's the way it works, the first line needs to be changed to

===
+		  GTK_NOTE (MODULES, g_print ("Reloading module: %s\n", name));
+		  
+		  if (!g_slist_find_custom (info->names, 
+					    (gconstpointer) name, 
+					    (GCompareFunc) strcmp))
+		    info->names = g_slist_prepend (info->names, g_strdup (name));
+		  info->ref_count++;
+		  /* FIXME: keep list of names */
+		  g_module_close (module);
+		  module = NULL;
===

"Reloading module" here is confusing. (confused me) I think "Module %s already
loaded, 
ignoring" or something would be clearer.

I don't know what the FIXME is about; it needs to be either made really specific,
fixed, or removed.

The g_module_close() should probably be commented with /* remove new reference
count on module, we already have one */ or something, since for people not familiar
very with GModule, it's not obvious that g_module_close() here won't close 
the module.

I'm not sure why module is set to NULL here. I think the whole function would be
clearer if the usage modinit_func variable to indicate success was removed and
a 'gboolean success' variable was added.

===
+void
+_gtk_modules_init (gint         argc, 
+		   gchar      **argv, 
+		   const gchar *gtk_modules_args)
+{
+  GdkDisplayManager *display_manager;
+  gint i;
+
+  for (i = 0; i < gtk_argc; i++)
+    g_free (gtk_argv [i]);
+  g_free (gtk_argv);
===

As far as I can see this function can only be hit once. So if anything, I'd just
g_assert (gtk_argv == NULL).

===
+  gtk_argv = g_malloc ((argc + 1) * sizeof (char*));
===

Might be good to clean this up to g_new() when moving it.

===
+  g_slist_free (load_modules (gtk_modules_args));
====

Needs a comment along the lines of "Modules specified in GTK_MODULES
or on the command line are always loaded, so we'll just leak the refcounts"

====
+  GSList *new_modules = NULL, *old_modules = NULL, *iter;
====

Tthe old_modules initialization isn't needed and mixing initializations
in a list of variables like this is ugly.

===
+  old_modules = g_object_get_data (G_OBJECT (settings), "gtk-modules");
+
+  for (iter = old_modules; iter; iter = iter->next) 
+    {
+      GtkModuleInfo *info = iter->data;
+      gtk_module_info_unref (info);
+    }
+  g_slist_free (old_modules);
+
+  g_object_set_data_full (G_OBJECT (settings),
+			  "gtk-modules",
+			  new_modules,
+			  settings_destroy_notify);
===

This double frees the list. Either g_object_steal_data() needs to be 
used or just reset and let the destroy_notify take care of cleaning up
the old list.

Created attachment 31183 [details] [review]
new patch, still untested

Created attachment 31190 [details] [review]
tested patch

Testing revealed that it basically works, two changes were necessary
a) when a display is opened, get the initial value of the settings
b) when a display is closed, make sure the modules are unrefed

2004-09-01  Matthias Clasen  <mclasen@redhat.com>

	Make it possible to specify additional modules to load
	via a setting.  (#117236, Alex Graveley)
	
	* gtk/gtkmodules.h: 
	* gtk/gtkmodules.c: New files which contain the module
	handling code which was previously in gtkmain.[hc]. 
	Additionally, the code now looks for the gtk-modules 
	setting, which can specify additional modules to load.

	* gtk/gtkmain.c: 
	* gtk/gtkmain.h: Remove all the module handling code.

	* gtk/gtkdebug.h: Add a debug flag for modules.

	* gtk/gtk.h: Include gtkmodules.h

	* gtk/Makefile.am (gtk_public_h_sources): Add gtkmodules.h
	(gtk_c_sources): Add gtkmodules.c

	* gtk/gtksettings.c: Add the gtk-modules setting.

	* gdk/x11/gdkevents-x11.c: Add the Gtk/Modules XSetting.

Alex: (see comment #9) - my issue is with the _name_ of the XSETTINGS variable
(if that's what ended up being used). 

To clarify:

Whatever we do here should have a toolkit-agnostic, discoverable "setting" that
apps and toolkits can use to determine that assistive technology support is
needed.  ENV variables aren't nice for this for reasons discussed long ago, and
gconf clearly isn't OK for gtk+ only or kde apps.  So apart from the mechanism
for loading _specific_ g_modules into gtk+ apps, we need a flag that's available
to all apps telling them to load "ATK" and "AT-SPI" support at runtime (KDE has
accepted the need to do this, and is using atk and atk-bridge to enable their
accessibility at runtime).

Bill, what difference would it make if we renamed it to Net/Modules ?
We clearly want to allow modules to use the gtk api, so other toolkits would
have to do some form of filtering to find the generic ones which can be
loaded by all toolkits. 

Renaming it to Net/Modules is *wrong*. These are GTK+ modules. If you
want ot add a Net/AtSpiEnabled key that gconf-settings-daemon sets,
sure. But this can't do anything for GTK+,  because there is no way that GTK+
would know how to process it. What has to be done for GTK+ is to tell it to 
load a specific GTK+ module.

Matthias, 

I really appreciate you taking control of this patch, since it had been
languishing for such a long time.  Thanks!

-Alex

Seems to still be massive miscommunication in the comments here. Did we end up
with an XSETTING in the end?

Yes, Gtk/Modules was how it ended up.