Code review comment for lp:~rodrigo-moya/unity/add-panel-service-a11y

> 154 + if (!a11y_initialized)
> 155 + {
>
> I think that it is worth to check the opposite, return if a11y_initialized.
> That would avoid a indentation level (IMHO, code would be clearer).
>
fixed

> 164 + /* Restore environment to load AT bridge */
> 165 + g_unsetenv ("NO_AT_BRIDGE");
> 166 + g_unsetenv ("NO_GAIL");
>
> Take into account that there are one case (!should_enable_a11y) where you
> don't restore the environment, *but*, you modify the environment alway. Sorry,
> I was the one that suggested to move that to just before the load bridge, but
> probably this should be done as the first thing, to ensure that you always
> restore the environment (although this would only be a problem if there are a
> process child of this service, unlikely).
>
done

>
> 299 + atk_object_set_name (accessible, _("An indicator")); /* FIXME */
>
> Do you really thing that worths to add this temporal names. Are useful for
> testing during development, but not sure if it is worth on have that on the
> actual code. If in the end you remove it, you will not require this include:
> 240 +#include <glib/gi18n.h>
>
in the next branch I will remove that temporary code, with real code to return the indicators. So just added this so that it shows something in accerciser.

>
> 294 +static void
> 295 +panel_indicator_accessible_initialize (AtkObject *accessible,
> gpointer data)
> 296 +{
> 297 + g_return_if_fail (PANEL_IS_INDICATOR_ACCESSIBLE (accessible));
> 298 +
> 299 + atk_object_set_name (accessible, _("An indicator")); /* FIXME */
> 300 + atk_object_set_role (accessible, ATK_ROLE_LABEL);
> 301 +
> 302 + ATK_OBJECT_CLASS
> (panel_indicator_accessible_parent_class)->initialize (accessible, data);
>
> Although cally-root also calls the parent class initialize as the last thing,
> by default this should be done as the first thing of the initialize. Ie:
> imagine that the parent class also set a role, in that case that
> ATK_ROLE_LABEL would be unused.
>
right, fixed

>
> 508 + accessible->role = ATK_ROLE_APPLICATION;
> 509 + atk_object_set_name (accessible, _("Unity Panel Service"));
>
> As the root represents the application I think that you should use the
> application name. Here you can use g_get_prgname(), like in gail or cally. I
> couldn't use that on Unity because I didn't found a equivalent to
> g_get_prgname to be used there (anyway, this is FIXME).
>
ok, fixed

> Some extra comments:
>
> * The indentation is somewhat weird. It seems that in some cases you have
> spaces and in other you have tabs, ie:
> 475 + atk_class->initialize = panel_root_accessible_initialize;
> 476 + atk_class->get_n_children = panel_root_accessible_get_n_children;
> 477 + atk_class->ref_child = panel_root_accessible_ref_child;
> 478 + atk_class->get_parent = panel_root_accessible_get_parent;
>
yes, seems I got the format header wrong, so I've removed it from all the files and fixed (hopefully) all TABs

>
> * The way we reuse gail here is allow gtk_init to load gail but not the
> bridge, and then load the bridge. I don't like too much this mixed approach,
> as it could be prone-error (what would happen if for any reason gtk_init
> doesn't load the accessibility but panel-service yes, or the opposite). Taking
> into account the current wisdom (firefox and so on), probably we should do all
> the work. That means setting both NO_GAIL and NO_AT_BRIDGE, and then load by
> hand both gail and the bridge. But this could be done in a posterior task, but
> take that into account if you want to do that while solving the other issues.
>
ok, file a bug for that and assign it to me and I'll work on that on another branch