<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://svn.reviewboard.kde.org/r/5186/">http://svn.reviewboard.kde.org/r/5186/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 29th, 2010, 4:50 p.m., <b>Oswald Buddenhagen</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;">this patch is fundamentally flawed - its function is to simply break the logic for home dir expansion entirely.
the correct approach is relying on tildeExpand()'s documented escaping behavior. i think it would make most sense to first run the string through tildeExpand unconditionally and only then check whether the path is absolute. and document the escaping in some user-visible places.</pre>
</blockquote>
<p>On August 29th, 2010, 7:34 p.m., <b>Oswald Buddenhagen</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;">addendum: my idea won't work, as kurl::setPath() calls tildeExpand() itself. while that is a nice convenience feature, such functions at such a low level pretty much always backfire, like in this case ...
to fix this, we need to add an enum parameter to setPath() which would tell it not to call tildeExpand().</pre>
</blockquote>
<p>On September 8th, 2010, 4:54 p.m., <b>Dawit Alemayehu</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;">Actually to me this seems to be caused by the logic used in KShell::tildeExpand. It is correct that KUrl::setPath has no business silently expanding the tilde character ; specially since the use of it in the path component of a url is prefectly valid according to RFC 2396. However, KShell::tildeExpand returns an empty string when you pass it "~blah", where "blah" is not a valid user, instead of returning "~blah" back. Unless that is intentional, it is the source of this bug.</pre>
</blockquote>
<p>On September 8th, 2010, 5:52 p.m., <b>Oswald Buddenhagen</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;">this is not the source of the bug, but it just exposes it.
the tildeExpand() behavior is kind of intentional - the bash-compliant behavior of not touching unresolvable users introduces an ambiguity, however, it is obviously more convenient.
</pre>
</blockquote>
<p>On September 8th, 2010, 6:26 p.m., <b>Mark</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, what can i do with this? I really have no idea which code part is the one that is bugged here besides the proposed patches...
Perhaps you guys with more in depth knowledge about this subject can figure out how it should work and patch accordingly?</pre>
</blockquote>
<p>On September 8th, 2010, 8:22 p.m., <b>Dawit Alemayehu</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 what I suggested in my review below and it should fix the bug... That is replace the original if statement that checks for for '/' or '~'
"if ((name[0] == '/') || (name[0] == '~')) {"
with
if (name[0] == '~') {
const QString expandedName = KShell::tildeExpand(name);
// empty string means, cannot expand...
if (!expandedName.isEmpty())
name = expandedName;
}
if (name[0] == '/')
url.setPath(name);
instead of what you did in your patch...</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;">this won't work too well:
- if the path starts with a tilde and is actually a home dir reference, it will be expanded to an absolute path, and thus the tildeExpand() in kurl::setpath() won't do anything with it any more. everything is just fine.
- if it starts with no tilde or cannot be expanded, it will not be expanded and i have no idea what the later kurl::addpath() do with it, expecially if it starts with an escaped tilde.
fwiw, kurl::addpath() is broken in that it uses setpath(), which in turn will call tildeExpand(), so the path is unescaped with each addpath() call.</pre>
<br />
<p>- Oswald</p>
<br />
<p>On August 29th, 2010, 7:45 p.m., Mark wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://svn.reviewboard.kde.orgrb/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 kdelibs.</div>
<div>By Mark.</div>
<p style="color: grey;"><i>Updated 2010-08-29 19:45:14</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;">This patch fixes https://bugs.kde.org/show_bug.cgi?id=117473 and allows "~" to be the folder name.
this is also consistent with new file names that could already have the "~" name.</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;">Tested the patch and seems to work fine.</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="https://bugs.kde.org/show_bug.cgi?id=117473">117473</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>trunk/KDE/kdelibs/kfile/knewfilemenu.cpp <span style="color: grey">(1169591)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/5186/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>