Re: Review Request: Adding Accessibility Interfaces for Dolphin Views & Widgets

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


Thanks for the quick update, looks much better now! I couldn't check everything because I have to leave now, but here are some more things that I noticed. 


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

    In Dolphin's code, the space is always behind the '*' for pointers and behind the '&' for references. Please fix that here and in some other places.
    
    (side note: I don't want to make the layouter accessible outside KItemListView, but I see why you need it, and making the row count and column count accessible in a better way is a job for me, so just leave it like it is in your patch for the moment).



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

    Dolphin does currently not have any function bodies inside the class declarations, and I'd like to keep it this way.
    
    You can leave the QModelIndex-related functions which aren't needed and just consist of {} inside the class declaration though (AFAIU, they can be dropped when using Qt 5 anyway).



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

    This class isn't needed any more, is it?



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

    Plese move the colon (:) to the line above (look at other Dolphin code for reference).



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

    How exactly should this be fixed? Looks like you return an uninitialised variable at the moment?



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

    This will definitely break if either grouping is enabled or the last row of items isn't full.
    
    Better: return view()->model()->count()



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

    Please add empty lines before and after this function.



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

    KItemListAccessibleCell::KItemListAccessibleCell(KItemListView *view, int index) :
        m_view(view),
        m_index(index)
    
    (look at other Dolphin code for reference)



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

    st -> state
    
    Don't abbreviate variable names unless there is a very good reason for it.



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

    r -> rect



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

    You're right, calling the widget's setData() method should normally only be done by the KItemListView when the data in the model change.


- Frank Reininghaus


On Aug. 14, 2012, 3:01 p.m., Amandeep Singh wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105972/
> -----------------------------------------------------------
> 
> (Updated Aug. 14, 2012, 3:01 p.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 afc190f 
>   dolphin/src/kitemviews/kitemlistcontroller.cpp 88f5d9f 
>   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
> 
>