[Kde-accessibility] Review Request: Adding Accessibility Interfaces for Dolphin Views & Widgets

[Frank Reininghaus]

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


There might be a couple more things than those that I've pointed out below, but I have to leave now, and you have enough on you TODO-list now I suppose, so I'll wait for the next version of the patch :-)


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

    You call QAccessible::updateAccessibility() with the 'Focus' argument in KItemListView::slotCurrentChanged(), so there should be no need to do that anywhere else.
    
    Considering this and the comments by Frederik, I think all changes to KItemListContainer can be removed, right?



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

    The other one is probably not needed anyway, see my comment above.



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

    Coding style: Use {...}, also in other places where you use 'if'.



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

    On the other hand, I see lots of QModelIndex and friends here, which is something I don't particularly like. After all, the basic idea of the new view engine is to get rid of Qt's itemviews.
    
    Is there an easy way to do it without Qt's itemviews?



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

    Members get a prefix "m_" in Dolphin code (also in other places, I'm just pointing out this one).



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

    I think this will fail when grouping is enabled.



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

    I think it's preferred to use i18n() and friends for translatable strings, see http://techbase.kde.org/Development/Tutorials/Localization/i18n


- Frank Reininghaus


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-accessibility/attachments/20120813/1a75554b/attachment.html>