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 626658 - StLabel doesn't expose any accessibility information
StLabel doesn't expose any accessibility information
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Dan Winship
gnome-shell-maint
Depends on: 636717
Blocks: 626660 634016
 
 
Reported: 2010-08-11 18:39 UTC by Alejandro Piñeiro Iglesias (IRC: infapi00)
Modified: 2011-01-20 14:55 UTC
See Also:
GNOME target: 3.0
GNOME version: ---


Attachments
Implement atk_object_get_name (5.27 KB, patch)
2010-08-11 18:39 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
reviewed Details | Review
Updated patch (6.27 KB, patch)
2010-10-15 14:52 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
Updated patch (6.00 KB, patch)
2010-12-07 18:42 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
reviewed Details | Review
Updated patch after Dan Winship review and bug 636717 update (5.15 KB, patch)
2010-12-29 16:10 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
reviewed Details | Review
Updated patch after review on comment 17 (4.78 KB, patch)
2011-01-11 15:30 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
accepted-commit_now Details | Review
Update patch after review on comment 20 (4.78 KB, patch)
2011-01-20 12:23 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review

Description Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-08-11 18:39:16 UTC
Created attachment 167648 [details] [review]
Implement atk_object_get_name

It you try to explore the accessibility hierarchy using accerciser or directly py-atspi, you expect to several named objects. The more evident is the "Activities" label.

As explained on bug 614121, this is because StLabel has a inner ClutterText but doesn't expose his information at all.

As at this moment StLabel has the intention to be a simple label, without all the functionalities of GtkLabel, and following AtkText documentation advice:

"AtkText should be implemented by AtkObjects on behalf of widgets that have text content which is either attributed or otherwise non-trivial. AtkObjects whose text content is simple, unattributed, and very brief may expose that content via atk_object_get_name instead; "

The patch attached just do that, expose the name of the label as the inner ClutterText text.

If StLabel becomes (or is right now) more complex that that, we have two options:
  * Expose the ClutterText as the StLabel child
  * Implement AtkText on StLabel redirecting directly to ClutterText AtkText implementation. This avoiding showing a (somewhat artificial) parent-child relation.
Comment 1 Dan Winship 2010-08-19 15:51:38 UTC
Comment on attachment 167648 [details] [review]
Implement atk_object_get_name

>"AtkText should be implemented by AtkObjects on behalf of widgets that
>have text content which is either attributed or otherwise
>non-trivial. AtkObjects whose text content is simple, unattributed,
>and very brief may expose that content via atk_object_get_name
>instead;"

assuming "attributed" means <b>pango <i>attributes</i></b>, then you can use those with StLabel, and we already do in some places. (eg, the message tray)

