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

[Dawit Alemayehu]

> 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?

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...


- Dawit


-----------------------------------------------------------
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/3ad7e430/attachment.htm>