<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/106475/">http://git.reviewboard.kde.org/r/106475/</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 4th, 2012, 4:28 p.m. UTC, <b>Christoph Feck</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;">Causes a regression. When using "Save Image As..." in Konqueror, the file dialog starts at "Custom Path" (and lists root folder), without remembering the previously used path.</pre>
</blockquote>
<p>On October 9th, 2012, 8:13 a.m. UTC, <b>Jonathan Marten</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;">Doing the test as follows appears to work, for both local files and smb:
if (!startDir.directory().isEmpty() ||
(!startDir.scheme().isEmpty() && !startDir.isLocalFile()))
</pre>
</blockquote>
<p>On October 26th, 2012, 1:02 a.m. UTC, <b>Christoph Feck</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;">If this is the correct fix, can you please commit it?</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;">Sorry for the non working suggestion, then. But I'm still confused about this code, it doesn't make sense to me.
The current code says:
(1) Has directory -> use full url as start dir
(2) No scheme, or local file -> set as fileName, use default dir
(3) Otherwise (other protocols, but with no directory) -> use full url as start dir
So, currently:
"ftp://foo/bar/", "file:///tmp" -> full url (rule 1), OK
"foo.jpg" -> fileName (rule 2), OK [this is the "save image as" case]
"smb://", "smb:" -> full url (rule 3), OK [this is the bugfix that led to this review request initially]
"foo/bar" -> has directory, so this is used as start dir URL, i.e. the method returns a relative URL. Surprising, I wonder if it works.
The "or local file" part of rule 2 seems pointless, a file URL always has a directory ("file:" doesn't exist, and KUrl("fileName").isLocalFile() is *false* nowadays). So a file URL will always use rule 1 anyway.
My suggested patch was "if relative, set as fileName". Needs adjustment for the corner case "foo/bar", but other than that, it should work.
Proof:
QVERIFY(KUrl("smb://").directory().isEmpty());
QVERIFY(KUrl("blah").directory().isEmpty());
QVERIFY(!KUrl("foo/bar").directory().isEmpty());
QVERIFY(!KUrl("blah").isLocalFile());
Ah, the konqueror breakage comes from this code (kwebpage.cpp and khtml_ext.cpp) :
118│ const QString fileName = suggestedName.isEmpty() ? srcUrl.fileName() : suggestedName;
119│ // convert filename to URL using fromPath to avoid trouble with ':' in filenames (#184202)
120├> KUrl destUrl = KFileDialog::getSaveFileName(KUrl::fromPath(fileName), QString(), parent);
This turns "foo.jpg" into "file:foo.jpg", i.e. "file://foo.jpg". Ouch.
More from me later...</pre>
<br />
<p>- David</p>
<br />
<p>On September 17th, 2012, 10:39 a.m. UTC, Àlex Fiestas wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://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 kdelibs and David Faure.</div>
<div>By Àlex Fiestas.</div>
<p style="color: grey;"><i>Updated Sept. 17, 2012, 10:39 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;">This patch addresses the following bug:
When an user clicks in a KUrlRequestUrl("smb://") to open the dialog, the KFIleDialog will be opened with the "current working dir" instead of smb://
Debugging the issue, I reached KFileWidget::getStartUrl, in that method there is a check that looks if the directory is not empty, in case it is the directory (startDir) is discarded in favor of some magic code that looks for recent folders blablabla.
this code apparently was added to support the case where an url is composed only by a fileName, for example when Konqueror Save as (commit 4d3933d4). So the solution I thought is to check if the schema is not empty, in case both directory and schema are empty the KUrl will be discarded as well, instead if any of them is not empty the directory will be used.</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;">Used Konqueror (as indicated in the commit) with both khtml and webkit.
Tested samba-mounter
Tested bluedevil
Everything seems to work.</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>kfile/kfilewidget.cpp <span style="color: grey">(7069a49)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106475/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>