<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 September 7th, 2014, 12:55 a.m. EEST, <b>Raphael Kubo da Costa</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/110672/diff/6/?file=310046#file310046line27" style="color: black; font-weight: bold; text-decoration: underline;">part/test/archivemodeltest.cpp</a>
<span style="font-weight: normal;">
(Diff revision 6)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">27</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="kt">void</span> <span class="n">ArchiveModelTest</span><span class="o">::</span><span class="n">initTestCase</span><span class="p">()</span></pre></td>
</tr>
</tbody>
</table>
<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;">This is wrong and makes the test crash the same way it did before.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You're only testing the model, so there's no need to create an application and a part just for that. <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QTEST_MAIN</code> below already creates a <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">main()</code> with some initialization, you're creating a second <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KAplication</code> here for no reason.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In other words: you can just create an <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ArchiveModel</code> directly in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">testGetSelectionRoot()</code> on the stack and be done with it. And consequently get rid of <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">initTestCase</code> and <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">debugGetArchiveModel()</code>.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Oh, and <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QTEST_MAIN</code> will create a <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QCoreApplication</code> instead of a <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QApplication</code> if you don't define <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QT_GUI_LIB</code>. Your life will be easier if you <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">#include <qtest_kde.h></code> and use <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QTEST_KDEMAIN(ArchiveModelTest, GUI)</code> instead. See Ark's <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">archivetest.cpp</code> and the documentation in kdelibs's <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kdecore/util/qtest_kde.h</code>.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">One final catch for everything to work: remember to add <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">../archivemodel.cpp</code> to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">archivemodeltest_SRCS</code> and link <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kerfuffle</code> into your test (I'm not sure if you went the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">initTestCase</code> route because you got build errors when trying to create an <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ArchiveModel</code> yourself.</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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;">That's what I was trying to achive, thanks for clearing this out. I got runtime errors while creating an instance of <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ArchiveModel</code>. Adding <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QApplication</code> was working for me. I moved to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">initTestCase</code> after adding <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">arkpart</code> but like you said, we don't really need the part itself.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 7th, 2014, 12:55 a.m. EEST, <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 September 7th, 2014, 1:17 a.m. EEST, <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>
</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;">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>
<br />
<p>- Alim</p>
<br />
<p>On September 6th, 2014, 6:40 p.m. EEST, 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 Sept. 6, 2014, 6: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>