Re: [evolution-patches] patches for bug 44592:accessibility initializing codes to loading evolution and gal atk support extension library

[Ettore Perazzoli <ettore ximian com>]

Hi Gilbert,

On Thu, 2003-06-12 at 10:29, Gilbert Fang wrote:
>   This patch is the ASE initialization part.

I re-iterate my opinion that it shouldn't go on HEAD yet.  :-)

Anyway, here are my comments on the patch:

	[init.c]
        +extern void gnome_accessibility_module_init     (void);
        +extern void gnome_accessibility_module_shutdown (void);

The declarations are redundant.  Also, I don't think Evolution's a11y
module should pollute the gnome_* namespace; you should just pick one
prefix and stick with it.  Actually you should probably use e_a11y_
instead of evolution_a11y_ since we use the e_ prefix everywhere else.

Or maybe there is a reason for having them named like this?

	[init.c]
        +       fprintf (stderr, "Evolution Accessibility Support
        Extension Module initialized\n");
        
	[init.c]
        +       fprintf (stderr, "Evolution Accessibilty Support
        Extension Module shutdown\n");

What's the point of these?

	[configure.in]
         dnl **************************************************
        +dnl * Accessibility support
        +dnl **************************************************
        +AC_ARG_ENABLE(accessibility, [ 
        --enable-accessibility=[no/yes]      Enable support for
        accessibility.],,enable_accessibility=no)
        +if test "x$enable_accessibility" = "xyes"; then
        +       AC_DEFINE(ENABLE_ACCESSIBILITY,1,[Enable Accessibility
        support])
        +       msg_accessibility=yes
        +       PKG_CHECK_MODULES(ACCESSIBILITY, atk)
        +       AC_SUBST(ACCESSIBILITY_CFLAGS)
        +       AC_SUBST(ACCESSIBILITY_LIBS)
        +else
        +       msg_accessibility=no
        +fi
        +AM_CONDITIONAL(ENABLE_ACCESSIBILITY, test
        "x$enable_accessibility" = "xyes")

Just an aesthetical nitpick, but I think it would look nicer if we
abbreviated ACCESSIBILITY with A11Y everywhere.

        --- Makefile.am 26 Mar 2003 23:02:26 -0000      1.80
        +++ Makefile.am 12 Jun 2003 06:55:01 -0000
        @@ -22,6 +22,10 @@
                intltool-extract.in     \
                $(pkgconfig_DATA:.pc=.pc.in)
         
        +if ENABLE_ACCESSIBILITY
        +ACCESSIBILITY_SUBDIR = a11y
        +endif
        +
         SUBDIRS =                      \
                 data                    \
                 e-util                  \
        @@ -43,6 +47,7 @@
                views                   \
                wombat                  \
                tools                   \
        +       $(ACCESSIBILITY_SUBDIR) \
                help                    \
                po

I think this is going to confuse Automake when making dist.  You
probably want to put the conditional in the subdir only, so the subdir
is always built but it doesn't actually create a library when a11y is
disabled.

	[main.c]
        +       sub = g_strconcat ("gtk-2.0/modules", G_DIR_SEPARATOR_S,
        fname, NULL);

Here (and in the other cases) you can use g_build_filename() instead.
        
        [main.c]
        +static gboolean
        +accessibility_invoke_module (GnomeProgram *program,
        +                            const char   *libname,
        +                            gboolean      init)
        
Another nitpick: in the shell I like to not namespace local functions. 
So I would name this invoke_a11y_module() instead...

	[main.c]
        +       if ((env_var = g_getenv (GNOME_ACCESSIBILITY_ENV)))

Please split this into two statements.

	[main.c]
        +               do_init = atoi (env_var);
        +       else
        +               do_init = gconf_client_get_bool (
        +                       gconf_client_get_default (),
        +                       GNOME_ACCESSIBILITY_KEY, NULL);

This leaks a GConfClient ref.  You need to g_object_unref() the return
value from gconf_client_get_default() when you are done.

In the same file, does it matter that you are not shutting down the a11y
modules on exit?

The rest of the patch looks fine, although I have never written any a11y
code and I don't know how it works in other apps...

Also I think I would make it non-conditional to simplify things a bit,
since it's going into its own branch anyways -- but do as you like.  :-)

I hope this helps,
-- Ettore