Review Request 109893: Fixed a Dolphin 2.x Regression: no option to select whole group in grouped view

[Frank Reininghaus]

> On April 9, 2013, 6:34 a.m., Frank Reininghaus wrote:
> > Thanks for working on this! I appreciate that you work on re-implementing this lost feature. However (I might have said it earlier), I think that any plans to make large changes in the code should be discussed in advance on the mailing list. At least you should add a few explanations why you add new classes and change lots of things. Note that when I submit code to anyone, I always try to explain all design choices to make it easy for people to understand and review it. 
> > 
> > I see that you spent lots of time on this, and I really appreciate it, but I cannot review such a patch (much of which affects code that I'm not familiar with at all) at the moment. After having moved recently, I'm without Internet connection at home for at least another week, and I can't spend hours at work trying to figure out what all this is about, sorry. Maybe others can have a look and say what they think about it.
> 
> Emmanuel Pescosta wrote:
>     > I think that any plans to make large changes in the code should be discussed
>     When I started to implement this feature, I didn't know that this change will end in so much changed/new code.
>     
>     Sorry for that.
>     
>     But I hope that my comments help you a little bit ;)

Thanks for your explanations!

I tested your patch - unfortunately, it crashes reproducibly as soon as I hover one of the "group selection toggles".

To be honest, after thinking about it some more, I'm not sure if we should implement this feature at all. I see that KCategorizedView had it (the class that was used for grouping in the Icons View in Dolphin 1.x) and that the bug report got some votes, but I doubt that the feature is really useful for a large part of our target user group.

Therefore, I think that the possible benefits that this feature might bring are too small to justify the added future maintenance burden from the new code.

Even if some users will hate me for this, I believe that the new grouping functionality in Dolphin 2.x is far superior to the one in KCategorizedView, not only because it can be used in all view modes, but also because it behaves more consistently with respect to selection and some other things, and that dropping this feature is an acceptable price for that. I believe that one cannot add new features all the time and at the same time preserve every 'old' feature. In the long term, this approach would make the code unmaintainable.

I'm sorry about that. This issue can of course be revisited by other future Dolphin maintainers, but I think that we should not add that feature for the time being.


- Frank


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


On April 7, 2013, 6:04 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109893/
> -----------------------------------------------------------
> 
> (Updated April 7, 2013, 6:04 p.m.)
> 
> 
> Review request for Dolphin and Frank Reininghaus.
> 
> 
> Description
> -------
> 
> Fixed a Dolphin 2.x Regression: no option to select whole group in grouped view
> 
> Added the (de-)selector widget to the item list group-header, to (de-)select all items in the group.
> 
> This feature got lost in Dolphin 2.0
> 
> 
> This addresses bug 292508.
>     http://bugs.kde.org/show_bug.cgi?id=292508
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/private/dolphingraphicswidget.cpp PRE-CREATION 
>   dolphin/src/kitemviews/private/dolphingraphicswidget.h PRE-CREATION 
>   dolphin/src/kitemviews/kstandarditemlistwidget.cpp 6adb546 
>   dolphin/src/kitemviews/kstandarditemlistgroupheader.cpp 1e23c0a 
>   dolphin/src/kitemviews/kitemlistwidget.cpp 6a7111a 
>   dolphin/src/kitemviews/kitemlistwidget.h 55181fa 
>   dolphin/src/kitemviews/kitemlistview.cpp 9ebad7f 
>   dolphin/src/kitemviews/kitemlistview.h cd59ddc 
>   dolphin/src/kitemviews/kitemlistgroupheader.cpp 17c95a9 
>   dolphin/src/kitemviews/kitemlistgroupheader.h 1e8ed2c 
>   dolphin/src/kitemviews/kitemlistcontroller.cpp c6239df 
>   dolphin/src/CMakeLists.txt ffb232c 
>   dolphin/src/kitemviews/kitemlistcontroller.h 4d5fee3 
> 
> Diff: http://git.reviewboard.kde.org/r/109893/diff/
> 
> 
> Testing
> -------
> 
> Works fine. All tests passed.
> 
> * Enable grouping
> * Hover the group header
> * Click on the selector -> All items in this group are selected
> * Click on the selector again -> No item in this group is selected
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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