Code review comment for lp://qastaging/~rodrigo-moya/unity/sync-geometries

Revision history for this message
Alejandro PiƱeiro (apinheiro) wrote :

I have tested it with accerciser, and about this comment:
"Note that there's something wrong with the geometries, and not all indicator objects/entries get highlighted in the correct position when selected in accerciser"

I only got one indicator object/entry having the proper position.

About the code, in general seems good, although it would be required a review of the non accessibility related code (the methods and signals used by the AtkComponent implementation) except in one detail:

149 +panel_indicator_accessible_get_extents (AtkComponent *component,
150 + gint *x,
151 + gint *y,
152 + gint *width,
153 + gint *height,
154 + AtkCoordType coord_type)

309 +panel_indicator_entry_accessible_get_extents (AtkComponent *component,
310 + gint *x,
311 + gint *y,
312 + gint *width,
313 + gint *height,
314 + AtkCoordType coord_type)

You don't check coord_type at all. Take into account that the data that you return are totally different depending on coord_type. As far as I see, in both cases you try to return the result assuming global screen positions (ATK_XY_SCREEN), so it is missing the ATK_XY_WINDOW

In the same way, taking into account that you are already using the signal geometries_changed, I think that it would be easy to also emit the signal "bounds-changed":

http://library.gnome.org/devel/atk/stable/AtkComponent.html#AtkComponent-bounds-changed

BTW, about the signals. Each time that you receive this signal the size is updated, but, what happen with the initial state? Initially this is set to zero. That means that if the indicators/entry has already his position/size set before the accessible object is created, it will have a wrong position/size value? (as it is not updated by the signal)?

review: Needs Fixing

« Back to merge proposal