Review Request: Allow "~" as folder name (bug: 117473)

[Oswald Buddenhagen]

> On 2010-08-29 16:50:56, Oswald Buddenhagen wrote:
> > 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.
> 
> Oswald Buddenhagen wrote:
>     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().
> 
> Dawit Alemayehu wrote:
>     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.
> 
> Oswald Buddenhagen wrote:
>     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.
>
> 
> Mark wrote:
>     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?
> 
> Dawit Alemayehu wrote:
>     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...

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.


- Oswald


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5186/#review7266
-----------------------------------------------------------


On 2010-08-29 19:45:14, Mark wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5186/
> -----------------------------------------------------------
> 
> (Updated 2010-08-29 19:45:14)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> 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.
> 
> 
> This addresses bug 117473.
>     https://bugs.kde.org/show_bug.cgi?id=117473
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdelibs/kfile/knewfilemenu.cpp 1169591 
> 
> Diff: http://svn.reviewboard.kde.org/r/5186/diff
> 
> 
> Testing
> -------
> 
> Tested the patch and seems to work fine.
> 
> 
> Thanks,
> 
> Mark
> 
>

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