Review Request: Mouse Click Animation Effect

[Martin Gräßlin]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105780/#review16612
-----------------------------------------------------------


Nice effect, looks really good and useful. Now we just need a screen record effect again ;-) I am not opposed to adding the effect to 4.10, thanks for sharing the code early for review.

A few general comments: I'm very picky about coding style. Looking at the patch I see many trailing whitespaces (note: if you use git, you can use a pre-commit hook to block commits with trailing whitespaces) and in a few places the coding style is not followed. In KWin we use the kdelibs coding style [1]. If I remember correctly there is an astyle setting available to convert.

I think you can remove the kdewin review group. AFAIK that's for KDE on Windows and well KWin is one of the few applications we will never see on Windows ;-)

[1] http://techbase.kde.org/Policies/Kdelibs_Coding_Style


kwin/effects/mouseclick/mouseclick.cpp
<http://git.reviewboard.kde.org/r/105780/#comment12935>

    you don't need the ifdef around this check, the enum is also available if not compiled with XRender support



kwin/effects/mouseclick/mouseclick.cpp
<http://git.reviewboard.kde.org/r/105780/#comment12936>

    general recommendation: don't explicit compare against null, just do
    if (m) {
    }
    
    it will evaluate to true if it is not null.



kwin/effects/mouseclick/mouseclick.cpp
<http://git.reviewboard.kde.org/r/105780/#comment12937>

    I would drop the two params "isGl" and "isXRender" and do the compositing backend check directly in this method. That way you don't have to care about the used backend in the calling method.



kwin/effects/mouseclick/mouseclick.cpp
<http://git.reviewboard.kde.org/r/105780/#comment12939>

    do you really need line smooth? Does it make any difference at all?
    
    Btw. GL_LINE_SMOOTH is not available in OpenGL ES 2.0, so if you need it, it needs to be ifdefed.



kwin/effects/mouseclick/mouseclick.cpp
<http://git.reviewboard.kde.org/r/105780/#comment12940>

    this method does not disable everything that got enabled



kwin/effects/mouseclick/mouseclick.cpp
<http://git.reviewboard.kde.org/r/105780/#comment12941>

    you probably won't need those two as in XRender there are no states to enable/disable as in OpenGL



kwin/effects/mouseclick/mouseclick.desktop
<http://git.reviewboard.kde.org/r/105780/#comment12945>

    No need to add your own translations. Our translation architecture would overwrite them anyway.


- Martin Gräßlin


On July 29, 2012, 3:01 p.m., Filip Wieladek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105780/
> -----------------------------------------------------------
> 
> (Updated July 29, 2012, 3:01 p.m.)
> 
> 
> Review request for kdewin and kwin.
> 
> 
> Description
> -------
> 
> I frequently have make screencasts (e.g. for video tutorials or filing bugs). One missing feature on KDE was capturing mouse presses when doing screencasts or presentations. It seems that at some point recorditnow did support mouse capture, but that functionality no longer works. 
> 
> I would propose to have a separate kwin effect which would do exactly that. I have started on an effect and finished the initial version. The results can be seen in the attached video.
> 
> Please let me know of any enhancements I can make. This is my first time writing a KDE Effect.
> 
> Note: The CMakeLists is still configured to create a separate library. This is for easier testing. if this effect would be integrated into Kwin builtin effects, I would be more than happy to change it so it would be included into the built in effects library. 
> 
> 
> Diffs
> -----
> 
>   kwin/effects/mouseclick/CMakeLists.txt PRE-CREATION 
>   kwin/effects/mouseclick/mouseclick.cpp PRE-CREATION 
>   kwin/effects/mouseclick/mouseclick.desktop PRE-CREATION 
>   kwin/effects/mouseclick/mouseclick.h PRE-CREATION 
>   kwin/effects/mouseclick/mouseclick_config.cpp PRE-CREATION 
>   kwin/effects/mouseclick/mouseclick_config.desktop PRE-CREATION 
>   kwin/effects/mouseclick/mouseclick_config.h PRE-CREATION 
>   kwin/effects/mouseclick/mouseclick_config.ui PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/105780/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Filip Wieladek
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-windows/attachments/20120729/4aa719f1/attachment-0001.html>