<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/110672/">https://git.reviewboard.kde.org/r/110672/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On setembre 6th, 2014, 9:55 p.m. UTC, <b>Raphael Kubo da Costa</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Again, thanks a lot for your work on this patch.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I've left some inline comments mostly concerning your unit test, which is still broken (after I fixed it locally all tests passed).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">One problem I had when testing the code is that some unnecessary directories are still being created because of the algorithm used in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">getSelectionRoot()</code>. 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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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 <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QItemSelectionModel</code> and grouping all the selected items by their closest unselected parent (which is the root node) and then creating separate <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ExtractJob</code>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 <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ExtractJob</code> whose root node is "phonon-4.7.2/declarative" and another one whose root node is "phonon-4.7.2".</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
</blockquote>
<p>On setembre 6th, 2014, 10:17 p.m. UTC, <b>Raphael Kubo da Costa</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
</blockquote>
<p>On setembre 7th, 2014, 1:07 a.m. UTC, <b>Alim Gokkaya</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think 2080384 is more related to that unzip plugin ignores <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">RootNode</code> 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.</p></pre>
</blockquote>
<p>On setembre 18th, 2014, 9:13 p.m. UTC, <b>Albert Astals Cid</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Sorry i lost track, what's the status of the patch? Needs review or still needs work from Alim?</p></pre>
</blockquote>
<p>On setembre 19th, 2014, 11:39 p.m. UTC, <b>Raphael Kubo da Costa</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Both :)
There's work to do but I haven't written everything down to Alim yet (trying as best as I can).</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ping?</p></pre>
<br />
<p>- Albert</p>
<br />
<p>On setembre 6th, 2014, 3:40 p.m. UTC, Alim Gokkaya wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Utils, Albert Astals Cid and Raphael Kubo da Costa.</div>
<div>By Alim Gokkaya.</div>
<p style="color: grey;"><i>Updated set. 6, 2014, 3:40 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
ark
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch enables extracting multiple files using drag&drop by removing the code that singles the selection.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>part/archiveview.cpp <span style="color: grey">(6b9918d)</span></li>
<li>part/archivemodel.cpp <span style="color: grey">(4326268)</span></li>
<li>part/archivemodel.h <span style="color: grey">(7f8c527)</span></li>
<li>part/CMakeLists.txt <span style="color: grey">(9e38443)</span></li>
<li>part/part.h <span style="color: grey">(5379b9f)</span></li>
<li>part/part.cpp <span style="color: grey">(b4ebcd2)</span></li>
<li>part/test/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>part/test/archivemodeltest.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/110672/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>