Also, "stally" just sounds ridiculous to my ears. I'd prefer "StLabelAccessible" or "StLabelA11y" or even "StA11yLabel"
Comment 2 Owen Taylor 2010-08-19 15:59:10 UTC
Can someone explain why we'd have both a label and an accessible label?
Comment 3 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-08-19 16:41:02 UTC
(In reply to comment #1)
> (From update of attachment 167648 [details] [review])
> >"AtkText should be implemented by AtkObjects on behalf of widgets that
> >have text content which is either attributed or otherwise
> >non-trivial. AtkObjects whose text content is simple, unattributed,
> >and very brief may expose that content via atk_object_get_name
> >instead;"
> 
> assuming "attributed" means <b>pango <i>attributes</i></b>, then you can use
> those with StLabel, and we already do in some places. (eg, the message tray)

ATK defines his own attribute set [3] as pretend to be as abstract as possible. In the underlying layers (GAIL) that means represent pango attributes as AtkAttributes that you can get using some AtkText methods [1][2]

But what Im trying to say here is as StLabel is a really basic text-representation object (IMHO), we don't need to expose all that information, and exposing just the name would be enough. My patch just does that, expose the label text as the name (it still misses a notify event for the label text changes, but we can fix that in other bug/patch).

> Also, "stally" just sounds ridiculous to my ears. I'd prefer
> "StLabelAccessible" or "StLabelA11y" or even "StA11yLabel"

I just used Stally using clutter scheme, for coherence. Clutter is the toolkit. Cally (ally instead of a11y to avoid numbers) is the Clutter a11y support class.

Initially I used GAIL and HAIL scheme (Gnome Accessibility Implementation Library, Hildon Accessibility Implementation Library), but CAIL was a registered name, so I renamed the library.

In the same way, if we apply this scheme to St, we would get SAIL, that would be more ridiculous (IMHO) (although HAIL is somewhat a funny name also).

But no problem to use a different scheme as far as we use the same for all the hypothetical St accessibility objects. Not sure why StA11y would be better than Stally. StLabelAccessible is good enough for me (ie: GtkSpinner defines a GtkSpinnerAccessible object).

I can update the name with that if you want.


[1] http://library.gnome.org/devel/atk/stable/AtkText.html#atk-text-get-run-attributes
[2] http://library.gnome.org/devel/atk/stable/AtkText.html#atk-text-get-default-attributes
[3] http://library.gnome.org/devel/atk/stable/AtkText.html#AtkAttributeSet
Comment 4 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-08-19 16:53:33 UTC
(In reply to comment #2)
> Can someone explain why we'd have both a label and an accessible label?

Short answer: AtkObject is not a interface.

Long answer:

The current way to provide accessibility support on GNOME with ATK is provide a hierarchy of accessibility objects representing the class hierarchy on the application. You can see that the accessibility objects are a kind of proxy of the objects of the application.

One of the main reasons of that is provide a accessibility hierarchy with meaningful objects, from the accessibility point of view.

I guess that the reason of this question is somewhat like "why not implement AtkText directly on StLabel instead of having two objects?". This issue was also pointed out during the cally integration discussion, as when Emmanuele Bassi first commented that idea, I thought that he was suggesting the same.

So you can read a more detailed explanation in this bug comment [1] (curiously using StLabel as example), and in this gnome-accessibility mail [2] (unfortunately the mail archives doesn't include EBassi mail).


[1] https://bugzilla.gnome.org/show_bug.cgi?id=614121#c4
[2] http://mail.gnome.org/archives/gnome-accessibility-devel/2010-March/msg00003.html
Comment 5 Owen Taylor 2010-08-19 17:32:15 UTC
gtk_widget_get_accessible() was my design for how we could enable ATK to be implemented for GTK+ 2.0.0 when the work was begun just a few months before we had to release a stable GTK+ 2.0.0.

The way it was *supposed* to work, was:

 A) if an object had a built-in implementation of accessibility, it would create an object and return it. (Or potentially just return itself if it wanted to implement the ATK interfaces directly.)

 B) if it didn't, then it would fall back to a default implementation in GtkWidget which would create the accessible object in alternate ways that would be extensible through the GtkModule system.

Looking at GtkIconView, it appears that something was done in ATK that made going through the factory system mandatory, so GtkIconView has to do gross hacks to register its own factory the first time gtk_icon_view_get_accessible() is called. (This is separate from the AtkObject-is-not-an-interface issue.)

Without fixing ATK, it doesn't look like taking the GtkIconView approach is useful. It's just far too much ugly per-widget code. ATK definitely *should* be fixed. I don't know if it is feasible to fix it while retaining compatibility.

Leaving that all aside:

 Everything inside the St library has the St namespace. There will be no "Stally" namespace. If there is some aggregate object that we need to register with the factory, then it's StLabelAccessible or whatever *inside the St namespace*.
Comment 6 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-08-20 11:50:10 UTC
(In reply to comment #5)
> gtk_widget_get_accessible() was my design for how we could enable ATK to be
> implemented for GTK+ 2.0.0 when the work was begun just a few months before we
> had to release a stable GTK+ 2.0.0.
> 
> The way it was *supposed* to work, was:
> 
>  A) if an object had a built-in implementation of accessibility, it would
> create an object and return it. (Or potentially just return itself if it wanted
> to implement the ATK interfaces directly.)
> 
>  B) if it didn't, then it would fall back to a default implementation in
> GtkWidget which would create the accessible object in alternate ways that would
> be extensible through the GtkModule system.

Hmm, now I don't understand your question at comment 2. Supposing that Gtk and Clutter were working as you are saying in A) and B), you still need a label and a accessibility object that represents that label. Could you elaborate that question?

