Code review comment for lp:~alanbell/compiz/texttracking

Oh yeah, totally forgot! Tests...

Generally speaking, I prefer all code going into compiz to come with unit tests. I don't think getting this particular code under test will be that difficult. The majority of the a11y code looks like its just wrappers for managing objects coming to us from AT-SPI.

Stuff that I think needs to be tested (or needs to be deleted, since it looks unused):

206 + virtual bool
207 + load (AtspiAccessible *);
208 +
209 + virtual bool
210 + contains (AccessibilityEntity *, int, int);
211 +

Actually they can be deleted completely as they are unused. But...

236 + CompRect
237 + getExtents () const;
238 +
239 + CompPoint
240 + getPosition () const;
241 +
242 + CompPoint
243 + getSize () const;

I am like 99% sure the second two are unused.

The relationship between getExtents and this:

1214 + if (optionGetZoomMode () == EzoomOptions::ZoomModePanArea)
1215 + {
1216 + ensureVisibilityArea (rect.x1(),
1217 + rect.y1(),
1218 + rect.x2(),
1219 + rect.y2(),
1220 + optionGetRestrainMargin (),
1221 + NORTHWEST);
1222 + }

Or, a potential getZoomCenterArea () needs to come under test

As well as:

267 + CompRect
268 + getCharacterExtents (int) const;
269 +
270 + CompRect
271 + getRangeExtents (int) const;
272 +
273 + int
274 + getCaretOffset ();

(getRangeExtents can be deleted)

But the relationship between the first and the third needs to be tested. That means breaking the dependency on

654 + AtspiRect *character_rect = atspi_text_get_character_extents
655 + (text, offset, ATSPI_COORD_TYPE_SCREEN, &error);

through an interface by which AtspiRect can be manipulated and atspi_text_get_character_extents be queried.

Then you can use Mock Objects to verify the behaviour of getCharacterExtents and the resulting zoom rect.

Feel free to poke with questions on IRC. I'm on planes for the next two days, but I can be found on freenode (nick: smspillaz)