----------------------------------------------------------- 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 > >
