I don't actually use those features, but. Would that actually be an improvement. Notification area space is precious to me, and especially for people who need those technologies enabled, isn't that just a waste of space? (showing it even when a11y features are off seems like a totally bad idea to me.) I've never managed to accidentally make one of those dialogs come up, either, fwiw.

Created attachment 108635 [details] [review]
patch

So, the idea here is that this will only be displayed when you have the "Allow to turn accessibility features on and off from the keyboard" checked.

And there are two principal reasons for this.

1) We dramatically change the behavior of the system when accessx is enabled.  If the user accidentally triggers this is seems like the computer stops working.  In fact, this happened to me yesterday!  Either the warning dialog did not show up at all or it popped under windows due to focus stealing prevention.  The dialog is not useful for someone that intentionally turns this feature on - only for when it is accidentally enabled.  But for that case the dialog is insufficient.

If the dialog is missed or dismissed it is not clear what is going on or how to disable the feature.


2) In the GDM greeter we already have basically the same functionality.  We always enable the accessx keys.  It would be worthwhile to provide a consistent interface between the greeter and the session.


The bubbles are not accessible but that isn't a problem since the bubbles aren't meant for someone that intentionally turned on the feature.  The status icon and preferences dialog, on the other hand, are accessible.

Again, for the user that doesn't enable a11y features there is really no change.

I'd really like some feedback from the a11y crowd on this. What I certainly don't like is that we're more or less replicating the control center dialogs in g-s-d.

I see a few issues with the implementation as well, but those can wait until the basics are cleared up.

What issues do you see with the implementation?  We are already using a good portion of this in the GDM greeter.

Nothing that would make it not work, just a few things I'd like to see cleaned up before committing.

Created attachment 110864 [details] [review]
resync with svn

Updated to svn.

The patch looks great to me, except for the preferences.[ch] thing. Can't we just g_spawn the corresponding capplet in control-center?

Created attachment 115583 [details] [review]
updated

Updated with some changes from the a11y folks.

See http://live.gnome.org/GDM/EaseOfAccess for the consensus that they came to.

Once we land this we'll add .desktop files to autostart for screen reader, keyboard, and magnifier that will start conditionally based on the following gconf keys:
KEY_AT_DIR                "/desktop/gnome/applications/at"
KEY_AT_SCREEN_KEYBOARD_ENABLED  KEY_AT_DIR "/screen_keyboard_enabled"
KEY_AT_SCREEN_MAGNIFIER_ENABLED KEY_AT_DIR "/screen_magnifier_enabled"
KEY_AT_SCREEN_READER_ENABLED    KEY_AT_DIR "/screen_reader_enabled"

Created attachment 115584 [details]
screenshot

Let me start with some comments regarding the strings in the dialog:

"Enable features that make your computer easier to use"

If that were universally true, we should just enable them and be done with it. No icky preferences dialogs needed. Unfortunately, it's not. "more accessible" is probably too technical a term, and I don't have a better suggestion off-hand, but this should be changed.

Also, some of the short descriptions in brackets sound a bit silly. They might be helpful for the AccessX features which might be more widely known and recognized by their short names, but "High Contrast" and "Large Print" especially, and maybe also "Speech Output" are of very dubious utility.

Now, into the code:

What if I don't have a screen reader or magnifier or on-screen kb installed? Those should be handled like the "disabled" setting.

+        if (large_font != NULL && font != NULL && strcmp (large_font, font) == 0) {
+                ret = TRUE;
+        }

Simplify to
ret = (large_font ...);
and get rid of the previous initialization. Same in other places.

+        dialog->priv->xml = glade_xml_new (GLADEDIR "/" GLADE_XML_FILE,
+                                           "main_box",
+                                           PACKAGE);

You shouldn't keep the GladeXML around. Just grab the widgets you need and dump it.

+if $PKG_CONFIG --atleast-version $LIBNOTIFY_REQUIRED_VERSION libnotify; then
+   have_libnotify=yes
+   PKG_CHECK_MODULES(LIBNOTIFY, libnotify >= $LIBNOTIFY_REQUIRED_VERSION)
+   AC_SUBST(LIBNOTIFY_CFLAGS)
+   AC_SUBST(LIBNOTIFY_LIBS)
+   AC_DEFINE(HAVE_LIBNOTIFY, 1, [Building with libnotify support])
+else
+   have_libnotify=no
+fi
+AM_CONDITIONAL([HAVE_LIBNOTIFY], [test $have_libnotify = yes])

That's... odd.

Why check for libnotify twice? Why define HAVE_LIBNOTIFY twice (PKG_CHECK already does that, plus the substs)? And you don't seem to actually use the AM_CONDITIONAL anywhere.

Overall, I still don't like the idea of having to maintain (at least) two dialogs for (the same) a11y settings but until someone comes up with a usable one for the control center that can also be spawned from gdm I don't see any other way.

Ok, I made the changes you requested and committed this to trunk.  I'll mark this fixed.  Thanks.