[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-core-devel
Subject: Re: Review Request: Restore ability to select a groups of files in
From: "Peter Penz" <peter.penz () gmx ! at>
Date: 2010-01-30 10:44:02
Message-ID: 20100130104402.10418.60087 () localhost
[Download RAW message or body]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/2764/#review3978
-----------------------------------------------------------
/trunk/KDE/kdebase/apps/dolphin/src/dolphiniconsview.cpp
<http://reviewboard.kde.org/r/2764/#comment3354>
Could you please merge the two if's into one if?
if (!index.isValid() && (QApplication::mouseButtons() & Qt::MidButton)) ...
/trunk/KDE/kdelibs/kdeui/itemviews/kcategorizedview.h
<http://reviewboard.kde.org/r/2764/#comment3355>
Is there a reason why this method is public and virtual? Currently I don't see a \
usecase for making this method public and would suggest making it private + \
non-virtual (at least until we have a usecase).
I think the name is confusing: the name indicates that a categoryDrawer is \
returned (see indexAt()), but an index is returned... Internally you use \
'categoryIndex' - why not name the method 'categoryIndexAt(...)'? Please also add a \
description to the method what it really does, as without checking the implementation \
it is impossible to know...
/trunk/KDE/kdelibs/kdeui/itemviews/kcategorizedview.cpp
<http://reviewboard.kde.org/r/2764/#comment3357>
just writing
, pastSelected()
is sufficient to have a defined initialization
/trunk/KDE/kdelibs/kdeui/itemviews/kcategorizedview.cpp
<http://reviewboard.kde.org/r/2764/#comment3356>
I'm not too familiar with the KCategorizedView internals and cannot judge how \
performant the code is. However I think the performance is a no-brainer for this \
usecase.
Still I would suggest to split the implementation into (several?) private methods \
and add descriptions what the code does. Currently there is a nesting depth of 7 (!) \
and it is very tricky keep an overview what the code does.
Maybe moving the code from line 936 to 956 into a private method would already be \
a big benefit.
- Peter
On 2010-01-30 06:46:08, Todd wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2764/
> -----------------------------------------------------------
>
> (Updated 2010-01-30 06:46:08)
>
>
> Review request for Dolphin and kdelibs.
>
>
> Summary
> -------
>
> This patch restores the ability to select a group of files in Dolphin by clicking \
> on the group's title. This is a feature regression between 4.3 and 4.4 Unlike the \
> original version, which could only select groups, this implementation allows for \
> selecting and deselecting groups (Peter Penz said he considered the lack of \
> deselecting to be a bug).
> The patch also removes a workaround in Dolphin that does not appear to be necessary \
> anymore and was preventing the group selection from working.
> I know this is probably not the prettiest solution, but it does work. I made a \
> couple of speed improvements in corner cases, which should be clear from the diff, \
> but these could be removed without altering the functionality of the code. I am \
> open to suggestions on improvements. I would also appreciate it if other people \
> tested it as well.
>
> This addresses bug 214859.
> https://bugs.kde.org/show_bug.cgi?id=214859
>
>
> Diffs
> -----
>
> /trunk/KDE/kdebase/apps/dolphin/src/dolphiniconsview.cpp 1082212
> /trunk/KDE/kdelibs/kdeui/itemviews/kcategorizedview.h 1082212
> /trunk/KDE/kdelibs/kdeui/itemviews/kcategorizedview.cpp 1082212
> /trunk/KDE/kdelibs/kdeui/itemviews/kcategorizedview_p.h 1082212
>
> Diff: http://reviewboard.kde.org/r/2764/diff
>
>
> Testing
> -------
>
> Selected and deselected groups in Dolphin under various circumstances.
>
>
> Thanks,
>
> Todd
>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic