<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/127111/">https://git.reviewboard.kde.org/r/127111/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 20th, 2016, 7:46 p.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/127111/diff/1/?file=444588#file444588line428" style="color: black; font-weight: bold; text-decoration: underline;">src/filewidgets/kurlnavigator.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">428</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">void</span> <span class="nf">selectParentOfPreviousUrl</span><span class="p">(</span><span class="k">const</span> <span class="n">QUrl</span> <span class="o">&</span><span class="n">url</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;">Signal names usually end with "ed", they reflect a state change or an action change.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"select" here is what you want the app to do (now that I read the bug report; otherwise it was very unclear just from the API docs), but you have no guarantee that the app will do that.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">void urlChangedToParent(const QUrl &ancestorOfPreviousUrl)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">An alternative is to let the app do the "finding the immediate child" logic by just emitting</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">void urlChangedToParent(const QUrl &oldUrl, const QUrl &newUrl)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It seems to me that this is a better signal, because it's more generic. On the other hand, if you are afraid that multiple apps would then need to implement the "first child" logic, then this could be done by a helper method in this class. But at least the signal has a much more generic purpose than being geared towards this specific feature,
which increases the chances that it is useful for other things later.</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;">I also thought first that something like urlChangedToParent(QUrl) would be a good signal name, but it might make people think that this signal will always be emitted when navigating from a URL to its (possibly indirect) parent.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">However, this is not the case - the new signal will only be emitted if the URL change of the KUrlNavigator was triggered by a call of setLocationUrl(const QUrl&). If the URL change was caused by invoking the goBack() or goForward() slots, then the signal will not be emitted. The reason is that the user of KUrlNavigator will try to restore the view state (using the result from KUrlNavigator's locationState() function) then, and selecting any parents would interfere with that (see the discussion in https://git.reviewboard.kde.org/r/123253/ ).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So the idea is that the new signal is only emitted if no history action was responsible for the URL change. This makes it possible for Dolphin and other users of KUrlNavigator to behave like some other file managers, which also select the parent of the previous URL, unless the URL change was caused by back/forward. So anything with 'changed' is not really accurate, because a change is not sufficient to emit this signal.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Still, it might make sense to have an 'ed' in the name. 'requested' appears often in signals which are not so much about state changes, but have the purpose to make the receiver do something. How about</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">urlSelectionRequested(const QUrl &) ?</p></pre>
<br />
<p>- Frank</p>
<br />
<p>On February 18th, 2016, 9:53 p.m. UTC, Gregor Mi 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 Dolphin, KDE Frameworks, Emmanuel Pescosta, and Frank Reininghaus.</div>
<div>By Gregor Mi.</div>
<p style="color: grey;"><i>Updated Feb. 18, 2016, 9:53 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="https://bugs.kde.org/show_bug.cgi?id=335616">335616</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kio
</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;">Moved logic from https://git.reviewboard.kde.org/r/123253 to here.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Provides a signal to implement bug https://bugs.kde.org/show_bug.cgi?id=335616: "Dolphin: Navigate to parent folder selects child folder".</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/filewidgets/kurlnavigator.h <span style="color: grey">(4ffe56acf9ef7a80ba27ba3a08327e363f98fb0d)</span></li>
<li>src/filewidgets/kurlnavigator.cpp <span style="color: grey">(64d2a6d1d5cf8ca2e0aaa61d00ac1ffb1a9866b3)</span></li>
<li>src/filewidgets/urlutil.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/CMakeLists.txt <span style="color: grey">(dc88ce9fd5c5ae6ad135b72785370c0094969b5c)</span></li>
<li>tests/urlutiltest.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127111/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>