Review Request 129533: Dolphin: expand all one level, expand all, and collapse all in details view mode

[Emmanuel Pescosta]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129533/#review101281
-----------------------------------------------------------



Huge patch ... :)

Can you please add test cases and clean up your patch (remove unused and commented code)?

Thanks!


src/kitemviews/kfileitemmodel.cpp (lines 592 - 600)
<https://git.reviewboard.kde.org/r/129533/#comment67836>

    Add a new enum parameter to `setExpanded` to pass on some flags. Add an enum which has a `RememberPreviouslyExpandedChildren` (or a better name) item. Use this item as default for the new parameter and only insert the previously expanded children in `KFileItemModel::setExpanded` if it is requested by the client.
    
    When you iterate through the list of items, you have to start from the beginning to avoid the mentioned performance problems. `KFileItemModel::setExpanded` will automatically remove all child items of the currently collapsed item (the list in KFileItemModel looks like A, B, B-1, B-2, B-3, C and when you collapse B B-1 to B-3 will automatically be removed from the list).
    
    In the end the code for collapse-all should look like:
    
    for i = 0 .. count()-1 {
      setExpanded(i, false, YourFlags::None);
    }



src/kitemviews/kfileitemmodel.cpp (line 650)
<https://git.reviewboard.kde.org/r/129533/#comment67842>

    Unused? -> remove



src/kitemviews/kfileitemmodel.cpp (lines 691 - 698)
<https://git.reviewboard.kde.org/r/129533/#comment67841>

    for i = 0 .. count()-1 {
      setExpanded(i, true);
    }



src/kitemviews/kfileitemmodel.cpp (line 701)
<https://git.reviewboard.kde.org/r/129533/#comment67840>

    `urls` contains only urls which are already part of the model, right? Then just iterating through this set and simply call `setExpanded(index(url), true)` for each url should be enough.



src/kitemviews/kfileitemmodel.cpp (lines 734 - 744)
<https://git.reviewboard.kde.org/r/129533/#comment67843>

    same as `expandFull(KFileItemModel::directory())`


- Emmanuel Pescosta


On Dec. 4, 2016, 8:01 a.m., Don Nguyen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129533/
> -----------------------------------------------------------
> 
> (Updated Dec. 4, 2016, 8:01 a.m.)
> 
> 
> Review request for Dolphin and KDE Usability.
> 
> 
> Bugs: 196772
>     https://bugs.kde.org/show_bug.cgi?id=196772
> 
> 
> Repository: dolphin
> 
> 
> Description
> -------
> 
> Implemented functions:  Expand, Expand Full, and Collapse All.  They are available under the View and Control menus as well as custom toolbar buttons as Thomas explained and are disabled when not in details view mode.  They are also available via the context menu only in details view mode.  See screenshots.  
> Expand - If there is a selection, expand the selection once.  Otherwise expand everything once.  
> Expand Full - If there is a selection, expand the selected items completely.  Otherwise expand everything completely.
> Collapse All - Collapse everything.
> 
> 
> Diffs
> -----
> 
>   src/dolphincontextmenu.h f67300d 
>   src/dolphincontextmenu.cpp ed3f643 
>   src/dolphinmainwindow.cpp e5103fd 
>   src/dolphinui.rc f197af4 
>   src/kitemviews/kfileitemmodel.h 8a0df72 
>   src/kitemviews/kfileitemmodel.cpp 05f85a6 
>   src/views/dolphinview.h 0b0d819 
>   src/views/dolphinview.cpp 4105628 
>   src/views/dolphinviewactionhandler.h eb375b5 
>   src/views/dolphinviewactionhandler.cpp 858f929 
> 
> Diff: https://git.reviewboard.kde.org/r/129533/diff/
> 
> 
> Testing
> -------
> 
> make test passes
> manual testing including:
> expand all on an Android SDK folder which yielded in 3180 folders, and 30993 files
> collapse all on above situation
> expand all on a directory that contains a symbolic link to a directory containing itself - operation will stop with error when expansion reaches a path that KCoreDirLister doesn't accept(see looptest screenshot) 
> changing directories in the middle of an expand all operation
> changing directories in the middle of a restore expanded directories operation 
> canceling in the middle of an expand all operation
> canceling in the middle of a restore expanded directories operation
> canceling in the middle of an expand all operation or restore expand directory operation, and then expanding an item manually with mouse click
> collapse all in the middle of an expand all operation
> collapse all in the middle of a restore expanded directories operation
> switched view modes and verified items are enabled only in details view mode
> 
> 
> File Attachments
> ----------------
> 
> looptest
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/11/25/f02b7e85-d0b3-4153-ae0d-e86a4ddfc916__looptest.png
> context-menu-disabled
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/12/04/1fcc4412-e5f2-4c31-92bf-b828eea938f7__contextdisabled.png
> context-menu-with-selection
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/12/04/8a4709e5-d674-4f74-8cbc-a71841db7c36__selectioncontextenabled.png
> view-menu-enabled
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/12/04/1ddaa88d-e0aa-441a-a840-5e2cdc06fbf2__viewmenuenabled.png
> 
> 
> Thanks,
> 
> Don Nguyen
> 
>

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