<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/103690/">http://git.reviewboard.kde.org/r/103690/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 29th, 2012, 2:08 a.m., <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;">Thank you for working on this. There is still some work ahead, but I think the issues can be sorted out.
Besides the specific points I have raised below, a few general comments:
- There are many coding style issues with the patch, mostly related to white space.
- Trailing white space
- foo( bar ) and foo[ bar ] being used instead of foo(bar) and foo[bar]. Ark follows kdelibs' and Qt's coding conventions.
- Please add the "kdeutils" group to the review request.
- I disagree with the "DefaultApplication" action and the way it is triggered. It is totally not clear to the user that the default behavior is now to try to launch the default application associated with the file's mimetype than to use the previewer. This is also unrelated to the main objective of the patch.
- Trying to preview a file with the default application always crashes Ark here.
- The version in ark_part.rc needs to be bumped.</pre>
</blockquote>
<p>On January 29th, 2012, 9:04 a.m., <b>Volker Härtel</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;">Thanks for your review, I'll try to fix the issues soon. Can you give me a hint about the crashes on preview? I did not have any crashes.</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;">Relevant part of the backtrace:
#0 0x000000080574d40c in thr_kill () from /lib/libc.so.7
#1 0x00000008057e84ab in abort () from /lib/libc.so.7
#2 0x0000000804aa4e7f in qt_message_output (msgType=QtFatalMsg,
buf=0x80bdd7818 "ASSERT: \"!kioexec.isEmpty()\" in file /home/rakuco/kde4/src/kde/kdelibs/kio/kio/krun.cpp, line 422") at /usr/home/rakuco/kde4/src/qt-4.8/src/corelib/global/qglobal.cpp:2252
#3 0x0000000804aa4ff2 in qt_message(QtMsgType, const char *, typedef __va_list_tag __va_list_tag *)
(msgType=QtFatalMsg, msg=0x804c64960 "ASSERT: \"%s\" in file %s, line %d", ap=0x7fffffffac58)
at /usr/home/rakuco/kde4/src/qt-4.8/src/corelib/global/qglobal.cpp:2298
#4 0x0000000804aa5782 in qFatal (msg=0x804c64960 "ASSERT: \"%s\" in file %s, line %d")
at /usr/home/rakuco/kde4/src/qt-4.8/src/corelib/global/qglobal.cpp:2481
#5 0x0000000804aa4a3c in qt_assert (assertion=0x80117167f "!kioexec.isEmpty()",
file=0x801170c70 "/home/rakuco/kde4/src/kde/kdelibs/kio/kio/krun.cpp", line=422)
at /usr/home/rakuco/kde4/src/qt-4.8/src/corelib/global/qglobal.cpp:1999
#6 0x0000000801088b44 in KRun::processDesktopExec (_service=..., _urls=..., tempFiles=true,
suggestedFileName=...) at /home/rakuco/kde4/src/kde/kdelibs/kio/kio/krun.cpp:422
#7 0x000000080108a2fa in runTempService (_service=..., _urls=..., window=0x80bd16f10,
tempFiles=true, suggestedFileName=..., asn=...)
at /home/rakuco/kde4/src/kde/kdelibs/kio/kio/krun.cpp:709
#8 0x000000080108a8e8 in KRun::run (_service=..., _urls=..., window=0x80bd16f10, tempFiles=true,
suggestedFileName=..., asn=...) at /home/rakuco/kde4/src/kde/kdelibs/kio/kio/krun.cpp:988
#9 0x000000080108c9a4 in KRun::runUrl (u=..., _mimetype=..., window=0x80bd16f10, tempFile=true,
runExecutables=true, suggestedFileName=..., asn=...)
at /home/rakuco/kde4/src/kde/kdelibs/kio/kio/krun.cpp:183
#10 0x000000080f622ef7 in Ark::Part::slotPreviewExtracted (this=0x80bc62100, job=<optimized out>)
at /home/rakuco/kde4/src/kdeutils/master/ark/part/part.cpp:594</pre>
<br />
<p>- Raphael</p>
<br />
<p>On January 29th, 2012, 9:04 a.m., Volker Härtel wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Utils and Raphael Kubo da Costa.</div>
<div>By Volker Härtel.</div>
<p style="color: grey;"><i>Updated Jan. 29, 2012, 9:04 a.m.</i></p>
<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;">Add an "open with" action to ark. Also add a context-menu (popup) for typical actions to ark. Change standard way to open files to preferred application.
This patch is based on https://svn.reviewboard.kde.org/r/6580/
</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;">Works for me</pre>
</td>
</tr>
</table>
<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=179066">179066</a>
</div>
<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.h <span style="color: grey">(daac59b)</span></li>
<li>part/archiveview.cpp <span style="color: grey">(6b9918d)</span></li>
<li>part/ark_part.rc <span style="color: grey">(044c11a)</span></li>
<li>part/part.h <span style="color: grey">(5379b9f)</span></li>
<li>part/part.cpp <span style="color: grey">(aa4756a)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/103690/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>