<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/123832/">https://git.reviewboard.kde.org/r/123832/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 17th, 2015, 4:46 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;">I'm sending this review request to gather feedback from the usability group on the UI changes this patch makes:
<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;"> Dropping use of <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">archive_error_string()</code> in the error messages. While on the error messages become less precise, using that function adds a piece of English text coming from libarchive to user-facing messages that are translated and lead to an ugly mixture of English and other languages in the same sentence. Additionally, <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">archive_error_string()</code> possibly returns a string that make more sense to the programmer than to users. If this makes sense I may change other error messages that make use of that function too.
</em> The actual contents of the error messages. Are they good enough?
* The decision to stop extracting/compressing when the first error is encountered, even if it is possible to proceed. I'm doing this because currently there is no good way to show a series of errors in the UI.</p></pre>
</blockquote>
<p>On May 17th, 2015, 4:49 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;">Ugh, the Markdown formatting of the previous comment got completely messed up and I am unable to edit or delete that comment. What's in italic is supposed to be one bullet point, and the final sentences not in italic are another one.</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;">Thank you for involving us!</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Now to your questions:</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Dropping use of archive_error_string() in the error messages. While on the error messages become less precise, using that function adds a piece of English text coming from libarchive to user-facing messages that are translated and lead to an ugly mixture of English and other languages in the same sentence. Additionally, archive_error_string() possibly returns a string that make more sense to the programmer than to users. If this makes sense I may change other error messages that make use of that function too.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I do not know exactly which strings that function returns, but I trust your judgment that these are probably mostly technical and therefore likely not useful to users anyway. I therefore support not showing them by default. What I think could make sense, though, is allowing users to show them on demand (e.g. with an expander), so that users who are either technically versed enough to understand it or who would like to paste it into their favorite search engine in hopes of finding help with their problem could do so.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The actual contents of the error messages. Are they good enough?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Be aware that we usability people are mostly non-coders and therefore do not know where in the patch we find the messages. Could you please put the messages in clear text in the review request description or a comment? Thanks!</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The decision to stop extracting/compressing when the first error is encountered, even if it is possible to proceed. I'm doing this because currently there is no good way to show a series of errors in the UI.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This could be problematic. Imagine there is an archive with lots of important files in it, and one of them (or maybe an actually unimportant file in it) is somehow corrupted, causing an error. With the proposed behavior, it would not be possible to extract the other files anymore.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm doing this because currently there is no good way to show a series of errors in the UI.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Would it be possible to continue extracting, collect all messages that occured and then show them all in one dialog at the end? If not, I'd rather ask they user after each error whether they want to continue extracting. this would make the process quite cumbersome if there are several errors but a user wants to continue anyway, but at least it would still be possible (and if a user just doesn't want to bother, they can still decide to cancel the extraction at the first error).</p></pre>
<br />
<p>- Thomas</p>
<br />
<p>On May 17th, 2015, 5:47 p.m. UTC, Raphael Kubo da Costa 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 Raphael Kubo da Costa.</div>
<p style="color: grey;"><i>Updated May 17, 2015, 5:47 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=335411">335411</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;">Errors while extracting an archive entry in copyFiles() were being
discarded without informing the user, who would then believe the entire
extraction had worked correctly. We now emit the error() signal when
there is an error and cancel the extraction.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Other callers of archive_write_header() have also been modified to use
the same coding style (handling errors in a switch()) and error message
format for consistency and ease of future maintenance.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Additionally, none of the error messages use archive_error_string()
anymore. While this means the messages are less detailed, it also means
users who use a translated KDE will not see part of the error messages
hardcoded in English.</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;">Creating archives still works as before, and error messages during extraction (such as the one in bug 335411) are properly reported.</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>plugins/libarchive/libarchivehandler.cpp <span style="color: grey">(75cf759d5e67508288ee6a42d42b4c0d6b557afe)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/123832/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>