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

Revision history for this message
Sam Spilsbury (smspillaz) wrote :

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 runtime, but I feel like there must be a better way of doing this.

Because of this system, we have

1201 + if (object->is (Component))
1202 + {
1203 +
1204 + AccessibilityComponent::Ptr ac =
1205 + boost::static_pointer_cast<AccessibilityComponent>
1206 + (object->getEntity (Component));
1207 +
1208 + CompRect rect = ac->getExtents ();
1209 +
1210 + compLogMessage ("Ezoom", CompLogLevelInfo,
1211 + "ensureVisibilityArea [%d, %d] [%d, %d]\n",
1212 + rect.x1(), rect.y1(), rect.x2(), rect.y2());
1213 +
1214 + if (optionGetZoomMode () == EzoomOptions::ZoomModePanArea)
1215 + {
1216 + ensureVisibilityArea (rect.x1(),
1217 + rect.y1(),
1218 + rect.x2(),
1219 + rect.y2(),
1220 + optionGetRestrainMargin (),
1221 + NORTHWEST);
1222 + }
1223 + }
1224 +
1225 + if (object->is (Text))
1226 + {
1227 + AccessibilityText::Ptr at =
1228 + boost::static_pointer_cast<AccessibilityText>
1229 + (object->getEntity (Text));
1230 +
1231 + CompRect rect = at->getCharacterExtents (at->getCaretOffset ());
1232 +
1233 + compLogMessage ("Ezoom", CompLogLevelInfo,
1234 + "TEXT - [%d, %d] [%d, %d]\n",
1235 + rect.x1(), rect.y1(), rect.x2(), rect.y2());
1236 +
1237 + if (optionGetZoomMode () == EzoomOptions::ZoomModePanArea)
1238 + {
1239 + ensureVisibilityArea (rect.x1(),
1240 + rect.y1(),
1241 + rect.x2(),
1242 + rect.y2(),
1243 + optionGetRestrainMargin (),
1244 + NORTHWEST);
1245 + }
1246 + }

Which is effectively an implementation detail leak into ezoom.

Since at the moment, we're only using accessibility to provide a means for ezoom to get an area to pan to, how about doing either one of these two things?

1. Have a wrapper object that satisfies an interface like getZoomToArea () or something which knows how to operate either case
2. Have both implementations of AccessibilityEntity (eg, AccessibilityComponent and AccessibilityText) satisfy that interface directly.

Now instead of getEntity (), you can have a call like:

class AccessibleZoomToInterface
{
    ~AccessibleZoomToInterface () {}

    virtual CompRect getZoomToArea () = 0;
};

/* AccessibilityComponent or AccessibilityText satisfy such an interface */

class AccessibilityComponent
{
...

    virtual AccessibilityZoomToInterface * getZoomToInterface ();
};

AccessibilityEntity::getZoomToInterface ()
{
    return NULL;
}

AccessibilityText::getZoomToInterface ()
{
    return this;
}

AccessibilityComponenet::getZoomToInterface ()
{
    return this;
}

Then client code will know if the object in the event supports the ability to be zoomed to simply by calling one method, rather than by inspecting the type.

Random errata:
 * The clone functions are unused, and will probably not be used and can be nuked.
 * Same goes for EzoomScreen::enableAccessibility
 * a11yHandle is not deleted anywhere, use a unique_pointer for that

You have

181 +enum IfaceType
182 +{
183 + Accessible = 0,
184 + Action,
185 + Collection,
186 + Component,
187 + Document,
188 + EditableText,
189 + Hypertext,
190 + Hyperlink,
191 + Image,
192 + Selection,
193 + Table,
194 + Text,
195 + Value,
196 +};

And

1061 +static const char *IfaceTypeStr[] =
1062 +{
1063 + "Accessible",
1064 + "Action",
1065 + "Collection",
1066 + "Component",
1067 + "Document",
1068 + "EditableText",
1069 + "Hypertext",
1070 + "Hyperlink",
1071 + "Image",
1072 + "Selection",
1073 + "Table",
1074 + "Text",
1075 + "Value",
1076 +};
1077 +
1078 +#define NUM_IFACES_SUPPORTED 13
1079 +

Ouch, a #define and data duplication. It see that its used for getting effectively a map of interface strings to enums. What you can do is just put it in a map like this:

class AccessibleObjectInterfaceTypes
{
    public:

        AccessibleObjectInterfaceTypes ()
        {
            mMap["Accessible"] = Accessible;
            ... etc
        }

        IfaceType get (const std::string &t) { return mMap[t]; }

    private:

        std::map <std::string, IfaceType> mMap;
};

namespace
{
    static AccessibleObjectInterfaceTypes accessibleInterfaceTypes;
}

Then instead of doing:

interfaces.push_back (enumFromStr (iface));

You can do

interfaces.push_back (::accessibleInterfaceTypes.get (iface));

I think it should be safe to remove
314 + Entities ents;
294 + AccessibilityEntity::Ptr
295 + get (IfaceType);

Since they look effectively unused.

Also there's a bunch of redundant placeholder code.

935 +void
936 +AccessibilityScreen::handleAccessibilityEvent (AccessibilityEvent *event)
937 +{
938 +
939 + AccessibleObject *object = event->getAccessibleObject ();
940 +
941 + if (object->is (Component))
942 + {
943 +
944 + AccessibilityComponent::Ptr ac =
945 + boost::static_pointer_cast<AccessibilityComponent>
946 + (object->getEntity (Component));
947 +
948 + CompRect rect = ac->getExtents ();
949 +
950 + compLogMessage ("Accessibility", CompLogLevelInfo, "Object is Component\n");
951 +
952 + compLogMessage ("Accessibility", CompLogLevelInfo,
953 + "Component Area [%d, %d] [%d, %d]\n",
954 + rect.x1(), rect.y1(), rect.x2(), rect.y2());
955 + }
956 + else
957 + {
958 +
959 + compLogMessage ("Accessibility", CompLogLevelInfo, "Object is NOT Component\n");
960 + }
961 +}

I think that's safe to nuke.

708 +Accessibility::Accessibility ()
709 +{
710 + compLogMessage ("Accessibility", CompLogLevelInfo,
711 + "Accessibility constructor called.\n");
712 +
713 +}
714 +
715 +Accessibility::~Accessibility ()
716 +{
717 + compLogMessage ("Accessibility", CompLogLevelInfo,
718 + "Accessibility destructor called.\n");
719 +
720 +}
721 +
722 +bool
723 +Accessibility::start ()
724 +{
725 + return true;
726 +}
727 +
728 +bool
729 +Accessibility::stop ()
730 +{
731 + return true;
732 +}
733 +
734 +bool
735 +Accessibility::active ()
736 +{
737 + return true;
738 +}

These all do nothing.

979 + /*
980 + registerEventHandler ("object:state-changed:", boost::bind (
981 + &AccessibilityScreen::handleAccessibilityEvent, this, _1));
982 + */
983 +
984 + compLogMessage ("Accessibility", CompLogLevelInfo, "Running!\n");
985 +
986 + /*
987 + // Launch main event atspi loop
988 + // This is not needed because compiz private screen launches its own
989 + // Glib MainLoop
990 + */
991 + /*
992 +
993 + atspi_event_main();
994 + */

this can all be deleted

All in all right direction, just needs some tidying to make it simpler.

review: Needs Fixing

« Back to merge proposal