Review Request: Changed KUrl::toLocalFile to use QUrl::path() instead QUrl::toLocalFile() on non-Windows platforms

[Aaron J. Seigo]

> On July 10, 2011, 1:20 p.m., Ingo Klöcker wrote:
> > This changes the behavior of toLocalFile() for non-local URLs. Instead of an empty string your version returns the (remote) path. I'm not sure how relevant this is because it makes little sense to call toLocalFile() on non-local URLs. OTOH, some developers might check the return value to toLocalFile() instead of using isLocalFile().
> > 
> > I suggest getting your fix into Qt instead of adding a workaround to kdelibs.
> 
> Dawit Alemayehu wrote:
>     That is true, but the patch can be easily change to accomodate that case as well. I have opened a ticket upstream, but just in case it is not fixed there at least for Qt 4.8 I have also updated the patch for KUrl not to break previous behavior for non-local URLs.

when the bug is fixed in Qt, then we will quite likely end up with unnecessary code in kdelibs. the patch doesn't document the reason for the code, either, increasing the risk.

at a minimum, i'd suggest there should be a comment in the file with a link to the upstream tracker issue so it is documented as to *why* that code is written like that (e.g. a bug in Qt). a #warning at compile time noting that the code should be removed when ticket # is fixed might also be nice to avoid it getting lost over time.


- Aaron J.


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101906/#review4572
-----------------------------------------------------------


On July 10, 2011, 3:34 p.m., Dawit Alemayehu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101906/
> -----------------------------------------------------------
> 
> (Updated July 10, 2011, 3:34 p.m.)
> 
> 
> Review request for kdelibs and David Faure.
> 
> 
> Summary
> -------
> 
> The attached patch changes KUrl::toLocalFile to use QUrl::path() when extracting the path from the current URL on non-Windows platforms so that KUrl would return the correct path when KUrl::toLocalFile is called on an absolute path url whose top level contains a ':', e.g. "file:///A:/".
> 
> 
> This addresses bug 194746.
>     http://bugs.kde.org/show_bug.cgi?id=194746
> 
> 
> Diffs
> -----
> 
>   kdecore/io/kurl.cpp acfc9a1 
> 
> Diff: http://git.reviewboard.kde.org/r/101906/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dawit
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20110711/b001247f/attachment.htm>