<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="https://git.reviewboard.kde.org/r/117175/">https://git.reviewboard.kde.org/r/117175/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 30th, 2014, 11:23 a.m. UTC, <b>Lamarque Souza</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;">Hi, there is an error when trying to show the patch on reviewboard. Can you provide the correct patch?
I looked into the raw patch and I think the "return 1" line that you commented should be kept when the action is not upgrade.</pre>
</blockquote>
<p>On March 30th, 2014, 12:34 p.m. UTC, <b>Andrei Amuraritei</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;">The case tested there are actions 'remove' or 'upgrade'. Since that gets called for a remove action there is a delete installer in case of failure to remove the package as in line 409 and 410.
For upgrade action if the "return 1" line is left then there is no install or upgrade action executed further down with the package.
</pre>
</blockquote>
<p>On March 30th, 2014, 2:13 p.m. UTC, <b>Lamarque Souza</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;">I agree that the code should not return when action is 'upgrade'. When action is 'remove' and nothing is actually removed (because there is no package to remove in this case) runKbuildsycoca() will be called unnecessarily some lines later, that is what I am trying to avoid. As long as calling runKbuildsycoca() here is ok for the other reviewers I am ok with your patch.</pre>
</blockquote>
<p>On March 30th, 2014, 6:14 p.m. UTC, <b>Andrei Amuraritei</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;">runKbuildsycoca() doesn't get called unless there's a package specified with an action to execute. The check package.isEmpty() asures that.</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;">The lines below are at the beginning of function the code in question resides:
if (args->isSet("remove")) {
package = args->getOption("remove");
}
So package will not be empty when using 'remove' action, then runKbuildsycoca() will get called because the check package.isEmpty() will return false. Am I correct?</pre>
<br />
<p>- Lamarque</p>
<br />
<p>On March 30th, 2014, 1:13 p.m. UTC, Andrei Amuraritei wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Runtime, Albert Astals Cid, Aaron J. Seigo, Ian Monroe, and Lamarque Souza.</div>
<div>By Andrei Amuraritei.</div>
<p style="color: grey;"><i>Updated March 30, 2014, 1:13 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=306279">306279</a>,
<a href="http://bugs.kde.org/show_bug.cgi?id=325028">325028</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kde-runtime
</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;">When installing a new .comic provider from GHNS, it doesn't appear in the installed list on the comic widget.
This fixes it.</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;">Compile, add new comic widget, install new comic providers.</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>plasma/tools/plasmapkg/main.cpp <span style="color: grey">(61492fe)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/117175/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>