Review Request: fix path for storing viewpropertysettings on Windows

[Peter Penz]

> On March 18, 2012, 10:50 a.m., Peter Penz wrote:
> > Thanks for the patch! As you say it is "not a beauty" but as it is a one-liner encapsulated within a define I'd say it is ok.
> > 
> > > Theoretically using KUrl::path() would fix that, but that doesn't work for the rest of the code.
> > 
> > You mean this does not work under Windows (I cannot test it)?
> > #ifdef Q_OS_WIN
> >     m_filePath = destinationDir("local") + url.path();
> > #else 
> >     m_filePath = destinationDir("local") + m_filePath;
> > #endif
> > 
> > I would slightly prefer the code above if it works, but please feel free to commit your patch 1:1 if I missed something in my suggestion.
> >
> 
> Patrick Spendrin wrote:
>     Well, because of the assumption that url.path() actually delivers a valid path, we patched KUrl::path() to return QUrl::toLocalFile() and not QUrl::path(). I don't want to change that because to many applications rely on KUrl::path() returning a useful path and not only the path of a url... :-/
>     There would still be a problem in your above solution: that QUrl::path() would also return a path containing a colon (which would lead to an invalid m_filePath again, or using the remove(QLatin1Char(':')) ).

Thanks for the explanation! Then I'd say please commit your patch: It's one line not winning the beauty-contest but it won't hurt the maintainability of viewproperties.cpp ;-)


- Peter


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


On March 17, 2012, 5:51 p.m., Patrick Spendrin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104313/
> -----------------------------------------------------------
> 
> (Updated March 17, 2012, 5:51 p.m.)
> 
> 
> Review request for Dolphin and Peter Penz.
> 
> 
> Description
> -------
> 
> On Windows you cannot simply concatenate two paths because the : character is forbidden except after the drive letter. The second issue I am addressing with this patch is that you use the local file name e.g. C:/kde/ which doesn't start with a / so the resulting path is something like this C:/Users/Patrick/.kde/.../localC:/kde. Theoretically using KUrl::path() would fix that, but that doesn't work for the rest of the code.
> 
> I know that the patch is a bit ugly, if you can think of a better solution, please just let me know.
> 
> 
> This addresses bug 287360.
>     http://bugs.kde.org/show_bug.cgi?id=287360
> 
> 
> Diffs
> -----
> 
>   dolphin/src/views/viewproperties.cpp 36ec6bd 
> 
> Diff: http://git.reviewboard.kde.org/r/104313/diff/
> 
> 
> Testing
> -------
> 
> Windows only, the bug 287360 is fixed.
> 
> 
> Thanks,
> 
> Patrick Spendrin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20120318/3e26ead5/attachment.htm>