After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 612599 - Require a way to load the accessibility modules
Require a way to load the accessibility modules
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Dan Winship
gnome-shell-maint
Depends on: 624571
Blocks: 614121 626660 634016 640057
 
 
Reported: 2010-03-11 17:13 UTC by Alejandro Piñeiro Iglesias (IRC: infapi00)
Modified: 2011-01-20 14:55 UTC
See Also:
GNOME target: 3.0
GNOME version: ---


Attachments
First draft (7.45 KB, patch)
2010-03-11 17:13 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
Updated patch (9.08 KB, patch)
2010-03-16 16:46 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
Optional patch using features not implemented in Clutter (8.26 KB, patch)
2010-04-28 11:47 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
Added a conditional cally dependency (2.75 KB, patch)
2010-05-21 14:48 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
Added a11y initialization code (10.41 KB, patch)
2010-05-21 14:49 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
a11y initialization code using bug 619946 patch (6.92 KB, patch)
2010-06-01 15:18 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
Initializes accessibility support (7.30 KB, patch)
2010-06-18 16:13 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
reviewed Details | Review
0001-Added-a11y-initialization-code (7.42 KB, patch)
2010-06-23 12:15 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
accepted-commit_now Details | Review
0002-Added-some-a11y-checks-on-configure (3.32 KB, patch)
2010-06-23 12:15 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
0003-Using-macros-defined-on-configure-checks (2.19 KB, patch)
2010-06-23 12:16 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
Little update (7.39 KB, patch)
2010-08-10 14:09 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
Updated patch (904 bytes, patch)
2010-10-05 10:35 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
The real update (7.39 KB, patch)
2010-10-26 15:24 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
Update: use gsettings instead of gconf (7.08 KB, patch)
2010-12-28 18:12 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
Little update: removing a silly debug message (7.04 KB, patch)
2010-12-28 19:01 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
Little update: NO_GAIL needs to be unset also (7.01 KB, patch)
2010-12-29 11:54 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
Update: rebased patch (7.02 KB, patch)
2011-01-04 12:05 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
reviewed Details | Review
Update the branch after review on comment 39 (8.63 KB, patch)
2011-01-11 15:18 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
accepted-commit_now Details | Review
Update the patch after review on comment 43 (9.07 KB, patch)
2011-01-20 12:15 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review

Description Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-03-11 17:13:01 UTC
Created attachment 155877 [details] [review]
First draft

As load the modules in mutter wasn't a real good idea to me (IMHO) I started to move that to gnome-shell.

Although I started implementing it as a extension, in order to being as less intrusive as possible, Colin Walters advised me to implement it directly on gnome-shell-plugin, as extensions seems to not be suitable for that. As he also talked about review patches in the early stages, I have uploaded a first draft.

The patch basically load cally and atk-bridge during the gnome-shell-plugin object construction if the gconf property says that.

Issues:

Cally is not still a gnome module, but as it is loaded as a dynamic module, it doesn't add any compile dependency, so I suppose that it is ok.

It only checks the gconf property on initialization time. Probably it would be good to add a listener so the modules could be loaded if the user change the a11y settings.

Problems:

The patch right now uses hardcoded directories for the module loading.

Some context.

One could wonder why make the same mistake that Firefox, implementing an own custom a11y load module process.

Gtk+ apps use GTK_MODULES envvar to know which modules load, and it uses the default gtk module directories to search them. It includes the atk-bridge, that it is placed in the gtk+ module directory, although this is something debatable, as now the bridge can be used for several toolkits. More information here [1]

The evident solution would be implement a similar approach on clutter: CLUTTER_MODULES. But this has also problems, as I said here [3]. The summary is that the modules required to be loaded are not only clutter modules. atk-bridge is a general module, and shouldn't be placed on clutter module directories, so a general clutter module process is not 100% suitable here.

Yes, the straightforward solution would be copy atk-bridge module to the clutter module directory, but IMHO it is not a good idea. In the same way, clutter-modules is not merged on the trunk, so we would need to wait to the next stable API if we choose that.

So, IMHO, right now it could be better to make a custom way to solve this situation and just be the gnome-shell the one loading by hand the modules.

So, about the directory, the solution to this hardcoded directories would be:

* Search the atk-bridge in the gtk module directory (as it is likely that it will be maintained here)
* Search the cally module in the module directories proposed by the clutter-modules branch, although it is not merged on the master, and there are no plans to do that. This would also solve [2], as cally right now is installed on the gtk module directory (legacy)

CCing Willie Walker, as he is more used to this a11y initalization issues.

[1] http://mail.gnome.org/archives/gnome-accessibility-list/2009-August/msg00019.html
[2] http://bugzilla.o-hand.com/show_bug.cgi?id=1737
[3] http://bugzilla.o-hand.com/show_bug.cgi?id=1738
Comment 1 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-03-16 16:46:51 UTC
Created attachment 156286 [details] [review]
Updated patch

A updated patch trying to avoid the big problems of the previous one.

Now the cally module is being searched on the directories proposed by the on bug 1738 [1], implemented on the branch clutter-modules.

But the branch is not merged, so the current clutter-version doesn't include CLUTTER_API_VERSION_S and CLUTTER_ABI_VERSION_S. This means that the cally directory is only searched using the envvar CLUTTER_MODULES_PATH. Enough for the moment.

The bridge directory is more simply. It tries to find it in "gtk-2.0/modules" in the default gtk libdir (the one gnome-shell is using). This is the same that make the current corba at-spi in his relocate code.

This could be not true in some of the development environments, but mostly true in other cases. Gtk+ doesn't export any of the gtkmodules headers, so any other search solution would mean re-implement gnome_program_locate_file, and I think that it is not worth.

Anyway, it could be possible to just add another envvar, and have this two paths to look for.

Any opinion or suggestion is welcome.

