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/#review17283
-----------------------------------------------------------


Thanks for the patch and for your interest in improving Dolphin!

Some questions:

1. I don't know much about QAccesible. Could you describe briefly what kind of functionality your patch adds to Dolphin?

2. Please follow the coding style. In particular, always use {...} after 'if' and similar statements. Look at existing Dolphin code for reference or look at 
http://techbase.kde.org/Policies/Kdelibs_Coding_Style

3. KItemListSelectionManager does intentionally not depend on KItemListView and KItemListController, and it should stay this way. Please remove the corresponding includes from kitemlistselectionmanager.cpp. The accessibiliy update that you do in KItemListSelectionManager::setCurrentItem() now could also be done in the view's slot which is invoked by the selection manager's currentChanged() signal.

4. The new files you are proposing to add contain lots of commented-out code and are therefore not easy to review. Please fix that before uploading a new version of your patch.


- Frank Reininghaus


On Aug. 11, 2012, 9:34 a.m., Amandeep Singh wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105972/
> -----------------------------------------------------------
> 
> (Updated Aug. 11, 2012, 9:34 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 5c1a6dad58cb579ee85731d4bfa0ebe9a6a1bea4 
>   dolphin/src/kitemviews/kitemlistcontainer.cpp 5500851c8c92c564bf3130c66198cea9b61eb8c7 
>   dolphin/src/kitemviews/kitemlistcontroller.cpp 88f5d9f7cff1614fb8595b470d46916584710a27 
>   dolphin/src/kitemviews/kitemlistselectionmanager.cpp 383914df01e9964d60bf009db7af636bd52fd55e 
>   dolphin/src/kitemviews/kitemlistview.h 5723b9aaab26019ecad698f94c9a855ace35766d 
>   dolphin/src/kitemviews/kitemlistview.cpp 72b3fd8fcbfbaa43660fac359320c8227ade9063 
>   dolphin/src/kitemviews/kitemlistviewaccessible.h PRE-CREATION 
>   dolphin/src/kitemviews/kitemlistviewaccessible.cpp PRE-CREATION 
>   dolphin/src/kitemviews/private/kitemlistviewlayouter.h da5bd1d7d9205ea20bb6112bf3e8ccb01919dae2 
>   dolphin/src/tests/CMakeLists.txt 3f906d18767435080c6b6309ffce5ca2e6445728 
> 
> 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: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20120812/438fc274/attachment.htm>