<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/120794/">https://git.reviewboard.kde.org/r/120794/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 28th, 2014, 9:32 a.m. UTC, <b>David Faure</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/120794/diff/2/?file=322403#file322403line785" style="color: black; font-weight: bold; text-decoration: underline;">src/application.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<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">784</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="kt">void</span> <span class="n">Application</span><span class="o">::</span><span class="n">processCmdLine</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;">It's a bit confusing because QCLP::process() is not the same, and also because of the other processCmdLine method here. Maybe call this one handleCmdLine()? Sorry for nitpicking, but if we are going to replace this in most other apps, might as well be perfect :-)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">parse = just the parsing
process = parse and handle the builtin options (in QLCP)
handle = handle the app options</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As far as the slot is concerned, it's ok to say that Application::processCmdLine is parse + handle the app options, it's "builtin to Application". So I would rename this method, but leave the other one (processCmdLine(arguments)) as it is.</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;">This is up for review precisely to get it nitpicked :)</p></pre>
<br />
<p>- David</p>
<br />
<p>On October 28th, 2014, 5:44 a.m. UTC, David Narváez 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 Frameworks and rekonq.</div>
<div>By David Narváez.</div>
<p style="color: grey;"><i>Updated Oct. 28, 2014, 5:44 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
rekonq
</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 is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that.</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;"><ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Open one Rekonq window</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Try opening Rekonq again</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Try opeining Rekonq from a command line with some URLs</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in)</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Open rekonq from the console using rekonq --incognito</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Open rekonq from the console using reknoq --webapp twitter.com</li>
</ol>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Before this patch, nothing happens in steps 2 - 6. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly, step 5 opens an incognito window and step 6 opens a webapp window (simple window).</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>src/application.h <span style="color: grey">(7ccd60d)</span></li>
<li>src/application.cpp <span style="color: grey">(c7c297d)</span></li>
<li>src/main.cpp <span style="color: grey">(7592f7a)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120794/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>