Re: AT-SPI caching and D-Bus usage

Alejandro et al,

Thanks for the reply. I apologize for taking so long to get to 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).

I might have removed it by accident. Thanks for the catch.

> 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).

Right, although whether to expose things as glib properties is a good 
question and might be worth looking at in terms of merging AT-SPI and ATK.

> 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.

At the moment I have a new D-Bus property defined, called ScreenExtents, 
which corresponds to the extents with ATSPI_COORD_TYPE_SCREEN specified.

>
> 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).

Right now we only have D-Bus properties in AT-SPI. A D-Bus property is 
sent using a variant which can hold an arbitrary type, including a 
structure. If you're talking about GObject properties, then it is similar, 
since they are sent using GValues which can potentially hold a boxed type.

> 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.

Good point; it is pretty arbitrary right now.

>> /**
>>  * 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.

Fwiw, it used to be that only documents implemented collections in AT-SPI, 
although there was no real reason for this restriction, and iirc Joanie 
felt that it would be useful to Orca for all objects to implement the 
interface.

>>  * @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.

Yeah, I was envisioning that properties could be added to fetch things in 
some cases that now require multiple calls. For instance, the magnifier 
might want to fetch the extents of the character under the cursor. To do 
this, it first needs to query the offset of the caret, then query the 
extents of the character at that offset, whereas, if there were a 
CurrentCharacterScreenExtents property, it could simply request the 
information it needed in one asynchronous call.

>>  * @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.

The use case I had in mind was that Orca might want to find an ancestor of the focused object with a given role, then query all descendants of that object that are on screen, and finding such an ancestor could require several calls. But then again my guess is that Orca would only do that once when constructing a flat review context, and several round-trip calls aren't really that much of a hit performance-wise (it's more the cases where we make several hundred calls that we need to worry about here). So I'm friendly to that amendment.

>>  * @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?

I'm hoping that the function will be useful to Orca for building flat 
review contexts, and Orca might want to query something that is not 
available as a property. If gnome-shell were to do this, then it should 
not be allowed and indicates that we need to come up with a way to allow 
gnome-shell to get the data that it needs asynchronously, but we don't 
currently need this hard restriction for Orca, since it doesn't need to 
respond to synchronous non-AT-SPI D-Bus calls from other applications

>>  * @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>)
>
Maybe. The new function is doing things that the previous functions 
haven't been doing (ie, querying properties and caching their values for 
use in the callback).

>
> 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.

-Mike