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