[1] http://bugzilla.openedhand.com/show_bug.cgi?id=1738
Comment 2 Colin Walters 2010-03-24 19:57:12 UTC
(In reply to comment #1)
> Created an attachment (id=156286) [details] [review]
> Updated patch
> 
> A updated patch trying to avoid the big problems of the previous one.

Can you expand the git commit message a bit more?  Something like:

Load Clutter accessibility module if accessibility is enabled

(bit more description here)

Also can we poke the clutter-modules branch and get it in rather than have the commented out/hackish bits?
Comment 3 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-03-24 21:59:08 UTC
(In reply to comment #2)
 
> Can you expand the git commit message a bit more?  Something like:

Sure, just being focused in the code.

> Also can we poke the clutter-modules branch and get it in rather than have the
> commented out/hackish bits?

If you are talking about the CLUTTER_[API/ABI]_VERSION_S code commented, yes probably it would be a good idea to just ask to export that information on clutter-version.h

About the hackish bits, if you are thinking about how cally/atk-bridge is loaded ... well as I explained on comment 1 and here [1], clutter-modules is intended to just load clutter modules, and it will search on clutter-related directories. But here we also want to load a non-clutter module, atk-bridge, and in the same way, we *need* to be sure that the order is cally:atk-bridge.

As I said, in the GTK world this is solved by putting atk-bridge in the gtk modules directory, not really true [2], but a good pragmatic solution. In the same way if in the future it is also required to load GAIL (AFAIK the idea is create gnome-shell as a pure clutter app, but that could change) a specific order could be also be required.

Anyway, CCing Emmanuele Bassi to check if he has any additional comment.

[1] http://bugzilla.o-hand.com/show_bug.cgi?id=1738
[2] http://mail.gnome.org/archives/gnome-accessibility-list/2009-August/msg00019.html
Comment 4 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-04-02 15:42:59 UTC
Little update. 

Right now we are discussing if it would be a good idea to forget the idea of Cally as a isolated module to be loaded on runtime, an integrate that directly on clutter. See the discussion here [1]

In this case this patch should be required to be updated (simplified)

Anyway, it would be still required to get a way to load the atk-bridge, and be sure that this is done *after* the cally initialization.

[1] http://mail.gnome.org/archives/gnome-accessibility-devel/2010-March/msg00003.html
Comment 5 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-04-28 11:47:41 UTC
Created attachment 159778 [details] [review]
Optional patch using features not implemented in Clutter

Right now we are in a process of integrate (or at least try to) cally into clutter. See clutter bug http://bugzilla.o-hand.com/show_bug.cgi?id=2069 and all his dependencies.

In this case the cally module loaded on runtime will disappear.

This patch uses all this features being implemented but not applied (patches still requires some review, and we still have the copyright waive problem).

Anyway, it is still required to load the atk-bridge by hand(in fact, this patch is basically that).
Comment 6 Dan Winship 2010-05-14 14:35:45 UTC
If the merging-cally-into-clutter issues aren't going to be solved soon, a simple solution for now might be to install cally as a shared library rather than as a module, and then gnome-shell could just link to it directly.

As for loading atk-bridge, that code looks extremely hackish and fragile...

And I don't get the comments about GTK_MODULES; if mutter calls gtk_init and thus loads GTK_MODULES, then doesn't that mean that if a11y is enabled at the desktop level, then mutter will already have loaded gail before loading the shell plugin?

One possibility there is that we can adjust $GTK_MODULES from the gnome-shell wrapper script. (But doesn't it get the a11y module list from XSETTINGS anyway? Or did that never happen?) Anyway, it would be nice if we could cause atk-bridge to be loaded in the "normal" way rather than hacking it ourselves. (And then that's one more thing we don't need to worry about updating in gtk 2.x -> 3.0.)
Comment 7 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-05-14 16:23:04 UTC
(In reply to comment #6)
> If the merging-cally-into-clutter issues aren't going to be solved soon, a
> simple solution for now might be to install cally as a shared library rather
> than as a module, and then gnome-shell could just link to it directly.

Well, this is an option, but I discarded it because this would mean add cally as a gnome-shell dependency, and I always thought that this would be not possible.

You don't see a cally dependency as a problem?

Anyway about this "aren't going to be solved *soon*": no idea. I don't have any time frame about this. Probably Emmanuele Bassi knows it better.

> As for loading atk-bridge, that code looks extremely hackish and fragile...

Well, it is based on the deprecated libgnome-ui, although somewhat simplified.

Anyway, it checks far enough the possible errors, so if it fails on runtime because it doesn't find the atk-bridge, it doesn't crash, it is just not able to load the bridge.

The main issue it that it requires to "guess/hardcode" in someway were the atk-bridge module is placed, as it is placed in a gtk+ directory. Anyway, checking bugs like [2] I think that I should investigate the possibility to use GTK_PATH envvar. I will investigate it.

Anyway, reviewing nsAppRootAccessible.cpp::LoadGtkModule method on firefox code, it is a similar code (and similarly hacky and weak).

> And I don't get the comments about GTK_MODULES; if mutter calls gtk_init and
> thus loads GTK_MODULES, then doesn't that mean that if a11y is enabled at the
> desktop level, then mutter will already have loaded gail before loading the
> shell plugin?

Why mutter requires to load GAIL? I supposed that gnome-shell was trying to avoid as much as possible gtk-clutter interaction, and that gtk_init is required due some utility theme-related functions.

In fact as this moment I didn't use much time investigating this gtk-clutter interaction because a) I thought it wasn't required on gnome shell and b) it would be a total headache, so this isn't worth.

> One possibility there is that we can adjust $GTK_MODULES from the gnome-shell
> wrapper script. (But doesn't it get the a11y module list from XSETTINGS anyway?
> Or did that never happen?) Anyway, it would be nice if we could cause
> atk-bridge to be loaded in the "normal" way rather than hacking it ourselves.
> (And then that's one more thing we don't need to worry about updating in gtk
> 2.x -> 3.0.)

Yes, as far as I know it is using XSettings right now [5].

When you mean "normal" way, you mean on the gtk_init?

The problem is that we need to be sure that the accessibility implementation toolkit module (or library as you suggested) being loaded, should be loaded *before atk-bridge* and be the "active" one in the moment atk-bridge is loaded.

This is because most AtkUtil [1] methods are implemented as class methods, and with the assumption of just one toolkit at the moment.

When atk-bridge is loaded one of the first things that does is call atk_util_get_root. Each toolkit should implement one. If no one is provided, the default implementation would return NULL. AFAIR, in my previous tests, this means a crash in the at-spi side trying to use the root object, although I would require to confirm that. In the same way, it is called just on the load. This prevents any "lazy loading" approach.

So, ideally we want cally:atk-bridge, although gail:cally:atk-bridge should be also possible.

I have just checked mutter code and he does this:
  meta_ui_init (&argc, &argv);              =>  calls gtk_init
  meta_clutter_init (ctx, &argc, &argv);    =>  calls clutter_init

The gtk_init is the one that would use GTK_MODULES, and we can't use it to load cally, and in the same way we need to ensure that it is the CallyUtil class the one present when the bridge is called. So although gail:cally:atk-bridge is a correct order, gail:atk-bridge:cally is not.

The current approach is similar to the one I implemented on hildon-desktop, and based on what firefox does, as he has a similar problem, as he defines his own AtkUtil implementation. So he overrides GTK_MODULES. It uses NO_GAIL and NO_AT_BRIDGE envars to avoid to load gail and atk-bridge, and then manually:
  a) Loads gail
  b) Initialize his own AtkUtil implementation
  c) Loads atk-briged