> Looking at GtkIconView, it appears that something was done in ATK that made
> going through the factory system mandatory, so GtkIconView has to do gross
> hacks to register its own factory the first time gtk_icon_view_get_accessible()
> is called. (This is separate from the AtkObject-is-not-an-interface issue.)

Well, on gtk_widget (and clutter_actor) B) is implemented by the factory system. GAIL and Cally defines a stack of widgets. When both are initialized all the factories are initialized so gtk_widget_get_accessible can use that to return the object.

The custom ->get_accessible methods basically register the factory if required and then delegates on the parent get_accessible. Probably this is somewhat hackish, but not sure why a gross hack. This leads to create the object always in the same way (homogeneity).

Although I don't have all the background I think that the main reason for that is that the accessible object can be already cached. Your A) and B) misses a detail. It is not just "create a object" is "return the related accessible object, create it if required". gtk_widget_get_accessible and atk_gobject_accessible_for_object (I used that on Clutter) maintains the accessibility object, so once it is created, it is returned always the same object.

As far as I see, we could avoid the factory thing on StLabel (or others), but in this case StLabel would require to maintain the cached accessible object. As part of his private structure, or using g_object_[set/get]_qdata, as GtkWidget or AtkGObject does. But we would require to do that for any (hypothetical) object defining his custom accessible object. It is debatable if replicating this accessible-object-maintenance code on different ST classes would more more hackish that this "use the factory as the only way to create accessible objects"

> Without fixing ATK, it doesn't look like taking the GtkIconView approach is
> useful. It's just far too much ugly per-widget code. ATK definitely *should* be
> fixed. I don't know if it is feasible to fix it while retaining compatibility.

What do you want to fix in ATK?

> Leaving that all aside:
> 
>  Everything inside the St library has the St namespace. There will be no
> "Stally" namespace. If there is some aggregate object that we need to register
> with the factory, then it's StLabelAccessible or whatever *inside the St
> namespace*.

Ok, fair enough, I will update the patch.
Comment 7 Owen Taylor 2010-08-23 17:14:07 UTC
(In reply to comment #6)
> Hmm, now I don't understand your question at comment 2. Supposing that Gtk and
> Clutter were working as you are saying in A) and B), you still need a label and
> a accessibility object that represents that label. Could you elaborate that
> question?

My question at comment 2 was a genuine question. I needed an explanation. Given that explanation (and "AtkObject is not an interface" is a fairly strong reason), I still had the comments here.

> > Looking at GtkIconView, it appears that something was done in ATK that made
> > going through the factory system mandatory, so GtkIconView has to do gross
> > hacks to register its own factory the first time gtk_icon_view_get_accessible()
> > is called. (This is separate from the AtkObject-is-not-an-interface issue.)
> 
> Well, on gtk_widget (and clutter_actor) B) is implemented by the factory
> system. GAIL and Cally defines a stack of widgets. When both are initialized
> all the factories are initialized so gtk_widget_get_accessible can use that to
> return the object.
> 
> The custom ->get_accessible methods basically register the factory if required
> and then delegates on the parent get_accessible. Probably this is somewhat
> hackish, but not sure why a gross hack. This leads to create the object always
> in the same way (homogeneity).

If it's not clear why it's a gross hack, I'm not really sure how to explain it. But if the GTK+ hook was meant to work that way, it would have been called register_accessible_factory() and called once per class, not called get_accessible().

> Although I don't have all the background I think that the main reason for that
> is that the accessible object can be already cached. Your A) and B) misses a
> detail. It is not just "create a object" is "return the related accessible
> object, create it if required". gtk_widget_get_accessible and
> atk_gobject_accessible_for_object (I used that on Clutter) maintains the
> accessibility object, so once it is created, it is returned always the same
> object.

Well, I would expect is:

 AtkObject *
 st_label_get_accessible(StLabel *label) {
    StLabelPrivate *priv = label->priv;

    if (priv->accessible == NULL)
       priv->accessible = st_label_accessible_new (label);

    return priv->accessible;
 }

