[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-utils-devel
Subject: Re: Review Request 124050: [Ark] Check if file exists before attempting to preview it
From: "Raphael Kubo da Costa" <rakuco () FreeBSD ! org>
Date: 2015-06-10 8:30:50
Message-ID: 20150610083050.21202.32924 () mimi ! kde ! org
[Download RAW message or body]
--===============7000030516220726541==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
> On June 9, 2015, 6:03 p.m., Raphael Kubo da Costa wrote:
> > What backtrace are you getting for your crash?
> >
> > I've just tried the KDE4 version with the file you mentioned and it does not \
> > crash, though the previewed file is shown as a new file instead.
>
> Ragnar Thomsen wrote:
> After the fix in commit 901593b59e8b6c684f8e823f410e41c939b08b99, the file is also \
> previewed as a new file for me. I guess the proper way to address this would be \
> that the clizip plugin should generate an error during extraction of the file.
I think so, or we just switch away from infozip altogether as we were discussing \
yesterday. I feel like adding this kind of check in this method is too late and is \
most likely just working around a symptom of a larger problem anyway.
> On June 9, 2015, 6:03 p.m., Raphael Kubo da Costa wrote:
> > part/arkviewer.cpp, line 103
> > <https://git.reviewboard.kde.org/r/124050/diff/1/?file=379387#file379387line103>
> >
> > User messages should never have exclamation marks.
>
> Ragnar Thomsen wrote:
> Are there any KDE guidelines for message boxes, etc?
There is this one at least: \
https://techbase.kde.org/Projects/Usability/HIG/Exclamation_points
- Raphael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124050/#review81335
-----------------------------------------------------------
On June 9, 2015, 5:36 p.m., Ragnar Thomsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124050/
> -----------------------------------------------------------
>
> (Updated June 9, 2015, 5:36 p.m.)
>
>
> Review request for KDE Utils and Raphael Kubo da Costa.
>
>
> Repository: ark
>
>
> Description
> -------
>
> Attempting to preview a file that doesn't exist in the temporary directory crashes \
> Ark. Ideally, this should not happen because only files that exist in an archive \
> should be presented in the QTreeView. However, for the clizip plugin at least, \
> files with filenames containing special characters cause this plugin to not find \
> the file and hence not extract it, and Ark crashes attempting to view it.
> This patch adds a simple check to see if the file exists in the temporary directory \
> before attempting to view it. If the file doesn't exist (e.g. could not be \
> extracted by clizip), a KMessageBox::error() is displayed. Also an error is printed \
> in console output.
>
> Diffs
> -----
>
> part/arkviewer.cpp 2276c5e
>
> Diff: https://git.reviewboard.kde.org/r/124050/diff/
>
>
> Testing
> -------
>
> Tested with the Zip archive provided in \
> https://bugs.kde.org/show_bug.cgi?id=345519.
>
> Thanks,
>
> Ragnar Thomsen
>
>
--===============7000030516220726541==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 7bit
<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/124050/">https://git.reviewboard.kde.org/r/124050/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On June 9th, 2015, 6:03 p.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;">What backtrace are you getting for your crash?</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">I've just tried the KDE4 version with the file you mentioned and it does \
not crash, though the previewed file is shown as a new file instead.</p></pre> \
</blockquote>
<p>On June 10th, 2015, 8:45 a.m. EEST, <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 \
the fix in commit 901593b59e8b6c684f8e823f410e41c939b08b99, the file is also \
previewed as a new file for me. I guess the proper way to address this would be that \
the clizip plugin should generate an error during extraction of the file.</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 so, or we just switch away from infozip altogether as we were discussing \
yesterday. I feel like adding this kind of check in this method is too late and is \
most likely just working around a symptom of a larger problem anyway.</p></pre> <br \
/>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On June 9th, 2015, 6:03 p.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/124050/diff/1/?file=379387#file379387line103" \
style="color: black; font-weight: bold; text-decoration: \
underline;">part/arkviewer.cpp</a> <span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void \
ArkViewer::dialogClosed()</pre></td>
</tr>
</tbody>
<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">103</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">KMessageBox</span><span class="o">::</span><span \
class="n">error</span><span class="p">(</span><span class="n">parent</span><span \
class="p">,</span> <span class="n">i18n</span><span class="p">(</span><span \
class="s">"The selected file could not be found!"</span><span \
class="p">),</span> <span class="n">i18n</span><span class="p">(</span><span \
class="s">"File not found"</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;">User \
messages should never have exclamation marks.</p></pre> </blockquote>
<p>On June 10th, 2015, 8:45 a.m. EEST, <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;">Are \
there any KDE guidelines for message boxes, etc?</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;">There is this one at least: \
https://techbase.kde.org/Projects/Usability/HIG/Exclamation_points</p></pre> <br />
<p>- Raphael</p>
<br />
<p>On June 9th, 2015, 5:36 p.m. EEST, Ragnar Thomsen 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 Ragnar Thomsen.</div>
<p style="color: grey;"><i>Updated June 9, 2015, 5:36 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;">Attempting to preview a file that doesn't exist in the \
temporary directory crashes Ark. Ideally, this should not happen because only files \
that exist in an archive should be presented in the QTreeView. However, for the \
clizip plugin at least, files with filenames containing special characters cause this \
plugin to not find the file and hence not extract it, and Ark crashes attempting to \
view it.</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">This patch adds a simple check to see if the file \
exists in the temporary directory before attempting to view it. If the file doesn't \
exist (e.g. could not be extracted by clizip), a KMessageBox::error() is displayed. \
Also an error is printed in console output.</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;">Tested with the Zip archive provided in \
https://bugs.kde.org/show_bug.cgi?id=345519.</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/arkviewer.cpp <span style="color: grey">(2276c5e)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/124050/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============7000030516220726541==--
[Attachment #3 (text/plain)]
_______________________________________________
Kde-utils-devel mailing list
Kde-utils-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-utils-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic