Re: [g-a-devel] AT-SPI caching and D-Bus usage

[Piñeiro <apinheiro igalia com>]

On 12/04/2013 04:54 AM, Mike Gorse wrote:
> Hi all,

Hi Mike,

sorry for the delay of the answer, I have been doing some research to
properly answer this email.

> Here is what I have for API additions right now (still haven't
> committed or fully tested the code). Of course this isn't necessarily
> final; just posting it here should people have feedback on it.
>
> -- 
>
> /**
>  * atspi_event_listener_register_full:
>  * @listener: The #AtspiEventListener to register against an event type.
>  * @event_type: a character string indicating the type of events for
> which
>  *            notification is requested.  See
> #atspi_event_listener_register
>  * for a description of the format and legal event types.
> * @properties: (element-type gchar*) (transfer none) (allow-none): a
> list of
>  *             properties that should be sent along with the event. The
>  *             properties are valued for the duration of the event
> callback.k
>  *             TODO: Document.

Nitpick: atspi_event_listener_register had a "error" param of type
GError*. Do you plan to remove the "error" from this method? Because if
that is the case, I'm not sure if adding _full is a good name. Usually
the _full is added when you define a method that has exactly the same
methods that the base case, plus some others (like properties).

Now real questions:

As this is an at-spi2-core addition, I assume that for properties you
mean those defined as properties on the DBUS xml files. Saying that as
the atspi library doesn't define any glib properties (in that aspect,
this is another point to improve documentation).

So under that assumption, it is true that this new method would solve
several roundabouts, so reducing the amount of DBUS messages on several
cases. But I don't see how this would solve the original problem that
triggered this API addition. Im talking about that magnifier freeze [1]
on some situations. One of the proposals was sending an AtspiRect with
the Extents of the object. Discarded waiting for a more high level
proposal. First step is this mail. The problem is that the Extents of an
object is not a property of the Component interface. It is defined the
method GetExtents, but not the property.

So, how this new API will handle this case? Or to solve this situation
you are proposing this API and defining new properties? I guess that
this would be complex for properties with more that one component, like
Extents. I'm not sure right now if you can define a property with two
components (typical case: Position with x and y).

And in relation with the properties, somewhat off-topic on this thread,
but still relevant. While working on the AtkDocument stuff, specifically
about bringing the new document stuff to at-spi2, I noticed that the
usage of properties and/or Get/Set methods is not consistent. For
example, Accessible define Name and Description as read-only properties,
so doesn't define a GetName/GetDescription methods. But Component define
methods GetAlpha/GetLayer, but not properties. In the same way, there
are some properties defined on at-spi but not as properties on atk (like
current-page-number, yes my fault). It would be good to try to define a
"when something should be defined as a property and when not" and try to
homogenize at-spi2 with that rules. And also apply that to atk,
something that would also simplify any future atk/at-spi2 merge.

> /**
>  * atspi_collection_get_tree:
>  * @collection: the #AtspiCollection to query.

FWIW: This assumes that any accessible would implement collection. Is
the current case but not sure if that will be still true when Atk adds
Collection.

>  * @properties: (element-type gchar*) (transfer none) (allow-none): The
>  * properties to fetch, or %NULL to fetch all supported properties.

properties param here is also affected by what I mentioned on previous
method. Not all the relevant information on at-spi2 is right now defined
as properties.

>  * @rule_start: (allow-none): a match rule specifying the ancestor
> from which
>  * to start, or %NULL to use the object specified in @collection itself.

I don't really understand that @rule_start param. Using as reference
other atspicollection is used, "rules" are used to define which elements
we want under an ancestor, not which ancestor we want. So if we use rule
to define the ancestor, we lose the way to define which siblings do we
want. It also doesn't define the depth. IMHO, I would keep the
collection used to call this method as the ancestor, and would use rule
to define which children from that collection we want.

>  * @allow_sync: Specifies whether synchronous calls will be allowed
> during
>  * the execution of @callback. If this is set to %FALSE, then an
> exception
>  * will be thrown if the callback calls an AT-SPI function which would
>  * otherwise make a synchronous D-Bus call. Set to %FALSE in cases
> where a
>  * synchrous D-Bus call has the potential to cause deadlock (for
> instance, if
>  * the application needs to be able to respond to non-AT-SPI D-Bus
> calls from
>  * other applications). Otherwise set to %TRUE.

We already had several sync methods, and the atspi collection. Is there
any reason to allow this method to be synchronous?

>  * @callback: (scope async): The callback to be invoked when data is
> available.
>  * @user_data: Data to pass to @callback.
>  *
>  * Asynchronously fetches data for an object and all of its descendants.
>  * The data is cached for the duration of @callback.
>  */

I liked the idea of using collection for this. Although collection needs
to be refined, and defined on ATK, I think that is a good idea using it,
as would mean putting all the "get all objects that fulfill X" on the
same interface. But as I mentioned, there are some problems on this
method definition. So in that sense I think that it would be simpler to
just reuse what collection defines, but adding an asynchronous calls.
So, something like:

atspi_collection_get_matches_async (<same signature that
atspi_collection_get_matches>)

But probably, there are some other reasons preventing this.

In any case, I have the feeling, that eventually all _get related
methods on atspi would need to be defined as asynchronous. Even stuff
like atspi_accessible_get_name. But it is true that this would be
somewhat tricky on the server side. But this is a long-long term
discussion, and this two methods are a good first step.

BR

[1] https://bugzilla.gnome.org/show_bug.cgi?id=708387

-- 
----
Alejandro Piñeiro