See that on (nsAppRootAccessible::nsApplicationAccessibleWrap::Init())

BTW, although I have suggested that GTK_MODULES should be empty, probably it has more sense use NO_GAIL[3] and NO_AT_BRIDGE[4] envvars. They were created for similar problems in firefox and Mono applications.

Well, probably I have just write down a messy explanation, sorry. Feel free to ask whatever you want.

Thanks for review this patches.

[1] http://library.gnome.org/devel/atk/stable/AtkUtil.html
[2] https://bugzilla.gnome.org/show_bug.cgi?id=607077
[3] https://bugzilla.gnome.org/show_bug.cgi?id=565110
[4] https://bugzilla.gnome.org/show_bug.cgi?id=563943
[5] http://mail.gnome.org/archives/gnome-accessibility-devel/2009-April/msg00001.html
Comment 8 Dan Winship 2010-05-14 16:48:45 UTC
(In reply to comment #7)
> You don't see a cally dependency as a problem?

It's temporary. We assume the merge will happen before 3.0. If it's looking like that's not going to happen... well, then we'd have to figure something out.

> > As for loading atk-bridge, that code looks extremely hackish and fragile...
> 
> Well, it is based on the deprecated libgnome-ui, although somewhat simplified.

Sure, but libgnome-ui basically "owned" this integration point. If the a11y hackers were going to change something about how a11y startup worked, they would have fixed the code in libgnome-ui to work with the new system. But they're not going to know that we've copied and pasted it into gnome-shell so they're not going to send the patches to us.

> Anyway, reviewing nsAppRootAccessible.cpp::LoadGtkModule method on firefox
> code, it is a similar code (and similarly hacky and weak).

Firefox should not be looked at as an example of how to integrate with GNOME. :-)

> Why mutter requires to load GAIL?

