Review Request 110672: Allow multiple selection in archiveview

[Albert Astals Cid]

On set. 6, 2014, 9:55 p.m., Alim Gokkaya wrote:
> > Again, thanks a lot for your work on this patch.
> > 
> > I've left some inline comments mostly concerning your unit test, which is still broken (after I fixed it locally all tests passed).
> > 
> > One problem I had when testing the code is that some unnecessary directories are still being created because of the algorithm used in `getSelectionRoot()`. If I take phonon-4.7.2.tar.xz again and drag and drop "phonon-4.7.2/declarative/abstractinitable.h" and "phonon-4.7.2/phonon.pc.cmake", I end up with "declarative/abstractinitable.h" and "phonon.pc.cmake" -- the "declarative/" part should not be there. This is related to bug 208384, which I'm beginning to believe should be addressed together with this one.
> > 
> > This led me to think about other solutions that didn't involve this amount of string manipulation in the first place. It's been a long time since I last had to do any serious ItemView programming with Qt so I'm a bit rusty, but I'm fairly sure we can exploit it some more.
> > 
> > I've started a branch called "multiple-dnd-selection" that attempts to use this approach: instead of processing all selected files as strings, I'm processing the view's `QItemSelectionModel` and grouping all the selected items by their closest unselected parent (which is the root node) and then creating separate `ExtractJob`s for them based on this root node. I think this is the only way to solve bug 208384. Going back to the Phonon example, there would be one `ExtractJob` whose root node is "phonon-4.7.2/declarative" and another one whose root node is "phonon-4.7.2".
> > 
> > This is an investigative work in progress; those jobs need to be properly serialized and only the libarchive backend is working as expected at the moment -- please take a look at my commit there, see if it makes sense and tell me what you think.
> 
> Raphael Kubo da Costa wrote:
>     Oh and before I forget: if my idea works it won't mean the unit test code will be discarded; I'd like to make sure everything's working, then move the code elsewhere and test it like you do in your patch.
> 
> Alim Gokkaya wrote:
>     I think 2080384 is more related to that unzip plugin ignores `RootNode` directive and creates whole structure (I personnaly prefer that the "declerative/" part to remain). I think we can easily get rid of that part by not creating any folder structure at all (unless the folder is explicitly selected). So, it could be better if we let user choose to create directory structure or not.
> 
> Albert Astals Cid wrote:
>     Sorry i lost track, what's the status of the patch? Needs review or still needs work from Alim?
> 
> Raphael Kubo da Costa wrote:
>     Both :)
>     There's work to do but I haven't written everything down to Alim yet (trying as best as I can).

Ping?


- Albert


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


On set. 6, 2014, 3:40 p.m., Alim Gokkaya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/110672/
> -----------------------------------------------------------
> 
> (Updated set. 6, 2014, 3:40 p.m.)
> 
> 
> Review request for KDE Utils, Albert Astals Cid and Raphael Kubo da Costa.
> 
> 
> Repository: ark
> 
> 
> Description
> -------
> 
> This patch enables extracting multiple files using drag&drop by removing the code that singles the selection.
> 
> 
> Diffs
> -----
> 
>   part/archiveview.cpp 6b9918d 
>   part/archivemodel.cpp 4326268 
>   part/archivemodel.h 7f8c527 
>   part/CMakeLists.txt 9e38443 
>   part/part.h 5379b9f 
>   part/part.cpp b4ebcd2 
>   part/test/CMakeLists.txt PRE-CREATION 
>   part/test/archivemodeltest.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/110672/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alim Gokkaya
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-utils-devel/attachments/20150102/e9e02033/attachment.html>