> As far as I see, we could avoid the factory thing on StLabel (or others), but
> in this case StLabel would require to maintain the cached accessible object. As
> part of his private structure, or using g_object_[set/get]_qdata, as GtkWidget
> or AtkGObject does. But we would require to do that for any (hypothetical)
> object defining his custom accessible object. It is debatable if replicating
> this accessible-object-maintenance code on different ST classes would more more
> hackish that this "use the factory as the only way to create accessible
> objects"

The caching could have been done in GtkWidget too, and the vfunction called create_accessible() if a couple lines of code like the above is considered excessive. But I don't see any reason for forcing creating a whole new factory type!
 
> > Without fixing ATK, it doesn't look like taking the GtkIconView approach is
> > useful. It's just far too much ugly per-widget code. ATK definitely *should* be
> > fixed. I don't know if it is feasible to fix it while retaining compatibility.
> 
> What do you want to fix in ATK?

Make it possible to implement accessibility for a widget or an actor in a few lines of code.
Comment 8 Owen Taylor 2010-08-23 17:20:20 UTC
(In reply to comment #7)
> > > Without fixing ATK, it doesn't look like taking the GtkIconView approach is
> > > useful. It's just far too much ugly per-widget code. ATK definitely *should* be
> > > fixed. I don't know if it is feasible to fix it while retaining compatibility.
> > 
> > What do you want to fix in ATK?
> 
> Make it possible to implement accessibility for a widget or an actor in a few
> lines of code.

Actually, even more importantly than "few lines of code" is the ability to write a closely linked pair of a widget and it's accessible object. To allow them to share private data. So that the widget can say if (priv->accessible) { <do something to the acesssible object> } rather than adding a signal that the accessible object connects to, etc. I haven't spent a lot of time with Gail, but what I have seen contains a lot to make one shudder. (And the performance of GTK+ accessibility is basically unacceptable.) Writing widget and accessible implementations together allows coherent design and performance that is only limited by the problematic parts of the at-spi protocol.
Comment 9 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-08-24 08:17:41 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > Hmm, now I don't understand your question at comment 2. Supposing that Gtk and
> > Clutter were working as you are saying in A) and B), you still need a label and
> > a accessibility object that represents that label. Could you elaborate that
> > question?
> 
> My question at comment 2 was a genuine question. I needed an explanation. Given
> that explanation (and "AtkObject is not an interface" is a fairly strong
> reason), I still had the comments here.

I wasn't suggesting your question to not be genuine. I was saying that I didn't understand it. Sorry if I sounded rude. That was not my intention.

> If it's not clear why it's a gross hack, I'm not really sure how to explain it.
> But if the GTK+ hook was meant to work that way, it would have been called
> register_accessible_factory() and called once per class, not called
> get_accessible().

My explanation was just a guess of why that was working in this way. But I'm not saying that the GTK+ hook was meant to work that way. I'm basically following the general guidelines that I can extract from the current code.

> Well, I would expect is:
> 
>  AtkObject *
>  st_label_get_accessible(StLabel *label) {
>     StLabelPrivate *priv = label->priv;
> 
>     if (priv->accessible == NULL)
>        priv->accessible = st_label_accessible_new (label);
> 
>     return priv->accessible;
>  }

That should work, as far as we keep calling clutter_actor_get_accessible to get the object, and not suppose that we can call always use the factory for that (not the case).

> > What do you want to fix in ATK?
> 
> Make it possible to implement accessibility for a widget or an actor in a few
> lines of code.

From the patch attached, you only need two lines to define and use the factory:

CALLY_ACCESSIBLE_FACTORY (STALLY_TYPE_LABEL, stally_label, stally_label_new)
CALLY_ACTOR_SET_FACTORY (ST_TYPE_LABEL, stally_label);

Anyway, as you said, the collateral effect of this is that you are creating a new type just for the factory.

Fair enough. Avoid a new factory type if it is not really required, and the other comments, seems a good reason to change this.

AFAIK, there isn't any thing in ATK to fix. You could create the accessible object as you said. At least I will check it and update the patch.

