Review Request: Restore ability to select a groups of files in Dolphin by clicking a group title

[Todd]

> On 2010-01-30 10:44:12, Peter Penz wrote:
> >

I have implemented the changes, but it will take me a few minutes to test them and publish the diff (more if there are any problems, which there probably are).


> On 2010-01-30 10:44:12, Peter Penz wrote:
> > /trunk/KDE/kdebase/apps/dolphin/src/dolphiniconsview.cpp, line 177
> > <http://reviewboard.kde.org/r/2764/diff/1/?file=18133#file18133line177>
> >
> >     Could you please merge the two if's into one if?
> >     
> >     if (!index.isValid() && (QApplication::mouseButtons() & Qt::MidButton)) ...

Done


> On 2010-01-30 10:44:12, Peter Penz wrote:
> > /trunk/KDE/kdelibs/kdeui/itemviews/kcategorizedview.cpp, line 58
> > <http://reviewboard.kde.org/r/2764/diff/1/?file=18135#file18135line58>
> >
> >     just writing
> >     , pastSelected()
> >     is sufficient to have a defined initialization

I changed this, but might leaving this in anyway make things more clear to other developers? They would be able to see what pastSelected contains without references the header file.


> On 2010-01-30 10:44:12, Peter Penz wrote:
> > /trunk/KDE/kdelibs/kdeui/itemviews/kcategorizedview.cpp, line 926
> > <http://reviewboard.kde.org/r/2764/diff/1/?file=18135#file18135line926>
> >
> >     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.

I've done this, I split it into two functions.  One selects all items in a list, the other determines whether all items in a range are selected.  The latter is not very generalizable, but should work fine within the limitations of KCategorizedView.  I am somewhat surprised the former is not already in Qt, though (I may have missed it).


> On 2010-01-30 10:44:12, Peter Penz wrote:
> > /trunk/KDE/kdelibs/kdeui/itemviews/kcategorizedview.h, line 175
> > <http://reviewboard.kde.org/r/2764/diff/1/?file=18134#file18134line175>
> >
> >     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...

I changed the first thing.  I was trying to keep it similar to indexAt.

I don't think your name is a good one.  Based on the naming elsewhere, a category includes the category drawer (title and surrounding graphics) and all the items under that category drawer.  So based on that naming convention "categoryIndexAt" would return the category's index no matter where in the category you clicked.  However, the function only returns a category's index if you click on the drawer.  So I think "categoryIndexAt" is misleading.  But I agree about the index part, so I changed it to "drawerIndexAt".  I don't see any other use of "drawer" in this context that could make this ambiguous, and "categoryDrawerIndexAt" is getting too long in my opinion.

I have also added a brief description.


- Todd


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


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