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