Thanks for the suggestions.
Comment 10 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-08-24 08:39:33 UTC
(In reply to comment #8)
> (In reply to comment #7)

> > > What do you want to fix in ATK?
> > 
> > Make it possible to implement accessibility for a widget or an actor in a few
> > lines of code.
> 
> Actually, even more importantly than "few lines of code" is the ability to
> write a closely linked pair of a widget and it's accessible object. To allow
> them to share private data. So that the widget can say if (priv->accessible) {
> <do something to the acesssible object> } rather than adding a signal that the
> accessible object connects to, etc. I haven't spent a lot of time with Gail,
> but what I have seen contains a lot to make one shudder. (And the performance
> of GTK+ accessibility is basically unacceptable.)

Yeah, in some cases it is unacceptable. The poster boy is GailTreeView [1][2][3], and the workaround suggested somewhat artificial.

> Writing widget and accessible
> implementations together allows coherent design and performance that is only
> limited by the problematic parts of the at-spi protocol.

Ok, we would try to avoid those problems here, and in the future, I would try to review Cally in order to check if it would possible/required to improve Cally-Clutter integration.

Although define Cally, or ClutterActorAccessible types as public would still required, in order to keep extending them.


[1] https://bugzilla.gnome.org/show_bug.cgi?id=575696
[2] https://bugzilla.gnome.org/show_bug.cgi?id=577098
[3] https://bugzilla.gnome.org/show_bug.cgi?id=571596
Comment 11 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-10-15 14:52:40 UTC
Created attachment 172429 [details] [review]
Updated patch

Updated patch, using all the suggestions on previous comments.

Sorry for the delay. While working on this bug I started to think if it would be a good idea to change CallyActor parent [1]. At this moment it is discarded, as that would mean too many changes.

Anyway, good to read, as this explain why in this patch st-label doesn't call g_object_unref on the dispose.


[1] http://bugzilla.clutter-project.org/show_bug.cgi?id=2371
Comment 12 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-12-07 18:42:26 UTC
Created attachment 176023 [details] [review]
Updated patch

With bug 636717 accessibility support was included to StWidget. So now the parent of StLabelAccessible should be StWidgetAccessible and not CallyActor. It also make other minor updates
Comment 13 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-12-07 18:46:45 UTC
I  have just removed the dependency to bug 612599, because formally, you can implement the accessibility support on St without loading the accessibility modules. But or course, without loading the accessibility support, no AT app (like Orca) would enjoy this a11y support.
Comment 14 Dan Winship 2010-12-21 22:32:06 UTC
Comment on attachment 176023 [details] [review]
Updated patch

>If someone wonders why not implement AtkText interface, or expose the
>internal ClutterText, here a extract from AtkText doc:
>
>"AtkText should be implemented by AtkObjects on behalf of widgets that
>have text content which is either attributed or otherwise
>non-trivial. AtkObjects whose text content is simple, unattributed,
>and very brief may expose that content via atk_object_get_name
>instead;"

StLabel does allow using attributes though. Eg, in the message tray we use bold text (and italic and underline for notifications that specify it).
Comment 15 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-12-23 09:55:50 UTC
(In reply to comment #14)
> (From update of attachment 176023 [details] [review])
> >If someone wonders why not implement AtkText interface, or expose the
> >internal ClutterText, here a extract from AtkText doc:
> >
> >"AtkText should be implemented by AtkObjects on behalf of widgets that
> >have text content which is either attributed or otherwise
> >non-trivial. AtkObjects whose text content is simple, unattributed,
> >and very brief may expose that content via atk_object_get_name
> >instead;"
> 
> StLabel does allow using attributes though. Eg, in the message tray we use bold
> text (and italic and underline for notifications that specify it).

So from the list of reasons to not implement AtkText ("simple, unattributed, very brief") only one is not fulfilled ;) 

Anyway, we can see this patch as the first stone. It defines the accessibility object, and expose the text as the name, something that it would be a good idea to do with or without a full AtkText implementation (as with GtkLabel).

In the future we can check if it is worth to fully implement AtkText (that in my opinion should be just a wrap to the inner ClutterText) but, IMHO, this should be done on a different bug.

Anyway, the patch still requires the "virtual _get_type to instantiate the accessible object" solution update as commented on bug 636717.
Comment 16 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-12-29 16:10:12 UTC
Created attachment 177197 [details] [review]
Updated patch after Dan Winship review and bug 636717 update
Comment 17 Dan Winship 2011-01-06 16:00:09 UTC
Comment on attachment 177197 [details] [review]
Updated patch after Dan Winship review and bug 636717 update

>Fixes NB#626658

"NB"? What's the "N" stand for?

Anyway, our standard (which you should do on the other patches too; I don't remember if I checked) is to just end the commit message with the complete bug URL on a line by itself.

>+  widget_class->get_accessible_type = st_label_get_accessible_type;

if you made the change I suggested in the StWidget bug, this would become

    widget_class->get_accessible_type = st_label_accessible_get_type;

and st_label_get_accessible_type() would go away.

>+  /* < private > */
>+};

Same spacing issue as in the StWidget bug, though since this struct isn't public anyway, there's really no reason to bother with that comment.

>+  AtkObjectClass *class         = ATK_OBJECT_CLASS (klass);

don't have "class" and "klass" both. Call it "atk_class" or something.

>+  /* most of the initalization done on AtkObject->initiliaze */

typo at the end. Also, s/most/all/ ? :-)

>+      actor = CLUTTER_ACTOR (atk_gobject_accessible_get_object (ATK_GOBJECT_ACCESSIBLE (obj)));
>+      /* this call is somewhat ugly, probably it would be good to
>+       * define a cally_actor_get_actor wrap:
>+       *
>+       * actor = cally_actor_get_actor (CALLY_ACTOR (obj));
>+       */

ok, go do that and then send us a patch to fix it here. But don't leave TODO notes for yourself in the St code. :)
Comment 18 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-01-11 15:30:20 UTC
Created attachment 178045 [details] [review]
Updated patch after review on comment 17

(In reply to comment #17)
> (From update of attachment 177197 [details] [review])
> >Fixes NB#626658
> 
> "NB"? What's the "N" stand for?

NB stands for *G*nome *B*ug but with a typo, sorry.

> Anyway, our standard (which you should do on the other patches too; I don't
> remember if I checked) is to just end the commit message with the complete bug
> URL on a line by itself.

Done, also applied on Bug 612599 and Bug 636717.

> >+  widget_class->get_accessible_type = st_label_get_accessible_type;
> 
> if you made the change I suggested in the StWidget bug, this would become
> 
>     widget_class->get_accessible_type = st_label_accessible_get_type;
> 
> and st_label_get_accessible_type() would go away.

Yep, done.

> 
> >+  /* < private > */
> >+};
> 
> Same spacing issue as in the StWidget bug, though since this struct isn't
> public anyway, there's really no reason to bother with that comment.

Yes, removed.

> >+  AtkObjectClass *class         = ATK_OBJECT_CLASS (klass);
> 
> don't have "class" and "klass" both. Call it "atk_class" or something.

Yes, it makes sense. This was also a problem on the StWidget patch (bug 636717), I also solved that there.

> >+  /* most of the initalization done on AtkObject->initiliaze */
> 
> typo at the end. Also, s/most/all/ ? :-)

