Merge lp:~alanbell/compiz/texttracking into lp:compiz/0.9.8
| Status: | Work in progress | ||||
|---|---|---|---|---|---|
| Proposed branch: | lp:~alanbell/compiz/texttracking | ||||
| Merge into: | lp:compiz/0.9.8 | ||||
| Diff against target: |
1249 lines (+1101/-2) 13 files modified
plugins/accessibility/AUTHORS (+1/-0) plugins/accessibility/CMakeLists.txt (+5/-0) plugins/accessibility/VERSION (+1/-0) plugins/accessibility/accessibility.xml.in (+17/-0) plugins/accessibility/compiz-accessibility.pc.in (+12/-0) plugins/accessibility/include/accessibility/accessibility.h (+217/-0) plugins/accessibility/src/accessibility.cpp (+651/-0) plugins/accessibility/src/private.h (+113/-0) plugins/ezoom/CMakeLists.txt (+1/-1) plugins/ezoom/ezoom.xml.in (+2/-0) plugins/ezoom/src/ezoom.cpp (+71/-0) plugins/ezoom/src/ezoom.h (+9/-0) plugins/mousepoll/mousepoll.xml.in (+1/-1) |
||||
| To merge this branch: | bzr merge lp:~alanbell/compiz/texttracking | ||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Daniel van Vugt | Needs Fixing on 2012-07-03 | ||
| Sam Spilsbury | 2012-06-23 | Needs Fixing on 2012-06-23 | |
|
Review via email:
|
|||
Description of the Change
merges in code from
https:/
https:/
fixing lp:#727290
it compiles, might have an additional dependency on at-spi stuff, I don't know how to merge it with the debian packaging to make an installable package.
| Sam Spilsbury (smspillaz) wrote : | # |
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 (AccessibilityE
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:
1215 + {
1216 + ensureVisibilit
1217 + rect.y1(),
1218 + rect.x2(),
1219 + rect.y2(),
1220 + optionGetRestra
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_
655 + (text, offset, ATSPI_COORD_
through an interface by which AtspiRect can be manipulated and atspi_text_
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)
| Daniel van Vugt (vanvugt) wrote : | # |
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.
| Daniel van Vugt (vanvugt) wrote : | # |
When done, please click Resubmit so we get notified.
| Daniel van Vugt (vanvugt) wrote : | # |
Please also click "Set commit message" when done so the automerger doesn't reject it once approved by us.
| Sam Spilsbury (smspillaz) wrote : | # |
(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)
| Alan Bell (alanbell) wrote : | # |
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:/
| Sam Spilsbury (smspillaz) wrote : | # |
Sure, no problem :)
| Alejandro Leiva (gloob) wrote : | # |
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.
| Alan Bell (alanbell) wrote : | # |
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
Unmerged revisions
- 3256. By Alan Bell on 2012-06-23
-
indentation issue
- 3255. By Alan Bell on 2012-06-23
-
remove message
- 3254. By Alan Bell on 2012-06-23
-
remove README and fix indentation
- 3253. By Alan Bell on 2012-06-22
-
renamed the accessibility plugin folder and fixed an unused variable in ezoom
- 3252. By Alan Bell on 2012-06-22
-
merging in text tracking zoom work by Alejandro Leiva
https://github. com/gloob/ compiz- accessibility- plugin
https://github. com/gloob/ gloob-Ezoom- fork


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