Review Request 118604: Fix wrong escaping in kfilewidget when selecting multiple files

[Mark Gaiser]

> On jun 14, 2014, 8:54 a.m., David Faure wrote:
> > What if the file isn't local?
> > 
> > Sounds to me like the bug is elsewhere.
> > 
> > Of course for local files, showing a local path looks better than a file:/// URL, so this could be improved, but in a way that doesn't break remote files.
> > 
> > The use of KUrl::relativeUrl and putting that into a url looks quite suspicious to me, actually.
> > KUrl didn't work well with relative urls.
> > 
> > Maybe locationEditCurrentTextList should be changed into a qstringlist rather than a url list...
> > 
> > It would also be interesting to check if this works in the KIO framework (Qt5), where I ported it to QUrl...
> 
> Mark Gaiser wrote:
>     Just tried it out on KF5. The issue as described on 335922 is partly present.
>     
>     Input files:
>     f#1.png
>     f%1.png
>     f#2.png
>     f%2.png
>     
>     Command used:
>     kdialog --multiple --getopenfilename ~ "*.png|PNG images"
>     
>     kdialog output when selecting those four files:
>     /home/kde-devel/335922/f#1.png /home/kde-devel/335922/f#2.png /home/kde-devel/335922/f%251.png /home/kde-devel/335922/f%252.png
>     
>     The files with a "#" in them seem to be parsed just fine in KF5. Those with a percent sign still get replaced by "%25".
>
> 
> Mark Gaiser wrote:
>     Note: Something is fuzzy in that slotOk. When i add one debug line to see the output of locationEditCurrentTextList i get two results (as if slotOk is called twice). The output i get (in a readable way):
>     -- First "Orig QUrl list:"
>     - QUrl("file:///home/kde-devel/335922/f%231.png")
>     - QUrl( "file:///home/kde-devel/335922/f%232.png" )
>     - QUrl( "file:///home/kde-devel/335922/f%251.png" )
>     - QUrl( "file:///home/kde-devel/335922/f%252.png" )
>     
>     -- Second "Orig QUrl list:"
>     - QUrl("file:///home/kde-devel/335922/f%231.png")
>     - QUrl( "file:///home/kde-devel/335922/f%232.png" )
>     - QUrl( "file:///home/kde-devel/335922/f%25251.png" ) <-- What happened here?
>     - QUrl( "file:///home/kde-devel/335922/f%25252.png" ) <-- What happened here?
> 
> Mark Gaiser wrote:
>     "when i add one debug line" : http://p.sc2.nl/pn8jjrqmx :) Sorry for the extra noise.
> 
> Mark Gaiser wrote:
>     More noise :)
>     I played with the code a bit and have it working. My changes are only in slotOk so that means the actual issue is in there as well. My "patch" smells somewhat hackish and i think David will not like this patch at all, hehe.
>     
>     Here it is: http://p.sc2.nl/ppfe83z6a
>     
>     The important change is:
>     - I got rid of the slotOk recursion in this code path (it's still in another else if).
>     - Because i don't enter the function for a second time, i had to make sure that locationEditCurrentTextList wasn't modified since that list would be needed later on. To do that i removed the "now recalculate all paths for them being relative in base of the top most url" loop and do that in the QStringList construction which was just a few lines down. This seems to work just fine and looks a little cleaner as well imho.
>     
>     I don't know exactly how this code is supposed to work so i hope one of the authors of it (quite a few!) could have a look at what's really going wrong here.
> 
> David Faure wrote:
>     By now the paste expired. Please use permanent URLs or reviewboard.
> 
> Mark Gaiser wrote:
>     I had no clue that this rr was still alive. I'll see if i can figure out what i pasted (half a year ago). It might take a few days..

Damn sorry! I don't have the patch anymore. Not in my git nor in the website archive. The thing i vaguely remember is slotOk being called from within slotOk (small recursion). That then caused the parsed url's to be re-parsed which in turn ends up with the wrong results. Or that's what i recall along with the stuff i wrote above.


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118604/#review60070
-----------------------------------------------------------


On jun 7, 2014, 4:43 p.m., Victor Dodon wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118604/
> -----------------------------------------------------------
> 
> (Updated jun 7, 2014, 4:43 p.m.)
> 
> 
> Review request for kdelibs, Albert Vaca Cintora and Aaron J. Seigo.
> 
> 
> Bugs: 335922
>     http://bugs.kde.org/show_bug.cgi?id=335922
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> Only when selecting multiple files in KFileDialog, if a file contains in the name the characters '#' or '%', it is (wrongly) escaped.
> 
> The only modification needed is in kfilewidget.cpp, in slotOk, instead of appending to the stringList the pretty url, append the url.toLocalFile, to prevent the double escaping.
> 
> 
> Diffs
> -----
> 
>   kfile/kfilewidget.cpp fc9b169 
> 
> Diff: https://git.reviewboard.kde.org/r/118604/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Victor Dodon
> 
>

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