Merge lp://qastaging/~ksamak/compiz/ezoom_focus_tracking into lp://qastaging/compiz/0.9.13

Proposed by ksamak
Status: Needs review
Proposed branch: lp://qastaging/~ksamak/compiz/ezoom_focus_tracking
Merge into: lp://qastaging/compiz/0.9.13
Diff against target: 1617 lines (+1252/-39)
17 files modified
debian/compiz-dev.install (+1/-0)
debian/compiz-plugins-default.install (+2/-0)
debian/control (+1/-0)
plugins/ezoom/CMakeLists.txt (+1/-1)
plugins/ezoom/ezoom.xml.in (+35/-8)
plugins/ezoom/src/ezoom.cpp (+123/-28)
plugins/ezoom/src/ezoom.h (+22/-2)
plugins/focuspoll/CMakeLists.txt (+9/-0)
plugins/focuspoll/compiz-focuspoll.pc.in (+12/-0)
plugins/focuspoll/focuspoll.xml.in (+34/-0)
plugins/focuspoll/include/accessibilitywatcher/accessibilitywatcher.h (+82/-0)
plugins/focuspoll/include/accessibilitywatcher/focusinfo.h (+76/-0)
plugins/focuspoll/include/focuspoll/focuspoll.h (+50/-0)
plugins/focuspoll/src/accessibilitywatcher.cpp (+506/-0)
plugins/focuspoll/src/focuspoll.cpp (+220/-0)
plugins/focuspoll/src/private.h (+75/-0)
plugins/showmouse/showmouse.xml.in (+3/-0)
To merge this branch: bzr merge lp://qastaging/~ksamak/compiz/ezoom_focus_tracking
Reviewer Review Type Date Requested Status
ksamak (community) Needs Resubmitting
Andrea Azzarone Pending
Marco Trevisan (TreviƱo) Pending
Sam Spilsbury code, style Pending
Review via email: mp+321643@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2017-02-24.

Description of the change

ezoom: added focus tracking based on at-spi a11y events.

To post a comment you must log in.
Revision history for this message
Andrea Azzarone (azzar1) wrote : Posted in a previous version of this proposal

Can you please link a bug to the MP?

Revision history for this message
Andrea Azzarone (azzar1) wrote : Posted in a previous version of this proposal

Ok this is the first part of the review. I'll continue later.

review: Needs Fixing
Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

Thanks for all the hard work in implementing this. There's a fair bit of work to be done before this can be integrated. In particular, this cannot be merged whilst it opens a separate display connection - that is a recipe for deadlocks.

review: Needs Fixing (code, style)
Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal
Revision history for this message
ksamak (ksamak) wrote : Posted in a previous version of this proposal

well, this plugin would indeed have been helpful.

Sam Spilsbury (smspillaz) wrote 18 hours ago:
I like the fact that this is a separate library, but this absolutely doesn't belong in the mousepoll plugin. The mouse poll plugin is supposed to do one thing and one thing only - update the position of the mouse every N milliseconds. Move all this into either a separate plugin or library.

I originally made a copy of the mousePoll plugin (called focusPoll), but for some unknown reason it... deadlocked, or froze at runtime. I couldn't find why, and no one had time to look at it, so I merged both logics in mousepoll, cause then, with sensibly the same methods, it didn't freeze.

I agree an additional plugin would serve better, so I'd appreciate some input on that problem.
there's a readyish branch on http://git.hypra.fr/hypra/compiz.git named focuspoll. Maybe you would have some insights as to what is blocking here. It might be, as you pointed, the call to open the display.
question: how would you circumvent the problem of the call to XOpenDisplay, if the whole a11ywatcher was a library? Is there a better way to get the screen info? that is without causing all those problems.

I will meditate on these ideas, and weight the pros and cons of the different solutions, and i'll then continue the work.
thank you for all the advice and corrections, including those pending below. I am available to discuss these matters further, or if you have more insight.

review: Needs Fixing
Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

> Maybe you would have some insights as to what is blocking here. It
> might be, as you pointed, the call to open the display.
> question: how would you circumvent the problem of the call to XOpenDisplay, if
> the whole a11ywatcher was a library? Is there a better way to get the screen
> info? that is without causing all those problems.

Yes, there's a global variable called 'screen' and you can access the current connection from there (screen->dpy()). If your library needs a reference to it, I'd suggest having the caller pass it in.

Revision history for this message
ksamak (ksamak) wrote : Posted in a previous version of this proposal

I've answered some of the following, and will superseed the merge request with the new version from the branch.
i thank you for your time, advice, and corrections.

Revision history for this message
ksamak (ksamak) : Posted in a previous version of this proposal
Revision history for this message
ksamak (ksamak) wrote : Posted in a previous version of this proposal

call for help on the matter of binding C callback to c++ member function.
thx

Revision history for this message
ksamak (ksamak) wrote : Posted in a previous version of this proposal

just a ping, i changed coding style again, on request from 3v1n0.

Revision history for this message
Andrea Azzarone (azzar1) wrote : Posted in a previous version of this proposal

I reviews part of the code. Fix it before I going on.

review: Needs Fixing
Revision history for this message
ksamak (ksamak) wrote : Posted in a previous version of this proposal

I fixed the code according to your specifications, i changed the whole coding style of my editor, thanks to the few lines i (finally) found here:
http://wiki.compiz.org/Development/CodingStyle
Sorry again for my misunderstanding tabs and coding style, i'll try my best to respect that from initial coding on, event though i find it less readable.
I thank you for your patience.

review: Needs Resubmitting
4122. By ksamak <ksamak@ksalaptop>

added precedence relation to start showmouse before ezoom

4123. By ksamak <ksamak@ksalaptop>

coding style

4124. By ksamak <ksamak@ksalaptop>

added rebranding of icedove to thunderbird support

Revision history for this message
ksamak (ksamak) wrote :

created lp:~ksamak/compiz/correct_mousetheme_targetting_for_ezoom, although i think it wasn't necessary. I'll delete it.
I add the update for the rebranding of icedove into thunderbird.
thank you.

review: Needs Resubmitting
Revision history for this message
Samuel thibault (samuel-thibault) wrote :

Hello,

I have gone through the previous review from azzar1, and commented further.

Samuel

Revision history for this message
Samuel thibault (samuel-thibault) wrote : Posted in a previous version of this proposal

I have commented on the callback issue, to avoid the singleton.

Unmerged revisions

4124. By ksamak <ksamak@ksalaptop>

added rebranding of icedove to thunderbird support

4123. By ksamak <ksamak@ksalaptop>

coding style

4122. By ksamak <ksamak@ksalaptop>

added precedence relation to start showmouse before ezoom

4121. By ksamak <ksamak@ksalaptop>

modifications of code according to azzar's request

4120. By ksamak <ksamak@ksalaptop>

coding style modifs. changed if format on 3v1n0's request

4119. By ksamak <ksamak@ksalaptop>

coding style modifs

4118. By ksamak <ksamak@ksalaptop>

coding style modifs

4117. By ksamak <ksamak@ksalaptop>

minors code modifs

4116. By ksamak <ksamak@ksalaptop>

minors code modifs

4115. By ksamak <ksamak@ksalaptop>

corrected segfault, minors code modifs

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches