Re: Review Request: Focus Tracking in KWin's Zoom Plugin

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


First of all: we cannot depend on anything in playground. This review has a push embargo till the library has gone through review.

Overall I dislike the extensive usage of ifdefs. I would prefer if the accessibility feature becomes a mandatory requirement for this effect to build. That is if LibKdeAccessibilityClient is not found the effect does not get build. I think this would make the code much cleaner to read and also to maintain.


kwin/effects/CMakeLists.txt
<http://git.reviewboard.kde.org/r/106041/#comment13663>

    this should be moved up to the main CMakeLists.txt of kde-workspace.



kwin/effects/zoom/focustrackconfig.h.in
<http://git.reviewboard.kde.org/r/106041/#comment13664>

    this file should then also be dropped and the cmakedefine be added to the config-workspace.h.cmake



kwin/effects/zoom/zoom.h
<http://git.reviewboard.kde.org/r/106041/#comment13665>

    if I see correctly you don't need the include in the header. Please use forward declaration if possible



kwin/effects/zoom/zoom.h
<http://git.reviewboard.kde.org/r/106041/#comment13669>

    the pointer does not get initialised in the ctor. Please default init with NULL



kwin/effects/zoom/zoom.cpp
<http://git.reviewboard.kde.org/r/106041/#comment13666>

    where does this pointer get deleted? It seems to be no QObject (no parent passed) and no delete in the dtor.



kwin/effects/zoom/zoom.cpp
<http://git.reviewboard.kde.org/r/106041/#comment13668>

    coding style: either Qt or KDE-libs coding style (I personally prefer KDE-libs):
    
    if (!success) {
        kWarning(1212) << "Could not connect signal to slot";
    }



kwin/effects/zoom/zoom.cpp
<http://git.reviewboard.kde.org/r/106041/#comment13667>

    please set a deleted pointer to NULL:
    registry = NULL;



kwin/effects/zoom/zoom.cpp
<http://git.reviewboard.kde.org/r/106041/#comment13662>

    we don't use this define inside KWin and we use kDebug(1212) instead of qDebug(). This holds of course for all usage of qDebug()


- Martin Gräßlin


On Aug. 15, 2012, 3:07 p.m., Amandeep Singh wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106041/
> -----------------------------------------------------------
> 
> (Updated Aug. 15, 2012, 3:07 p.m.)
> 
> 
> Review request for KDE Accessibility, kwin, Frederik Gladhorn, Sebastian Sauer, and Luboš Luňák.
> 
> 
> Description
> -------
> 
> This patch is for KWin, makes the focus-tracking feature of KWin work with applications. This makes KDE more accessible.
> A new accessibility client library "libkdeaccessibilityclient" (which can be found here: https://projects.kde.org/projects/playground/accessibility/libkdeaccessibilityclient) is also to be added in kde-support.
> 
> The focus-tracking feature will use the new library to get the accessibility information from at-spi.
> 
> Adjustments in CMakeLists have been made to add the library as an optional dependency, which if found enables the focus-tracking option in Zoom Plugin.
> And in absence of library, the option is removed.
> 
> 
> Diffs
> -----
> 
>   kwin/effects/CMakeLists.txt 29accb1 
>   kwin/effects/zoom/CMakeLists.txt 36e11ac 
>   kwin/effects/zoom/focustrackconfig.h.in PRE-CREATION 
>   kwin/effects/zoom/zoom.h d809b21 
>   kwin/effects/zoom/zoom.cpp b275b1e 
>   kwin/effects/zoom/zoom_config.cpp 3edd10c 
> 
> Diff: http://git.reviewboard.kde.org/r/106041/diff/
> 
> 
> Testing
> -------
> 
> I tested KWin's new feature working with KWrite and Konsole.
> 
> 
> Thanks,
> 
> Amandeep Singh
> 
>

_______________________________________________
kwin mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/kwin