Review Request: Adding Accessibility Interfaces for Dolphin Views & Widgets

[Frederik Gladhorn]

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



dolphin/src/kitemviews/kitemlistcontainer.cpp
<http://git.reviewboard.kde.org/r/105972/#comment13465>

    missing #ifndef QT_NO_ACCESSIBIILTY



dolphin/src/kitemviews/kitemlistcontainer.cpp
<http://git.reviewboard.kde.org/r/105972/#comment13464>

    I agree with Jose, remove the removeFactory.



dolphin/src/kitemviews/kitemlistcontainer.cpp
<http://git.reviewboard.kde.org/r/105972/#comment13468>

    #ifndef QT_NO_ACCESSIBIILTY



dolphin/src/kitemviews/kitemlistcontainer.cpp
<http://git.reviewboard.kde.org/r/105972/#comment13466>

    Why do you send LocationChanged here? I don't think it's right or relevant. We generally don't send geometry updates.



dolphin/src/kitemviews/kitemlistcontainer.cpp
<http://git.reviewboard.kde.org/r/105972/#comment13469>

    #ifndef QT_NO_ACCESSIBIILTY



dolphin/src/kitemviews/kitemlistcontainer.cpp
<http://git.reviewboard.kde.org/r/105972/#comment13467>

    Remove LocationChanged



dolphin/src/kitemviews/kitemlistcontainer.cpp
<http://git.reviewboard.kde.org/r/105972/#comment13470>

    #ifndef QT_NO_ACCESSIBIILTY and remove LocationChanged everywhere.



dolphin/src/kitemviews/kitemlistselectionmanager.cpp
<http://git.reviewboard.kde.org/r/105972/#comment13471>

    not needed, revert the whole file.



dolphin/src/kitemviews/kitemlistview.cpp
<http://git.reviewboard.kde.org/r/105972/#comment13472>

    I agree, you should only have one factory for all classes. Missing #ifndef QT_NO_ACCESSIBIILTY here again.



dolphin/src/kitemviews/kitemlistviewaccessible.h
<http://git.reviewboard.kde.org/r/105972/#comment13474>

    Qt itemview headers should not be needed here.



dolphin/src/kitemviews/kitemlistviewaccessible.h
<http://git.reviewboard.kde.org/r/105972/#comment13477>

    Doesn't make sense, you could have itemviews ng without the traditional itemviews.



dolphin/src/kitemviews/kitemlistviewaccessible.h
<http://git.reviewboard.kde.org/r/105972/#comment13475>

    use the same style of comment everywhere (either capitalize or not, either space after // or not)



dolphin/src/kitemviews/kitemlistviewaccessible.h
<http://git.reviewboard.kde.org/r/105972/#comment13476>

    Having the protected keyword twice doesn't make much sense.



dolphin/src/kitemviews/kitemlistviewaccessible.h
<http://git.reviewboard.kde.org/r/105972/#comment13478>

    Weird style (two spaces), parenthesis with extra spaces etc, see kde libs coding style.



dolphin/src/kitemviews/kitemlistviewaccessible.cpp
<http://git.reviewboard.kde.org/r/105972/#comment13473>

    dtor not needed, I'd prefer to remove it.



dolphin/src/kitemviews/kitemlistviewaccessible.cpp
<http://git.reviewboard.kde.org/r/105972/#comment13479>

    Never use "" for an empty string. You want QString().



dolphin/src/kitemviews/kitemlistviewaccessible.cpp
<http://git.reviewboard.kde.org/r/105972/#comment13480>

    Actually I don't think there is much to fix. The model change can be ignored, it's a broken concept that got into QAccessible from IAccessible2. Rather add a comment that it's ignored on purpose (and will be gone in Qt 5).


- Frederik Gladhorn


On Aug. 13, 2012, 5:50 a.m., Amandeep Singh wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105972/
> -----------------------------------------------------------
> 
> (Updated Aug. 13, 2012, 5:50 a.m.)
> 
> 
> Review request for Dolphin, KDE Base Apps and KDE Accessibility.
> 
> 
> Description
> -------
> 
> Added Accessibility Interfaces for Dolphin Views & Widgets, to make it accessible.
> 2 New files added in dolphin/ src/ kitemviews/ kitemlistviewaccessible.* that contain the three new classes.
> 
> 
> Diffs
> -----
> 
>   dolphin/src/CMakeLists.txt 5c1a6da 
>   dolphin/src/kitemviews/kitemlistcontainer.cpp 5500851 
>   dolphin/src/kitemviews/kitemlistcontroller.cpp 88f5d9f 
>   dolphin/src/kitemviews/kitemlistselectionmanager.cpp 383914d 
>   dolphin/src/kitemviews/kitemlistview.h 5723b9a 
>   dolphin/src/kitemviews/kitemlistview.cpp 72b3fd8 
>   dolphin/src/kitemviews/kitemlistviewaccessible.h PRE-CREATION 
>   dolphin/src/kitemviews/kitemlistviewaccessible.cpp PRE-CREATION 
>   dolphin/src/kitemviews/private/kitemlistviewlayouter.h da5bd1d 
>   dolphin/src/tests/CMakeLists.txt 3f906d1 
> 
> Diff: http://git.reviewboard.kde.org/r/105972/diff/
> 
> 
> Testing
> -------
> 
> Focus-tracking tested with KMag / KWin. 
> 
> 
> Thanks,
> 
> Amandeep Singh
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20120813/c24a0570/attachment.htm>
-------------- next part --------------
_______________________________________________
kde-accessibility mailing list
kde-accessibility at kde.org
https://mail.kde.org/mailman/listinfo/kde-accessibility