Review Request: Bugfix for Bug 303133 - wrong text color in places

[Frank Reininghaus]

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


Thanks for the patch! Looks very good overall! Just two little ideas how you could make it even better:

1. Maybe it might be better to derive your new class FoldersItemListWidget from KFileItemListWidget rather than KStandardItemListWidget? I think that symbolic links won't be shown in italics otherwise.

2. A possibility to reduce code duplication and make the code more future-proof: rather than making KStandardItemListWidget::textColor() virtual and overriding it in derived classes, one could make a small change in KStandardItemListWidget::textColor(): replace QPalette::Text by a call to a new virtual function normalTextColor(), make KStandardItemListWidget::normalTextColor() return QPalette::Text and let it return QPalette::WindowText in the derived classes. This could have two small advantages:

a) If any change in the 3 code lines that determine the color is required, we only have to change it in KStandardItemListWidget::textColor(), rather than in three different places.
b) We might decide at some point that setting a custom text color for some items in the panels might be necessary.

But these are just ideas - if you think that there are good reasons to not do it this way, just let me know.

- Frank Reininghaus


On Aug. 2, 2012, 9:05 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105832/
> -----------------------------------------------------------
> 
> (Updated Aug. 2, 2012, 9:05 p.m.)
> 
> 
> Review request for Dolphin and Frank Reininghaus.
> 
> 
> Description
> -------
> 
> Fixes wrong text color in places and in folders panel.
> 
> 
> This addresses bug 303133.
>     http://bugs.kde.org/show_bug.cgi?id=303133
> 
> 
> Diffs
> -----
> 
>   dolphin/src/panels/folders/foldersitemlistwidget.cpp PRE-CREATION 
>   dolphin/src/panels/folders/folderspanel.cpp 78e437a 
>   dolphin/src/panels/places/placesitemlistwidget.h d7a4f3d 
>   dolphin/src/panels/places/placesitemlistwidget.cpp 24c2b3f 
>   dolphin/src/CMakeLists.txt 5c1a6da 
>   dolphin/src/kitemviews/kstandarditemlistwidget.h 82b68ea 
>   dolphin/src/panels/folders/foldersitemlistwidget.h PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/105832/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20120804/87c52360/attachment.htm>