Done (typo) and done (although I just made shorter the comment).

> >+      actor = CLUTTER_ACTOR (atk_gobject_accessible_get_object (ATK_GOBJECT_ACCESSIBLE (obj)));
> >+      /* this call is somewhat ugly, probably it would be good to
> >+       * define a cally_actor_get_actor wrap:
> >+       *
> >+       * actor = cally_actor_get_actor (CALLY_ACTOR (obj));
> >+       */
> 
> ok, go do that and then send us a patch to fix it here. But don't leave TODO
> notes for yourself in the St code. :)

;), added to my TODO list, removed from the patch.
Comment 19 André Klapper 2011-01-16 22:23:53 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 20 Dan Winship 2011-01-18 15:37:22 UTC
Comment on attachment 178045 [details] [review]
Updated patch after review on comment 17

good except:

>+  /* initialization done on AtkObject->initialiaze */

you only half-fixed the typo :-) ("initialiAze")
Comment 21 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-01-18 16:47:05 UTC
(In reply to comment #20)
> (From update of attachment 178045 [details] [review])
> good except:
> 
> >+  /* initialization done on AtkObject->initialiaze */
> 
> you only half-fixed the typo :-) ("initialiAze")

urgh, ok, sorry
Comment 22 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-01-20 12:23:14 UTC
Created attachment 178833 [details] [review]
Update patch after review on comment 20
Comment 23 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-01-20 14:55:58 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!