Merge lp://qastaging/~alanbell/compiz/texttracking into lp://qastaging/compiz/0.9.8

Looks good, and definitely on the right track, needs some work before we can merge it in, but definitely something we can have for 12.10

The README can probably be cut down or go entirely, it seems to be mostly about how to enable AT-SPI and how to build and install the plugin, which is mostly redundant anyways as the buildsystem will build it for us.

There are a few indentation problems that need to be fixed. Indentation in compiz is a little weird (X11 style). It goes:

4 spaces
8-wide tab
8-wide tab + 4 spaces
8-wide tab + 8 wide tab
8-wide tab + 8-wide tab + 4 spaces

etc

So some areas where this needs to be fixed are:

215 + virtual AccessibilityEntity *
216 + clone () const;
217 +
218 + AtspiAccessible *
219 + getObject ();

215 + virtual AccessibilityEntity *
216 + clone () const;
217 +
218 + AtspiAccessible *
219 + getObject ();

412 + for (int i = 0; i < len; i++) {
413 +

The brace goes on the next line, eg
for (int i = 0; i < len; i++)
{

 + switch (iface) {

ditto

518 + compLogMessage ("Accessibility", CompLogLevelInfo,
519 + "AccessibilityEntity::AccessibilityEntity (%s)\n", object->name);
520 +

I don't think these messages are necessary. Program function can be verified through unit tests (which I'll get on to in a minute)

1269 + if (a11yHandle->active ())
1270 + a11yHandle->unregisterAll ();

That needs to be a tab and not four spaces.

The second big thing I have about this code is that the design feels a little weird.

The inheritance hierarchy looks a bit like this:

AccessibilityEntity
 - AccessibilityComponent
 - AccessibilityText

AccessibleObject (which appears to be a wrapper around AtspiAccessible *)
Accessibility (which appears to be a manager class)

The real gripe I have here are things like this:

434 + case Component:
435 + entity = AccessibilityEntity::Ptr (new AccessibilityComponent (object));
436 + break;
437 +
438 + case Text:
439 + entity = AccessibilityEntity::Ptr (new AccessibilityText (object));
440 + break;
441 +
442 + case Accessible:
443 + case Action:
444 + case Collection:
445 + case Document:
446 + case EditableText:
447 + case Hypertext:
448 + case Hyperlink:
449 + case Image:
450 + case Selection:
451 + case Table:
452 + case Value:
453 + break;
454 +
455 + default:
456 + entity = AccessibilityEntity::Ptr (new AccessibilityEntity (object));

This code feels fragile, because our fallback case is "silently do nothing", and we just waste memory by tracking objects we don't really care about. Moreover, that behaviour is different from cases that we know we don't care about where we return a shared_ptr to NULL and then the client code just assumes that the pointer is valid.

There are other examples like this:

941 + if (object->is (Component))
942 + {
943 +
944 + AccessibilityComponent::Ptr ac =
945 + boost::static_pointer_cast<AccessibilityComponent>
946 + (object->getEntity (Component));

Generally speaking, code where you need to go from some "placeholder" down to something that you think the object is feels to me like reintepret_casting through void * with a note that everything will be okay.

I understand that Atspi is based on GObject and as such, you have to handle the type system at run...

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)

Please review and fix the indentation and style again. For example:

135 + virtual IfaceType
136 + is ();
137 +
138 + virtual AccessibilityEntity *
139 + clone () const;
140 +
141 + AtspiAccessible *
142 + getObject ();

should be more like:

   virtual IfaceType is ();
   virtual AccessibilityEntity *clone () const;
   AtspiAccessible *getObject ();

And the same applies elsewhere.

When done, please click Resubmit so we get notified.

Please also click "Set commit message" when done so the automerger doesn't reject it once approved by us.

(This will be looked at and I will propose a merge to fix some of the overarching design issues once the gles2 and gsettings work is done)

I will have a go at the indentation issues, but I am a bit out of my depth on the refactoring, this isn't my code, I am just trying to integrate it into the upstream tree, I will try and contact gloob https://github.com/gloob for some help I think.

Sure, no problem :)

Hi guys, thanks for working on this.

As Sam pointed out before there's some methods, classes and structures that I'm not using in the plugin. In fact I'm not happy with the global architecture. A refactor is needed. I'll do.

Alan if you can grant me write access to your branch I won't need to create a new branch and merge and so on.

Cheers.

thanks Alejandro,

I don't think I can give you write access to my branch, but you can probably do
bzr branch lp:~alanbell/compiz/texttracking
bzr push lp:~gloob/compiz/texttracking

and then do a merge request from there