compiz - Apr 2015 - Accessibility: mouse guides for Compiz, and forthcoming contributions

Hi Luca,

Nice to see some new Compiz code here. :)
The rulers are a good idea and a useful, nice addition to Compiz' 
mouse-related capabilities.

You should be rewarded with a review.

Regarding your question about putting this functionality into a new plugin:

After all it fits into Showmouse as it does exactly that, but then the 
visual style does not have much in common with the particle-fx this 
plugin usually uses and I doubt many people will use both effects 
(particles  and rulers)  at the same time...
I suggest adding shortcuts to be able to toggle the rulers independently 
from the particles and to optimize the "wrapped" paint functions 
(preparePaint (), glPaintOutput () and donePaint ()) to not do any 
redundant calculations.

Regarding your code and possible optimizations:

You are doing redundant type conversions from float to double, then back 
to GLfloat again, drawLine () should use floats/GLfloats as arguments 
instead.

In drawGuides () you are also wasting some CPU as you are ignoring the 
types of variables you use - CompRegion () expects ints, while drawLine 
() and glLineWidth () (should) expect floats - your optionGet* () return 
types are int and mousePos.x ()/.y () also. You should try to have this 
method do the least amount of type conversions.
Also you could save the int return values of screen->width () and 
screen->height () to not have to call them twice in this one method.
Setting thickness to 20 for damaging is not necessary at all - simply 
use the actual int thickness optionGetGuideThickness () gives you - do 
not always damage the maximum paint region, simply damage the actual 
paint region instead, which will also improve performance. Hint: You can 
use the showrepaint plugin for damage region visualization and debugging.
You could also check if glIsEnabled (GL_BLEND) and only toggle blending 
if it is not activated, as this global state might have been activated 
already by some other plugin.

In preparePaint () you are still calling ps.updateParticles () and 
updating the rotation member variable, in donePaint () you are still 
damaging/redrawing the particle area, even if there are no particle 
emitters.

Ideas for improvements:

The rulers should be separated more from the particles, code-wise and 
control-wise - triggering rulers should not depend on triggering the 
particle emitters.
The rulers could gently fade in/out, not pop in/out like they currently 
do - when disabling showmouse with particle emitters enabled the rulers 
currently wait for the particles to fully disappear...
There should be a possibility to configure a maximum length for the 
rulers, maybe an option to show them only on the output device the 
cursor is on as well - some users might not like the from one screen end 
to the other screen's end approach (multimonitor users for example).
The rulers could optionally be stippled, animated or use some 
color-cycling effects, which should of course be configurable.

As your other questions regarding sourcecode and repos have already been 
answered by Stephen and others, I will not reiterate this.

I'll take a look at your mouse cursor theme setting plugin soon, as 
finally getting this missing core mouse functionality for Compiz is 
awesome, but you might want to post a merge request against lp:compiz 
[1] so more people will notice your work.

Greetinx,

MC Return

[1] https://code.launchpad.net/~compiz-team/compiz/0.9.12/+activereviews
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/compiz/attachments/20150407/616876ac/attachment.html>

Hello MC Return, and thanks for your detailed review.

On 2015-04-07 at 12:42, MC Return wrote:
> Nice to see some new Compiz code here. :)
More will come.
> Regarding your code and possible optimizations:
I'm now updating the code following your advice.  I was in fact a little
worried about high CPU usage; I hope your suggested changes will improve
that substantially.

I'm keeping the new functionality in the showmouse plugin for the time
being.  It shouldn't be too hard to separate later, if you prefer.
> you
> might want to post a merge request against lp:compiz [1] so more people
will
> notice your work.
I certainly will.

Thanks,

-- 
Luca Saiu
Hypra team (Accessible website in construction)