<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 Januar 12th, 2015, 1:58 nachm. UTC, <b>Thomas Pfeiffer</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;">Here is a UI review:
1. Please get rid of the group boxes, they don't serve any purpose here whatsoever.
2. Having a separate tab for the "Show info panel" checkbox doesn't make sense. Actually, this is an option that could very well stay directly in the settings menu instead of the config dialog, because it's simply showing vas, hiding a UI elemement. Most other applications keep such options in the Settings or View menu. The settings dialog is usually reserved for settings on which changes are not immediately visible in the UI, just like "Show status bar". So please move it back to the Settings menu.</p></pre>
</blockquote>
<p>On Januar 12th, 2015, 2:40 nachm. 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;">Thanks for your review. What about the scroll area container? I just realized that most applications don't use it at all (e.g. Dolphin, Kate, Konsole, Gwenview, ...).
Here the resulting screenshots:</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">without scroll area: http://abload.de/img/ark-settingswqkg5.png</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">with scroll area: http://abload.de/img/ark-settings-scrollareukmv.png</li>
</ul></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;">Since there are so few settings that it will never need to scroll, you should remove the scroll area container as well.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And now that there is only one "tab", does the sidebar even still have to be shown? It doesn't really have any use anymore now.</p></pre>
<br />
<p>- Thomas</p>
<br />
<p>On Januar 12th, 2015, 12:14 nachm. 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 Jan. 12, 2015, 12:14 nachm.</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>part/ark_part.rc <span style="color: grey">(044c11a562d03589314f05e86eb1d68e633ee35e)</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>app/arkui.rc <span style="color: grey">(6775c3310533ff164ed083da770643024947d490)</span></li>
<li>app/mainwindow.h <span style="color: grey">(5dd7fb82e3f5a48c9bdbefcef2a83c3e990482ca)</span></li>
<li>app/mainwindow.cpp <span style="color: grey">(aee823174a0b731e125381be09181938cdb6dd7d)</span></li>
<li>kerfuffle/ark.kcfg <span style="color: grey">(97d2086688698e96c429def089c50ff3cdbe4c4e)</span></li>
<li>part/CMakeLists.txt <span style="color: grey">(9e384438b60322f1d51d31e40c556b2912970ceb)</span></li>
<li>part/interface.h <span style="color: grey">(40f590284502d23a2a4ffaa333bfd5b63e6ec773)</span></li>
<li>part/part.h <span style="color: grey">(5379b9fc1aaa4ce451c8b1745ca46ee78630b005)</span></li>
<li>part/part.cpp <span style="color: grey">(09fe1cbfcc7f4345fe12932055dcb041f50abb7b)</span></li>
<li>part/dialogs/mainwindowsettings.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>part/dialogs/mainwindowsettings.ui <span style="color: grey">(PRE-CREATION)</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/01/12/76411a57-e9ed-4aec-8265-0608beb3f221__ark-settings1.png">ark-settings1.png</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/01/12/7a82816b-17dd-4526-8424-5b0c1162ca76__ark-settings2.png">ark-settings2.png</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>