<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/108196/">http://git.reviewboard.kde.org/r/108196/</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 5th, 2013, 11:11 a.m., <b>Andreas Pakulat</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="http://git.reviewboard.kde.org/r/108196/diff/1/?file=104951#file104951line92" style="color: black; font-weight: bold; text-decoration: underline;">project/path.h</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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">public:</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">92</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">explicit</span> <span class="n">Path</span><span class="p">(</span><span class="k">const</span> <span class="n">QString</span><span class="o">&</span> <span class="n">pathOrUrl</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;">Is the string-conversion really needed? I would've hoped that most of the time we're dealing with KUrl's already.</pre>
</blockquote>
<p>On January 5th, 2013, 3:04 p.m., <b>Milian Wolff</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;">Not if you operate on local files, e.g. Qt uses QString for local paths everywhere. Same for QFileInfo or IndexedString.</pre>
</blockquote>
<p>On January 5th, 2013, 4:07 p.m., <b>Andreas Pakulat</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 guess the latter (IndexedString) is a bit of a problematic case, but for the others I'd say that whoever uses Path needs to do the conversion when it needs to access local files.
If it turns out that we end up with tons of this:
... (absfilename is a QString produced here somehow)
Path p(KUrl::fromPath(absfilename))
Then we can have a convenience constructor, but if thats not needed that often or a KUrl is being used in that place to get a local path for some Qt API and then later on a Path is constructed from the same thing, then I'd leave this constructor out.</pre>
</blockquote>
<p>On January 6th, 2013, 4:53 p.m., <b>Milian Wolff</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;">Do you want to remove it to make clear that one should use the KUrl ctor if one already has a KUrl? I don't see what you gain by removing this constructor.
Also note that the case you mention (i.e. KUrl::fromPath) should get its own method in Path imo, since in such cases we do not want to go through the costly KUrl and instead split the incoming QString path directly.</pre>
</blockquote>
<p>On January 7th, 2013, 3 a.m., <b>Andreas Pakulat</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;">Unless I'm mistaken the Path constructor with QString is doing exactly the same as a fromPath() static function would do. And I'm suggesting to get rid of it to avoid people using it if they don't need it. This is purely to keep the API as small and simple as possible, don't add functions/constructors that nobody (or less than a handfull places) uses right now. In particular with generic types like strings in constructors since you need to do parsing on that and if you don't offer the constructor you don't need to do any parsing.
BTW: the reason conversion of string to url is so expensive is because thats not trivial, so I don't see how you can avoid the cost since you'd still need to at least verify the incoming string is a simple local filepath and not some remote url before splitting it up on slashes.</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;">No, the Path(QString) ctor is the same as Path(KUrl(QString)) which in turn is the same as Path(KUrl::fromPathOrUrl(QString)). This is being used at quite a few places in our code, but its mostly unit tests which pass either URLs or local paths.
I'll investigate what will break if we'll remove that ctor and look at the required porting effort. Thinking about it some more I also agree that it should be "fast enough" to use KUrl::fromPath where appropriate to speed up the process versus rolling our own path parsing. Especially because I forgot that we want clean paths thus would have to implement that on our own as well.
Thanks for nagging Andreas :)
</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 5th, 2013, 11:11 a.m., <b>Andreas Pakulat</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="http://git.reviewboard.kde.org/r/108196/diff/1/?file=104951#file104951line311" style="color: black; font-weight: bold; text-decoration: underline;">project/path.h</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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">public:</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">311</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QVector</span><span class="o"><</span><span class="n">QString</span><span class="o">></span> <span class="n">m_data</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;">Hmm, I think I'd separate the remote-url part from the path itself here. There are some places in the code where you have explicit checks for remote-urls and then take the first or second item from the list. Having a separate member would make that code a lot clearer.</pre>
</blockquote>
<p>On January 5th, 2013, 3:04 p.m., <b>Milian Wolff</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 also thought about that but deliberately decided against it for space reasons (saving one pointer for local paths). But I agree that it might be a bit over-optimized. I'll revisit the code and take another look at it.</pre>
</blockquote>
<p>On January 5th, 2013, 4:07 p.m., <b>Andreas Pakulat</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 see, but in this case I'd say optimization comes after code-readability. So once everythings ported we should measure the difference between saving the pointer or not and then decide wether saving it outweighs the special code in the various functions - at least IMO.</pre>
</blockquote>
<p>On January 6th, 2013, 10:21 a.m., <b>Milian Wolff</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 did compare it back then but sadly I just realize that my results in the commits are not comparable: 949ed44e369d8d3ce798737f1a3517bc9fe48caa vs c6dca99c710f99c7ddbc47b00dd70157f7d3a076 also shows different numbers for the other datatypes... I'll redo the measurement and figure out whether it's worth it.</pre>
</blockquote>
<p>On January 6th, 2013, 5:46 p.m., <b>Milian Wolff</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;">Looking at it again, I have to say that the code readability would not increase considerably imo... Especially since you'd now have to compare/check two variables instead of just one the code becomes bigger which is arguably also worse to read.
And so far only 6 places or so need to take special care when dealing with the aggregated prefix-in-m_data. Imo this is not so bad and I'd like to keep it as-is.</pre>
</blockquote>
<p>On January 7th, 2013, 2:53 a.m., <b>Andreas Pakulat</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;">So you think that this (from setFilename):
if( m_data.isEmpty() ) {
m_data.append(name);
} else {
m_data.last(name);
}
is not easier to understand than the current code:
if (m_data.isEmpty() || (!isLocalFile() && m_data.size() == 1)) {
// append the name to empty Paths or remote Paths only containing the Path prefix
m_data.append(name);
} else {
// overwrite the last data member
m_data.last() = name;
}
I wholeheartedly disagree. Or this one (from up/parent):
if( m_data.size() == 1 ) {
ret.m_data.last().clear();
} else {
ret.m_data.pop_back();
}
is certainly easier to understand than this one (and both are actually shorter as well):
if (m_data.size() == (1 + (isRemote() ? 1 : 0))) {
// keep the root item, but clear it, otherwise we'd make the path invalid
// or a URL a local path
ret.m_data.last().clear();
} else {
ret.m_data.pop_back();
}
The reason is simply that its more self-explanatory to someone who hasn't written up the code. In the current code one first has to think through why with isRemote() it compares to 2 before clearing the last element, so you always have to keep that special behaviour in the back of your head when reading or debugging the code. Can you show the example where you actually get more code for the separation of url and path (yeah I'm too lazy to look through the complete implementation)?</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;">Hehe these will of course become simpler. I don't debate that. When I started introducing the second member yesterday, I did dislike it in a few places. I'll just do it again and show you the diff.</pre>
<br />
<p>- Milian</p>
<br />
<p>On January 5th, 2013, 10:59 a.m., Andreas Pakulat 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 KDevelop.</div>
<div>By Andreas Pakulat.</div>
<p style="color: grey;"><i>Updated Jan. 5, 2013, 10:59 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;">Discuss API of the new Path class</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>project/path.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>project/path.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/108196/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>