It doesn't *require* it, it's just that, as you noted later, mutter calls gtk_init(), and it does it *before* any gnome-shell code gets a chance to run. So if GTK_MODULES (or the corresponding xsetting) is set, then gail and atk-bridge will already have been loaded before we get a chance to do anything.
Comment 9 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-05-14 17:30:06 UTC
(In reply to comment #8)
> (In reply to comment #7)
> > You don't see a cally dependency as a problem?
> 
> It's temporary. We assume the merge will happen before 3.0. If it's looking
> like that's not going to happen... well, then we'd have to figure something
> out.

Ok, I can try an optional patch. I will try to have some working by the Tuesday (17th is a bank day here)

> > > As for loading atk-bridge, that code looks extremely hackish and fragile...
> > 
> > Well, it is based on the deprecated libgnome-ui, although somewhat simplified.
> 
> Sure, but libgnome-ui basically "owned" this integration point. If the a11y
> hackers were going to change something about how a11y startup worked, they
> would have fixed the code in libgnome-ui to work with the new system. But
> they're not going to know that we've copied and pasted it into gnome-shell so
> they're not going to send the patches to us.

Well, they didn't changed how a11y startup worked in a total way, they just tweak it, and just because they needed to adapt to changes in the GNOME platform (as this XSettings issue).

But (sorry I don't want to be rude) why they (they?) would have fixed a code of a library marked as deprecated several years ago and not used at all except in outdated systems?

Anyway, yes, it is based on the libgnome-ui code, but it isn't too much different to the code used on gtk to load the modules, except the way to obtain the atk-bridge path.

> > Anyway, reviewing nsAppRootAccessible.cpp::LoadGtkModule method on firefox
> > code, it is a similar code (and similarly hacky and weak).
> 
> Firefox should not be looked at as an example of how to integrate with GNOME.
> :-)

No, in fact it was a a11y headache ;). Anyway, Mono is doing something similar, although I should check the code to see if there are a cleaner way to do that.

> > Why mutter requires to load GAIL?
> 
> It doesn't *require* it, it's just that, as you noted later, mutter calls
> gtk_init(), and it does it *before* any gnome-shell code gets a chance to run.
> So if GTK_MODULES (or the corresponding xsetting) is set, then gail and
> atk-bridge will already have been loaded before we get a chance to do anything.

Yes, but as I said in my previous (boring and huge) comment, we have NO_GAIL and NO_AT_BRIDGE envvars to avoid that. So now we are in the initial spot, we need a way to load the atk-bridge. 

Anything issue that you dislike in particular about the current code to load atk-bridge? You are thinking in any other option?

Anyway, and to finalize, I would like to point that all these issues (where the atk-bridge is placed, problems with getRoot, etc) and others related to the atk-bridge, were pointed out several time ago. The thread here [1]. Mark Doffman summarizes it well: "I'm sure there is a-lot more to this than I have found out, but I'm certain that what Firefox / Mono / Silverlight are doing to modify the order of module loading is a hack."

Unfortunately no conclusion/design/roadmap were extracted from these mails, I guess that mainly due lack of manpower. So probably gnome-shell would be included in this list of apps/toolkits "module loading hackers".

http://mail.gnome.org/archives/gnome-accessibility-list/2009-August/msg00019.html
Comment 10 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-05-21 14:48:56 UTC
Created attachment 161650 [details] [review]
Added a conditional cally dependency
Comment 11 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-05-21 14:49:21 UTC
Created attachment 161651 [details] [review]
Added a11y initialization code
Comment 12 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-05-21 14:58:39 UTC
I have just uploaded another solution, using Dan Winship suggestions (sorry for the delay). I have marked other solutions as obsolete. It is not 100% true, as in fact were better solutions, but as they are blocked and not possible to be used I think that it is better forget them from the moment.

As it is a temporary solution, I added cally dependency as optional. By default the a11y support is not compiled, and cally is not required. In the same way, if you try to enable the a11y support but cally is not installed, it is detected and a11y support is not compiled.

There is another reason for that. Gnome Shell is in Module Proposal process. But Cally not. It isn't a external dependency either. Reason: we are trying to integrate Cally with Clutter so this wouldn't be required at all. I think that doesn't worth to propose Cally as a module/external dependency if the final target is being part of clutter.

About the atk-bridge loading, I finally copy&paste&adapt code from gtkmodules, so I hope this make code less hacky.
Comment 13 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-06-01 15:18:15 UTC
Created attachment 162467 [details] [review]
a11y initialization code using bug 619946 patch

This patch is equivalent to attachment 161651 [details] [review], but in this case it uses a gtk method to find the atk bridge, installed right now as a gtk module.

This method is new, added by the patch uploaded on bug 619946.

This solution is better that just c&p gtkmodules code. Being able to have this gtk_find_module method is now more important, due the imminent gtk-3.0 release.

I don't mark previous attachment as obsolete as Bug 619946 is not closed yet.
Comment 14 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-06-15 15:47:12 UTC
Well, as the clutter copyright waiver is over [1], we have one problem less (well, almost, as it is still in "work in progress", so I will not set attachments 161650 and 161651 as obsolete).

So the remaining issue is the "load atk-bridge".

It seems that it would be really unlikely that bug 619946 would be resolved, as Matthias Clasen (AFAIU), feels that it a patch like that is too ad-hoc, and don't solve the main issue in a elegant way.

We were briefly talking about this on the last #a11y meeting [2]:

* Li Yuan is using GTK_PATH on OpenSolaris. But by default GTK_PATH is unset and I think that this is somewhat dangerous overriding this variable.
* Mike Gorse propose add a new gconf/g_settings var on the at-spi schema file.

I prefer Mike Gorse option. How viable you see something like that?

However, meanwhile I wait for your answers I would work on a patch implementing this option as a proof of concept.

[1] http://bugzilla.openedhand.com/show_bug.cgi?id=2097#c23
[2] http://live.gnome.org/Accessibility/Minutes/20100610#GNOME_3.0_items_status
Comment 15 Dan Winship 2010-06-15 15:52:05 UTC
> * Mike Gorse propose add a new gconf/g_settings var on the at-spi schema file.

(containing the path to the atk-bridge module)

that seems plausible
Comment 16 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-06-18 16:13:00 UTC
Created attachment 164018 [details] [review]
Initializes accessibility support

Here a patch to load the a11y related stuff. This patch supposes that clutter is integrated [1] (remember: copyright waive is over) and that we have a gconf property provided by at-spi2 in order to know the atk-bridge path. Although it is still in process [2], from gnome-shell pov, only the exact name could change, so there won't be too many changes in this aspect.

Just to note that for the first time, after integrate it, I required to use NO_GAIL [3] and NO_AT_BRIDGE [4], as mutter was loading already GAIL. For the same reason, I needed to set NO_AT_BRIDGE to false after check cally initialization, to ensure that the bridge is really loaded.

[1] http://bugzilla.o-hand.com/show_bug.cgi?id=2069
[2] https://bugs.freedesktop.org/show_bug.cgi?id=28580
[3] https://bugzilla.gnome.org/show_bug.cgi?id=565110
[4] https://bugzilla.gnome.org/show_bug.cgi?id=563943
Comment 17 Dan Winship 2010-06-18 16:37:21 UTC
(In reply to comment #16)
> Here a patch to load the a11y related stuff. This patch supposes that clutter
> is integrated

meaning it won't build against the current clutter, right? (no clutter_get_accessibility_enabled () at least?). If you added a configure check and an #ifdef we could get this integrated sooner, making it easier to try it out before the cally stuff lands. (Is there a branch/repo of clutter with your a11y stuff integrated somewhere?)

> Just to note that for the first time, after integrate it, I required to use
> NO_GAIL [3] and NO_AT_BRIDGE [4], as mutter was loading already GAIL.

Right. We can set those from the gnome-shell wrapper script I guess? (src/gnome-shell.in).

> For the
> same reason, I needed to set NO_AT_BRIDGE to false after check cally
> initialization, to ensure that the bridge is really loaded.

Probably better to use g_unsetenv() to get rid of it completely? Also, we need to unset NO_GAIL too, because we don't want to pass that on to the shell's children.
Comment 18 Dan Winship 2010-06-18 16:48:27 UTC
Comment on attachment 164018 [details] [review]
Initializes accessibility support

> shell_public_headers_h =		\
>+	shell-a11y.h			\

doesn't need to be public, it's only used internally. Just add both the .c and the .h to libgnome_shell_la_SOURCES.

>@@ -172,6 +173,8 @@ update_font_options (GtkSettings *settings)
>   cairo_antialias_t antialias_mode = CAIRO_ANTIALIAS_NONE;
>   cairo_font_options_t *options;
> 
>+  shell_a11y_init ();

This is a pretty random place to call shell_a11y_init() from... was there a bad git merge or something?

>+  if (init)
>+    method = "gnome_accessibility_module_init";
>+  else
>+    method = "gnome_accessibility_module_shutdown";

you never actually call it with init==FALSE though...

>+  gchar *bridge_path = NULL;
>+  gchar *final_path = NULL;

please use char and int, not gchar and gint. (there might have been some other uses of gchar above)

>+  if (!_should_enable_a11y ())
>+    {
>+      g_debug ("Not enabling gnome-shell a11y.");
>+      return;
>+    }

We still need to unset NO_GAIL and NO_AT_BRIDGE even in that case though. Probably it's safe to do that very first thing?

>+    g_debug ("Enabling gnome-shell a11y.");

btw, probably we just want to get rid of all the g_debugs, assuming that you know the code works at this point.

>+  /* NOTE: We avoid to load gail module, as gail-cally interaction is
>+     not fully supported right now */

/* do comments
 * like this
 */

>+void
>+shell_a11y_init (void);

no newline there in the .h file.
Comment 19 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-06-21 11:51:21 UTC
(In reply to comment #17)

> meaning it won't build against the current clutter, right? (no
> clutter_get_accessibility_enabled () at least?). If you added a configure check
> and an #ifdef we could get this integrated sooner, making it easier to try it
> out before the cally stuff lands. (Is there a branch/repo of clutter with your
> a11y stuff integrated somewhere?)

After a brief talk with ebassi on IRC, I will put a branch with these changes on gitorious.

As this changes will be temporal, and probably not applied in the end, you still need the configure check and the #ifdef?

> > Just to note that for the first time, after integrate it, I required to use
> > NO_GAIL [3] and NO_AT_BRIDGE [4], as mutter was loading already GAIL.
> 
> Right. We can set those from the gnome-shell wrapper script I guess?
> (src/gnome-shell.in).

Ok, this seems a good place to do that, I will start to check how to do it.

> Probably better to use g_unsetenv() to get rid of it completely? Also, we need

Well, use g_unsetenv would also work (in theory), but, why better? Any reason?

> to unset NO_GAIL too, because we don't want to pass that on to the shell's
> children.

Good catch. I have tested it, and it fails (ie launching Orca using Alt+F2), I will update the patch with this change.

(In reply to comment #18)
> (From update of attachment 164018 [details] [review])
> > shell_public_headers_h =		\
> >+	shell-a11y.h			\
> 
> doesn't need to be public, it's only used internally. Just add both the .c and
> the .h to libgnome_shell_la_SOURCES.

Ok

> >@@ -172,6 +173,8 @@ update_font_options (GtkSettings *settings)
> >   cairo_antialias_t antialias_mode = CAIRO_ANTIALIAS_NONE;
> >   cairo_font_options_t *options;
> > 
> >+  shell_a11y_init ();
> 
> This is a pretty random place to call shell_a11y_init() from... was there a bad
> git merge or something?

Probably, I also agree that call shell_a11y_init in update_font_options is somewhat weird. In my original patch, the method was called on gnome_shell_plugin_constructed. It was removed (probably the reason of this randomly placement) on commit 7467e9e3a52dd9df2f45a48d937169475e5d98. I will move that to the method _start

> >+  if (init)
> >+    method = "gnome_accessibility_module_init";
> >+  else
> >+    method = "gnome_accessibility_module_shutdown";
> 
> you never actually call it with init==FALSE though...

Not right now, but just included that if in the future we need to shutdown it. As this is really unlikely, I will simplify that as you suggest.

> >+  gchar *bridge_path = NULL;
> >+  gchar *final_path = NULL;
> 
> please use char and int, not gchar and gint. (there might have been some other
> uses of gchar above)

This is the comment that shocked me. There is any reason of that? After all we are using glib. In the same way there are a lot of other places in the code using gchar and gint. I can change if it is required, just to know why.


> >+  if (!_should_enable_a11y ())
> >+    {
> >+      g_debug ("Not enabling gnome-shell a11y.");
> >+      return;
> >+    }
> 
> We still need to unset NO_GAIL and NO_AT_BRIDGE even in that case though.
> Probably it's safe to do that very first thing?
> 
> >+    g_debug ("Enabling gnome-shell a11y.");
> 
> btw, probably we just want to get rid of all the g_debugs, assuming that you
> know the code works at this point.
> 
> >+  /* NOTE: We avoid to load gail module, as gail-cally interaction is
> >+     not fully supported right now */
> 
> /* do comments
>  * like this
>  */
> 
> >+void
> >+shell_a11y_init (void);
> 
> no newline there in the .h file.

Ok, I agree with these four comments.
Comment 20 Dan Winship 2010-06-21 13:50:33 UTC
(In reply to comment #19)
> As this changes will be temporal, and probably not applied in the end, you
> still need the configure check and the #ifdef?

What won't applied in the end? Oh, you mean the patches here? If we don't know how long it's going to take to get the cally stuff reviewed and merged into the clutter mainline, then we could easily end up committing this patch to gnome-shell first. (Also, we're currently building against clutter-1.2, not master, and I don't know when we're planning to switch over.)

> > Probably better to use g_unsetenv() to get rid of it completely? Also, we need
> 
> Well, use g_unsetenv would also work (in theory), but, why better? Any reason?

Some code might just be doing "if (getenv ("NO_GAIL")) ..." etc, rather than actually looking at its value.

> > please use char and int, not gchar and gint. (there might have been some other
> > uses of gchar above)
> 
> This is the comment that shocked me. There is any reason of that? After all we
> are using glib. In the same way there are a lot of other places in the code
> using gchar and gint. I can change if it is required, just to know why.

The glib types that are just the letter "g" plus a C type name are generally considered to have been a bad idea. While glib itself and some projects like gtk  (and apparently clutter) still use them, many other projects, including gnome-shell, prefer to avoid them and just use the real type names instead. (That only applies to the types that are "g" plus a C type name. guint, gpointer, gboolean, guint32, etc, are all fine to use.)
Comment 21 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-06-21 14:15:28 UTC
(In reply to comment #20)
> (In reply to comment #19)
> > As this changes will be temporal, and probably not applied in the end, you
> > still need the configure check and the #ifdef?
> 
> What won't applied in the end? Oh, you mean the patches here? If we don't know
> how long it's going to take to get the cally stuff reviewed and merged into the
> clutter mainline, then we could easily end up committing this patch to
> gnome-shell first. (Also, we're currently building against clutter-1.2, not
> master, and I don't know when we're planning to switch over.)

Ok, fair enough, so I will try to summarize what I should do now:

* As cally is not integrated yet, give the option to link against it, so add the conditional dependency, as I previously do in a previous patch.
* The configure check will try to detect if clutter provides this method.
* If not, will try to detect if cally is installed.
* If cally is installed, and you have the option, --enable-a11y compile against it.
* Use the #ifdef magic in the code in the proper.

> > > Probably better to use g_unsetenv() to get rid of it completely? Also, we need
> > 
> > Well, use g_unsetenv would also work (in theory), but, why better? Any reason?
> 
> Some code might just be doing "if (getenv ("NO_GAIL")) ..." etc, rather than
> actually looking at its value.

It that case it would be a mistake in that code. But as it really doesn't matter, I would use g_unsetenv.

> > > please use char and int, not gchar and gint. (there might have been some other
> > > uses of gchar above)
> > 
> > This is the comment that shocked me. There is any reason of that? After all we
> > are using glib. In the same way there are a lot of other places in the code
> > using gchar and gint. I can change if it is required, just to know why.
> 
> The glib types that are just the letter "g" plus a C type name are generally
> considered to have been a bad idea. While glib itself and some projects like
> gtk  (and apparently clutter) still use them, many other projects, including
> gnome-shell, prefer to avoid them and just use the real type names instead.
> (That only applies to the types that are "g" plus a C type name. guint,
> gpointer, gboolean, guint32, etc, are all fine to use.)

Ok, thanks for the explanation. I will do that. Anyway, just to comment that there are several places on gnome-shell code using gchar and gint.
Comment 22 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-06-23 12:15:28 UTC
Created attachment 164386 [details] [review]
0001-Added-a11y-initialization-code

This is basically the previous patch but with the updates suggested by Dan Winship, except the configure checks (following patches)
Comment 23 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-06-23 12:15:53 UTC
Created attachment 164387 [details] [review]
0002-Added-some-a11y-checks-on-configure
Comment 24 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-06-23 12:16:14 UTC
Created attachment 164388 [details] [review]
0003-Using-macros-defined-on-configure-checks
Comment 25 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-06-23 12:18:44 UTC
In order to test clutter with cally integrated, you can use branch 2099 that I have uploaded on gitorious:

http://gitorious.org/clutter-cally/clutter-cally

If you want to keep using stable clutter-1.2, you can use cally-1.2 branch:

git://git.clutter-project.org/cally

BTW: clutter bugzilla was moved. Right now old and new 2097 bug are not sync. I will upload patches on the new localization soon.
Comment 26 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-07-05 14:41:03 UTC
FWIW Emmanuele Bassi has added a cally-merge branch on clutter repository:

http://git.clutter-project.org/clutter/log/?h=cally-merge

This branch is the current "official one", although we could still use the gitorious one for tests.

Anyway, my idea would be still update the current isolated cally repository, and make a final "farewell" release of the isolated cally when cally gets integrated on clutter.

I know that right now there isn't any clients of cally (as hildon-desktop still uses a really old cail 0.8 branch), but anyway I would prefer to have a final release of cally, instead of just stop in the middle of the cally integration.

JFYI, a extract from IRC:

Jul 05 16:12:24 <ebassi>	API: the cally-merge branch has all the patches in bugzilla, plus some commits to fix up headers, documentation and other minor things
Jul 05 16:14:57 <API>	ebassi, ok, thanks, so this means that my call-clutter branch on gitorious is officially superfluous
Jul 05 16:14:59 <API>	http://gitorious.org/clutter-cally
Jul 05 16:15:00 <API>	right?
Jul 05 16:16:43 <ebassi>	API: you can use it for development and ask me when to pull in your changes
Jul 05 16:17:05 <ebassi>	we're trying to move away from a development model where everyone has the keys to access clutter-project.org
Jul 05 16:17:47 <API>	ebassi, so when cally get finally integrated, I would need to ask you to apply changes in cally?
Jul 05 16:19:00 <ebassi>	API: we're still experimenting; maybe we'll switch back and put your key on the ACL for core. but for now, we'd like to try a decentralized approach, with the release "manager" doing the integration at given times
Jul 05 16:19:42 <API>	ebassi, ok, thanks
Jul 05 16:20:49 <ebassi>	API: we've already moved to doing all our work on separate branches that I merge before doing the snapshots; ideally, we'd move to per-developer repository on clutter-project.org
Jul 05 16:25:42 <API>	ebassi, will this policy being gently documented anywhere?, like a warning email or a entry in the cookbook?
Jul 05 16:25:54 <ebassi>	API: I'll update the wiki
Jul 05 16:26:09 <API>	ebassi, ok
Comment 27 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-07-08 14:52:40 UTC
Cally has just merged on Clutter master

Should I remove the conditional dependency against cally on configure.ac. I know that this would mean that this change will not enter until mutter/gnome-shell moves to use clutter-1.4, but not sure if it worth maintain that dependency.
Comment 28 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-08-02 10:19:43 UTC
GUADEC conclusions:
  * Setting as obsolete patches related to be able to use cally module
  * Setting dependency with the "porting to clutter 1.4"

I have also removed dependency to the gtk module bug, as current solution doesn't use that anymore.
Comment 29 Dan Winship 2010-08-10 13:11:20 UTC
Comment on attachment 164386 [details] [review]
0001-Added-a11y-initialization-code

this is good to commit now (unless the updated GNOME 3 plan is going to result in some change to the atk-bridge-loading stuff)
Comment 30 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-08-10 14:09:15 UTC
Created attachment 167514 [details] [review]
Little update

After resolution of bug 624571 I tested this again.

The previous patch added a "include <cally.h>"

This was affected by clutter bug http://bugzilla.clutter-project.org/show_bug.cgi?id=2234.

There are already a patch that solves it, but in the same way, this include is not required.

So, instead of wait for the clutter bug resolution (although also necessary), I think that it is easiest just to avoid the include.
Comment 31 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-10-05 10:35:13 UTC
Created attachment 171752 [details] [review]
Updated patch

During the accessibility hackfest I rebased my branch against master and I noticed that this patch doesn't apply.

Little update on the patch to solve this issue.
Comment 32 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-10-26 15:24:16 UTC
Created attachment 173268 [details] [review]
The real update

Sorry, I uploaded a wrong patch in my previous comment. Please ignore it, as it is a patch for a total different project.

This is the correct one.

Anyway, probably due the last changes on module loading using gsettings (something like bug 633191) this patch would require a new review and update.
Comment 33 Mike Gorse 2010-12-17 16:47:46 UTC
I've just added a gsettings schema/key into at-spi2-atk with the location of libatk-bridge.so.  Feel free to tel me to modify it if the namespace is off, etc.
Comment 34 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-12-18 15:24:12 UTC
(In reply to comment #33)
> I've just added a gsettings schema/key into at-spi2-atk with the location of
> libatk-bridge.so.  Feel free to tel me to modify it if the namespace is off,
> etc.

Ok, thanks. That means that I will require to update the current patch. The good new that we will use gsettings instead of gconf also here.
Comment 35 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-12-28 18:12:46 UTC
Created attachment 177153 [details] [review]
Update: use gsettings instead of gconf

As Mike said on comment 33, he recently added a gsettings key with the atk-bridge-location [1] on at-spi2, so I used it.

In the same way, there are already a "accessibility enabled" gsetting (something that I learned first here [2]), so I also used it.

So with this patch, initialize and loading the accessibility modules are gconf free.

Notes:

 * This bridge location patch was just added on at-spi2. I guess that this is the ideal, as GNOME 3.0 target is at-spi2. Anyway, if required for at-spi, we could open a bug on it.
 * org.gnome.desktop.interface.gschema.xml (from gsettings-desktop-schemas) defines the "accessibility" key. But at this moment the Universal Access Settings don't allow you to set/unset it (it allows you to set/unset accessibility). JIK you planned to test use. I will open a bug related to that.


[1] http://git.gnome.org/browse/at-spi2-atk/commit/?id=a16f5306aab7f9d5a8e0fe32fb75deddba175341
[2] https://bugzilla.gnome.org/show_bug.cgi?id=633194
Comment 36 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-12-28 19:01:21 UTC
Created attachment 177154 [details] [review]
Little update: removing a silly debug message

Previous patch included a silly debug message, removed.

Sorry for the noise.
Comment 37 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-12-29 11:54:04 UTC
Created attachment 177187 [details] [review]
Little update: NO_GAIL needs to be unset also

Sorry for the noise
Comment 38 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-01-04 12:05:44 UTC
Created attachment 177470 [details] [review]
Update: rebased patch
Comment 39 Dan Winship 2011-01-05 21:51:08 UTC
Comment on attachment 177470 [details] [review]
Update: rebased patch

>+# Ensure proper NO_GAIL and NO_AT_BRIDGE values for a11y support
>+os.environ['NO_GAIL'] = '1'
>+os.environ['NO_AT_BRIDGE'] = '1'

You don't want to set these in os.environ, or they'll be set when the wrapper script restarts gnome-panel on exit as well. (As well as in Xephyr, dconf, and anything else the wrapper script starts up.)

Instead, set them in the local "env" variable in start_shell(), which is used only for mutter's environment.

Also, the comment could use some more clarification

  # If a11y is enabled, gtk_init() will normally load gail and
  # at-bridge. But we don't want at-bridge to be loaded until after
  # clutter is initialized (which mutter does after initializing gtk)
  # and we don't want gail to be loaded at all. So set these flags.
  # shell_a11y_init() will clear them so they don't get passed to
  # gnome-shell's children.

>+static gboolean
>+_should_enable_a11y (void)

Don't use leading "_" on static methods. Just call it "should_enable_a11y". (Likewise elsewhere.)

>+  atspi_settings = g_settings_new (AT_SPI_SCHEMA);

Due to the way GSettings works, this imposes a hard run-time dependency on AT_SPI_SCHEMA. (If it's not installed, g_settings_new() will abort.) So we either need a configure-time check (as with the desktop schemas), or else get_atk_bridge_path() needs to check g_settings_list_schemas() before calling g_settings_new() (and return a default value if it's not there?).

Although... now that we have the upstream at-spi with the path in GSettings it seems like putting it a pkg-config file would have been better :-}

>+_a11y_invoke_module (const char *module_path)
>+{
>+  GModule    *handle;
>+  void      (*invoke_fn) (void);
>+
>+  if (!module_path)
>+    return FALSE;

(as the code is currently written, module_path will never be NULL, but that could change depending on what you do with get_atk_bridge_path().)

>+  if (!(handle = g_module_open (module_path, G_MODULE_BIND_LAZY)))

don't pass G_MODULE_BIND_LAZY. If the module is compiled against old gtk or something, we want it to fail immediately, not at a random point later on.

>+/*
>+ * It loads the atk-bridge if required. It checks:
>+ *  * If the proper gsetting key is set
>+ *  * If clutter has already enabled the accessibility
>+ */
>+void
>+shell_a11y_init (void)

It would be good to mention here if there are any constraints on what happens before/after this gets run. In particular, is there anything where we have to make sure it doesn't happen until after shell_a11y_init() gets called? (Something like, if we set the initial keyboard focus before calling shell_a11y_init(), then orca wouldn't know about it? I assume that's not actually a problem, but you get the idea.)
Comment 40 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-01-11 15:18:34 UTC
Created attachment 178041 [details] [review]
Update the branch after review on comment 39

(In reply to comment #39)

> Instead, set them in the local "env" variable in start_shell(), which is used
> only for mutter's environment.
> 
> Also, the comment could use some more clarification
> 
>   # If a11y is enabled, gtk_init() will normally load gail and
>   # at-bridge. But we don't want at-bridge to be loaded until after
>   # clutter is initialized (which mutter does after initializing gtk)
>   # and we don't want gail to be loaded at all. So set these flags.
>   # shell_a11y_init() will clear them so they don't get passed to
>   # gnome-shell's children.

Done, I used that text in order to explain it.

> >+static gboolean
> >+_should_enable_a11y (void)
> 
> Don't use leading "_" on static methods. Just call it "should_enable_a11y".
> (Likewise elsewhere.)

Ok

> >+  atspi_settings = g_settings_new (AT_SPI_SCHEMA);
> 
> Due to the way GSettings works, this imposes a hard run-time dependency on
> AT_SPI_SCHEMA. (If it's not installed, g_settings_new() will abort.) So we
> either need a configure-time check (as with the desktop schemas), or else
> get_atk_bridge_path() needs to check g_settings_list_schemas() before calling
> g_settings_new() (and return a default value if it's not there?).

In the end I opted for the second option, use g_settings_list_schemas checking if we have that schema there.

Anyway, this "g_settings_new will abort if the schema is not installed" is normal? Will be the behaviour of g_settings_new for now and the future?

> Although... now that we have the upstream at-spi with the path in GSettings it
> seems like putting it a pkg-config file would have been better :-}

Well, using gsettings for this is easy. BTW that it was just installed on the upstream at-spi2. And as we are nearer and nearer of the deadline, it seems that most distributions will still opt for at-spi. I need to create a bug (or patch) on at-spi in order to also apply this change.

> >+_a11y_invoke_module (const char *module_path)
> >+{
> >+  GModule    *handle;
> >+  void      (*invoke_fn) (void);
> >+
> >+  if (!module_path)
> >+    return FALSE;
> 
> (as the code is currently written, module_path will never be NULL, but that
> could change depending on what you do with get_atk_bridge_path().)

Well, module path would be NULL if for any reason the bridge key had as value NULL. Anyway, now it is more possible (if the schema is missing). I also added a warning message in that case (the other error cases printed a g_warning, while in this case it failed without saying why)


> >+  if (!(handle = g_module_open (module_path, G_MODULE_BIND_LAZY)))
> 
> don't pass G_MODULE_BIND_LAZY. If the module is compiled against old gtk or
> something, we want it to fail immediately, not at a random point later on.

Ok, done.

> >+/*
> >+ * It loads the atk-bridge if required. It checks:
> >+ *  * If the proper gsetting key is set
> >+ *  * If clutter has already enabled the accessibility
> >+ */
> >+void
> >+shell_a11y_init (void)
> 
> It would be good to mention here if there are any constraints on what happens
> before/after this gets run. In particular, is there anything where we have to
> make sure it doesn't happen until after shell_a11y_init() gets called?
> (Something like, if we set the initial keyboard focus before calling
> shell_a11y_init(), then orca wouldn't know about it? I assume that's not
> actually a problem, but you get the idea.)

Well, I have just added that the atk-bridge can't be loaded before call this method.

I can't think right now on a postcondition of this call, I could add later if I remember that.

About your example: as you said this is not the case. Orca reacts to the event window::activate to know which app is active, and then if required, ask for the current focused object.
Comment 41 André Klapper 2011-01-16 22:23:50 UTC
Setting GNOME Target field to 3.0 as per Piñeiro's post at https://mail.gnome.org/archives/gnome-accessibility-list/2011-January/msg00046.html
Comment 42 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-01-16 23:54:27 UTC
(In reply to comment #41)
> Setting GNOME Target field to 3.0 as per Piñeiro's post at
> https://mail.gnome.org/archives/gnome-accessibility-list/2011-January/msg00046.html

Although it is possible that this patch will be applied for GNOME 3.0, taking into account the tight schedule for GNOME 3.0, and other tasks required to solve (some listed here [1]), AFAIK, accessibility support for GNOME Shell are planned for following cycles.

So I guess that it would be more suitable setting GNOME Target field to 3.2.

This also applies for bug 626658 and bug 636717

[1] http://live.gnome.org/GnomeShell/RoadmapTwoThirtyOne
Comment 43 Dan Winship 2011-01-18 15:02:42 UTC
Comment on attachment 178041 [details] [review]
Update the branch after review on comment 39

Good to commit with just a few tweaks

>+  list_schemas = g_settings_list_schemas ();

"list_schemas" is an odd name for it... I'd just call it "schemas"

>+      if (!g_strcmp0 (list_schemas[i], AT_SPI_SCHEMA))

and you don't need strcmp0 here; regular strcmp should be fine.

>+  if (!module_path)
>+    {
>+      g_warning ("Accessibility: invalid module path (NULL)");
>+
>+      return FALSE;
>+    }

So this can only happen if the at-spi schema isn't found. (g_settings_get_string() never returns NULL.) So you can have a clearer error message here explaining that. ("Accessibility is enabled, but at-spi is not installed" ?)
Comment 44 Bastien Nocera 2011-01-18 15:14:36 UTC
(In reply to comment #43)
> (From update of attachment 178041 [details] [review])
<snip>
> >+      if (!g_strcmp0 (list_schemas[i], AT_SPI_SCHEMA))
> 
> and you don't need strcmp0 here; regular strcmp should be fine.

g_str_equal()?
Comment 45 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-01-18 15:19:11 UTC
(In reply to comment #43)
> (From update of attachment 178041 [details] [review])
> Good to commit with just a few tweaks
> 
> >+  list_schemas = g_settings_list_schemas ();
> 
> "list_schemas" is an odd name for it... I'd just call it "schemas"
> 
> >+      if (!g_strcmp0 (list_schemas[i], AT_SPI_SCHEMA))
> 
> and you don't need strcmp0 here; regular strcmp should be fine.

Ok, I will change both.

> >+  if (!module_path)
> >+    {
> >+      g_warning ("Accessibility: invalid module path (NULL)");
> >+
> >+      return FALSE;
> >+    }
> 
> So this can only happen if the at-spi schema isn't found.
> (g_settings_get_string() never returns NULL.) So you can have a clearer error
> message here explaining that. ("Accessibility is enabled, but at-spi is not
> installed" ?)

This warning is shown by a11y_invoke_module, and as this method just tries to invoke a method on that module. So in the context of the function, that message is right. 

Anyway I will add a warning on shell_a11y init (after all a11y_invoke_module has a return value, we can use it), with a text like "System Accessibility is enabled, Clutter Accessibility is enabled, but we were not able to load the atk-bridge", as this is what happens there.

Finally, as you suggested a "at-spi is not installed message", I think that this make more sense on get_atk_bridge_path. If you don't find the schema, is a hint that at-spi is not installed.
Comment 46 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-01-18 15:21:16 UTC
(In reply to comment #44)
> (In reply to comment #43)
> > (From update of attachment 178041 [details] [review] [details])
> <snip>
> > >+      if (!g_strcmp0 (list_schemas[i], AT_SPI_SCHEMA))
> > 
> > and you don't need strcmp0 here; regular strcmp should be fine.
> 
> g_str_equal()?

From g_str_equal doc:

"Note that this function is primarily meant as a hash table comparison function. For a general-purpose, NULL-safe string comparison function, see g_strcmp0()."

So g_str_equal doesn't seems the best option here, and g_strcmp0 would be better.

But as Dan Winship prefers strcmp, and there isn't too much difference, I will use that instead.
Comment 47 Dan Winship 2011-01-18 15:39:03 UTC
(In reply to comment #44)
> > and you don't need strcmp0 here; regular strcmp should be fine.
> 
> g_str_equal()?

no, just plain ISO C strcmp() is fine
Comment 48 Owen Taylor 2011-01-18 15:47:24 UTC
(In reply to comment #46)
> (In reply to comment #44)
> > (In reply to comment #43)
> > > (From update of attachment 178041 [details] [review] [details] [details])
> > <snip>
> > > >+      if (!g_strcmp0 (list_schemas[i], AT_SPI_SCHEMA))
> > > 
> > > and you don't need strcmp0 here; regular strcmp should be fine.
> > 
> > g_str_equal()?
> 
> From g_str_equal doc:
> 
> "Note that this function is primarily meant as a hash table comparison
> function. For a general-purpose, NULL-safe string comparison function, see
> g_strcmp0()."
> 
> So g_str_equal doesn't seems the best option here, and g_strcmp0 would be
> better.
> 
> But as Dan Winship prefers strcmp, and there isn't too much difference, I will
> use that instead.

It is a specific GLib development policy that functions that exactly duplicate C library functions aren't added. So there's no g_strcmp() - not because g_strcmp0() is better - but because there is strcmp()
Comment 49 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-01-20 12:15:25 UTC
Created attachment 178831 [details] [review]
Update the patch after review on comment 43
Comment 50 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-01-20 14:55:48 UTC
Taking into account the flag "accepted-commit_now" on the previous patch, and after a brief chat with Dan Winship on IRC, patch applied.

Close bug as FIXED

Thanks!