Review Request 111911: Port kioslave/ftp/ftp.cpp away from kde_file.h

[David Faure]

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


A global KDE::open: why not, might make windows porting easier.
But for stat/QT_STAT, the best solution would be to port to QFileInfo.


kioslave/ftp/ftp.cpp
<http://git.reviewboard.kde.org/r/111911/#comment27593>

    This isn't unix-only code, it should be cross platform. So please use:
    
    QFileInfo info(...);
    bSrcExists = info.exists(); 
    S_ISDIR becomes info.isDir();



kioslave/ftp/ftp.cpp
<http://git.reviewboard.kde.org/r/111911/#comment27595>

    This seems more tricky to make portable. I'm OK if you leave it as an exercise to the (Windows) reader.



kioslave/ftp/ftp.cpp
<http://git.reviewboard.kde.org/r/111911/#comment27596>

    Funny, the author didn't know that KDE::rename() would overwrite (on unix).
    Well, this makes the porting easy.
    
    (The trap is that porting from KDE::rename to QFile::rename changes the behavior when the destination already exists; but this particular code is ready for that)



kioslave/ftp/ftp.cpp
<http://git.reviewboard.kde.org/r/111911/#comment27597>

    Right, we're missing a portable utime (`touch`). This will do for now.
    
    Actually, looking at sqlite3, it simply says "use utime if available, otherwise use utimes". Seems the porting to Windows will be easy then.


- David Faure


On Aug. 6, 2013, 4:11 p.m., Vishesh Handa wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111911/
> -----------------------------------------------------------
> 
> (Updated Aug. 6, 2013, 4:11 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Description
> -------
> 
> A couple of issues -
> 
> 1. KDE::open/stat/etc take a QString and convert it to a char* via QFile::encodeName(str).constData(). Qt obviously does not have methods to do so. Instead of me doing it manually for each call to QT_OPEN/QT_STAT/etc, would be it okay for me to declare local functions called KDE::stat/open?
> 
> Something along the lines of -
> namespace KDE {
> int open(const QString& filePath, ...) {
>     return QT_OPEN(QFile::encodeName(filePath).constData()), ...);
> }
> }
> 
> 2. The kioslave uses KDE::utime to set the utime of file. I've used ::utime, but that obviously won't work on non-unix platforms. What is the correct solution? 
> 
> One option is to add utime in qplatformdefs.h, but that is non trivial since Qt seems to support about 104 different qplatformdefs and therefore all of them will have to be updated.
> 
> 
> Diffs
> -----
> 
>   kioslave/ftp/ftp.cpp a0da54b 
> 
> Diff: http://git.reviewboard.kde.org/r/111911/diff/
> 
> 
> Testing
> -------
> 
> Doesn't even compile right now. With (1) it will start to compile.
> 
> 
> Thanks,
> 
> Vishesh Handa
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20130807/e51252e4/attachment.html>