<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/121997/">https://git.reviewboard.kde.org/r/121997/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On giugno 3rd, 2015, 5:59 p.m. UTC, <b>Ragnar Thomsen</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 tested it and it works great :) thanks for doing this.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think we need to discuss how Ark remembers the extraction settings now that we have two places where they can be set. I see these possibilities:</p>
<ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Always use the settings from the settings page. This means that if the user changes the settings from the extraction dialog, then these settings are used only for that particular extraction. We should emphasize in the settings dialog, that these are the default settings.</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">When the user changes the settings in the extraction dialog, store these settings and use them in the future. This means making sure the settings page displays the updated settings.</li>
</ol>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What do you think? I think option one makes most sense.</p></pre>
</blockquote>
<p>On giugno 3rd, 2015, 6:19 p.m. UTC, <b>Elvis Angelaccio</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;">Option two is what is already done by Ark. We could migrate to option one, but changing the behavior of the extraction dialog would be too risky, imho. Long-time Ark users could be confused/annoyed (e.g. "Why I have to check again option foo?!?"). Let's just say that the extraction dialog allows the user to quickly access those settings, without having to open the settings dialog first.</p></pre>
</blockquote>
<p>On giugno 3rd, 2015, 8:05 p.m. UTC, <b>Ragnar Thomsen</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;">After thinking more about this, I think also option 2 is probably better. Currently, however, when I open the extract dialog and change the settings and then either cancel or extract the files, and subsequently open the settings dialog the settings are not updated.</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;">Weird, this should not happen. I will investigate tomorrow.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On giugno 3rd, 2015, 5:59 p.m. UTC, <b>Ragnar Thomsen</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/121997/diff/5/?file=378719#file378719line27" style="color: black; font-weight: bold; text-decoration: underline;">kerfuffle/ark.kcfg</a>
<span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">25</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><label>Limit preview file size</label></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></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;">Why were the labels deleted here?</p></pre>
</blockquote>
<p>On giugno 3rd, 2015, 6:19 p.m. UTC, <b>Elvis Angelaccio</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;">They were not used, were they?
Plus, I rephrased them in the actual configuration dialog.</p></pre>
</blockquote>
<p>On giugno 3rd, 2015, 6:34 p.m. UTC, <b>Ragnar Thomsen</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;">But I think they should remain in the kcfg file. The documentation recommends they are always set.</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;">Ok, I didn't know that. I will re-insert the labels changing the text accordingly to the labels showed in the dialog.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On giugno 3rd, 2015, 5:59 p.m. UTC, <b>Ragnar Thomsen</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/121997/diff/5/?file=378726#file378726line23" style="color: black; font-weight: bold; text-decoration: underline;">part/dialogs/previewsettings.ui</a>
<span style="font-weight: normal;">
(Diff revision 5)
</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">23</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <string>Do not preview large files</string></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;">I propose to change the text to:
"Disable preview for files larger than:"</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">and then put the QSpinBox directly after this text (i.e. delete the QLabel).</p></pre>
</blockquote>
<p>On giugno 3rd, 2015, 6:19 p.m. UTC, <b>Elvis Angelaccio</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;">Are you also implying to drop the related boolean setting?</p></pre>
</blockquote>
<p>On giugno 3rd, 2015, 6:23 p.m. UTC, <b>Ragnar Thomsen</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;">No, of course not. The checkbox should remain. Just delete the label and put the spinbox directly after the text.</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;">It looks simpler, indeed...</p></pre>
<br />
<p>- Elvis</p>
<br />
<p>On giugno 3rd, 2015, 4:50 p.m. UTC, Elvis Angelaccio 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 and Raphael Kubo da Costa.</div>
<div>By Elvis Angelaccio.</div>
<p style="color: grey;"><i>Updated Giu. 3, 2015, 4:50 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=165314">165314</a>
</div>
<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 implements the standard Configuration Dialog (i.e. using <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KConfigDialog</code>) in Ark, as one would expect from a KDE application.
This feature has been requested in more than one bug. I choosed to target bug 165314 since the others are more like "add the config dialog to implement the feature <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">foo</em>".</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The widgets showed in the config dialog are provided by the Ark <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Part</code> interface (just one widget for now). This should help to show the Ark settings in, for instance, Konqueror's config dialog. A similar approach is done in Kate and Cantor, from what I have seen.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't like very much the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">document-save</code> icon used for the Extraction page, but I couldn't find any better.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;">Compile and run, then try to change some settings and check that they persist.</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>app/mainwindow.h <span style="color: grey">(f2366c8bd047e3484daa4eabeb4f4b790c437c4a)</span></li>
<li>app/mainwindow.cpp <span style="color: grey">(470b72d00d0544662f2c1f10dbc0638b99f993af)</span></li>
<li>kerfuffle/ark.kcfg <span style="color: grey">(97d2086688698e96c429def089c50ff3cdbe4c4e)</span></li>
<li>part/CMakeLists.txt <span style="color: grey">(cca6d25e1941cecde07d0cd342cc2602fbc5235e)</span></li>
<li>part/arkconfigpage.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>part/arkconfigpage.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>part/dialogs/extractionsettings.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>part/dialogs/extractionsettings.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>part/dialogs/previewsettings.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>part/dialogs/previewsettings.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>part/interface.h <span style="color: grey">(40f590284502d23a2a4ffaa333bfd5b63e6ec773)</span></li>
<li>part/part.h <span style="color: grey">(4c81da5541c1143ad4cfe9093f9c307145ac561e)</span></li>
<li>part/part.cpp <span style="color: grey">(ee7f75ff9acd8ea3c72aa5a400d713ffc6d1c7c4)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121997/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/06/03/3dfe3110-4f46-4c14-9b19-79060577a4c9__extraction-settings.png">extraction-settings.png</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/06/03/92e1e6a4-6b6a-4a2f-b149-809501173925__preview-settings1.png">preview-settings1.png</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/06/03/200d41a5-6f06-4bea-9f92-d8899b051bf4__preview-settings2.png">preview-settings2.png</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>