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:
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.
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.
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 */
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
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:
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, ntity:: AccessibilityEn tity (%s)\n", object->name);
519 + "AccessibilityE
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 ()) >unregisterAll ();
1270 + a11yHandle-
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 mponent
- AccessibilityCo
- 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: tity::Ptr (new AccessibilityCo mponent (object)); tity::Ptr (new AccessibilityText (object)); tity::Ptr (new AccessibilityEntity (object));
435 + entity = AccessibilityEn
436 + break;
437 +
438 + case Text:
439 + entity = AccessibilityEn
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 = AccessibilityEn
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)) mponent: :Ptr ac = static_ pointer_ cast<Accessibil ityComponent>
942 + {
943 +
944 + AccessibilityCo
945 + boost::
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)) mponent: :Ptr ac = static_ pointer_ cast<Accessibil ityComponent> tyArea [%d, %d] [%d, %d]\n", :ZoomModePanAre a) yArea (rect.x1(), inMargin (), xt::Ptr at = static_ pointer_ cast<Accessibil ityText> rExtents (at->getCaretOffset ()); :ZoomModePanAre a) yArea (rect.x1(), inMargin (),
1202 + {
1203 +
1204 + AccessibilityCo
1205 + boost::
1206 + (object->getEntity (Component));
1207 +
1208 + CompRect rect = ac->getExtents ();
1209 +
1210 + compLogMessage ("Ezoom", CompLogLevelInfo,
1211 + "ensureVisibili
1212 + rect.x1(), rect.y1(), rect.x2(), rect.y2());
1213 +
1214 + if (optionGetZoomMode () == EzoomOptions:
1215 + {
1216 + ensureVisibilit
1217 + rect.y1(),
1218 + rect.x2(),
1219 + rect.y2(),
1220 + optionGetRestra
1221 + NORTHWEST);
1222 + }
1223 + }
1224 +
1225 + if (object->is (Text))
1226 + {
1227 + AccessibilityTe
1228 + boost::
1229 + (object->getEntity (Text));
1230 +
1231 + CompRect rect = at->getCharacte
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:
1238 + {
1239 + ensureVisibilit
1240 + rect.y1(),
1241 + rect.x2(),
1242 + rect.y2(),
1243 + optionGetRestra
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 mponent and AccessibilityText) satisfy that interface directly.
2. Have both implementations of AccessibilityEntity (eg, AccessibilityCo
Now instead of getEntity (), you can have a call like:
class AccessibleZoomT oInterface ZoomToInterface () {}
{
~Accessible
virtual CompRect getZoomToArea () = 0;
};
/* AccessibilityCo mponent or AccessibilityText satisfy such an interface */
class AccessibilityCo mponent
{
...
virtual AccessibilityZo omToInterface * getZoomToInterface ();
};
AccessibilityEn tity::getZoomTo Interface ()
{
return NULL;
}
AccessibilityTe xt::getZoomToIn terface ()
{
return this;
}
AccessibilityCo mponenet: :getZoomToInter face ()
{
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: :enableAccessib ility
* The clone functions are unused, and will probably not be used and can be nuked.
* Same goes for EzoomScreen:
* 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[] = SUPPORTED 13
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_
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 AccessibleObjec tInterfaceTypes
{
public:
{
... etc
}
IfaceType get (const std::string &t) { return mMap[t]; }
private:
std::map <std::string, IfaceType> mMap;
};
namespace tInterfaceTypes accessibleInter faceTypes;
{
static AccessibleObjec
}
Then instead of doing:
interfaces. push_back (enumFromStr (iface));
You can do
interfaces. push_back (::accessibleIn terfaceTypes. get (iface));
I think it should be safe to remove tity::Ptr
314 + Entities ents;
294 + AccessibilityEn
295 + get (IfaceType);
Since they look effectively unused.
Also there's a bunch of redundant placeholder code.
935 +void creen:: handleAccessibi lityEvent (AccessibilityEvent *event) getAccessibleOb ject (); mponent: :Ptr ac = static_ pointer_ cast<Accessibil ityComponent>
936 +AccessibilityS
937 +{
938 +
939 + AccessibleObject *object = event->
940 +
941 + if (object->is (Component))
942 + {
943 +
944 + AccessibilityCo
945 + boost::
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 () :~Accessibility () :start () :stop () :active ()
709 +{
710 + compLogMessage ("Accessibility", CompLogLevelInfo,
711 + "Accessibility constructor called.\n");
712 +
713 +}
714 +
715 +Accessibility:
716 +{
717 + compLogMessage ("Accessibility", CompLogLevelInfo,
718 + "Accessibility destructor called.\n");
719 +
720 +}
721 +
722 +bool
723 +Accessibility:
724 +{
725 + return true;
726 +}
727 +
728 +bool
729 +Accessibility:
730 +{
731 + return true;
732 +}
733 +
734 +bool
735 +Accessibility:
736 +{
737 + return true;
738 +}
These all do nothing.
979 + /* ndler ("object: state-changed: ", boost::bind ( creen:: handleAccessibi lityEvent, this, _1));
980 + registerEventHa
981 + &AccessibilityS
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.