----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110988/#review34280 -----------------------------------------------------------
Good work, not an easy task. In addition to the issues I found, could you ensure that the unittests still pass? In particular jobtest in kio, looks appropriate. Plus the one that calls chmod(). kioslave/file/file.cpp <http://git.reviewboard.kde.org/r/110988/#comment25153> You cannot cast from mode_t to QFileDevice::Permissions. They are different enums with different values. SlaveBase::chmod() has to be ported to QFileDevice::Permissions - and all kioslaves, or this will have to stay being a mode_t. One issue with QFileDevice::Permissions is that it doesn't have support for SUID and SGID, but only root can set that anyway, so one could say it's out of scope for KDE - or at best, can be handled locally in kpropertiesdialog.cpp. So I'm OK with a port to QFileDevice::Permissions -- but the signature of chmod() should change to avoid any ambiguity between mode_t and Permissions, and obviously the calling code in libkio has to be adjusted too. All this should probably be a different patch, so I'd leave this one with mode_t and ::chmod() for now. kioslave/file/file.cpp <http://git.reviewboard.kde.org/r/110988/#comment25155> QDir has a bit of a strange API when using it only with absolute paths. Since you're calling mkdir(absoulte), the QDir instance can be default-constructed, so if ( !QDir().mkdir(path)) kioslave/file/file.cpp <http://git.reviewboard.kde.org/r/110988/#comment25156> More precisely, QDir should be able to tell us if mkdir failed due to access denied, or disk full, or another reason. (=> adjust the comment) kioslave/file/file.cpp <http://git.reviewboard.kde.org/r/110988/#comment25157> Bug in the original code? Should be ..._FOR_WRITING if mode & QIODevice::WriteOnly? - David Faure On June 13, 2013, 10:40 a.m., Martin Klapetek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/110988/ > ----------------------------------------------------------- > > (Updated June 13, 2013, 10:40 a.m.) > > > Review request for KDE Frameworks. > > > Description > ------- > > Ports away kioslave/file from unix/kde specific calls to QFile calls > > > Diffs > ----- > > kioslave/file/file.h 93aeef4 > kioslave/file/file.cpp d57517b > kioslave/file/file_unix.cpp 8b92e06 > > Diff: http://git.reviewboard.kde.org/r/110988/diff/ > > > Testing > ------- > > > Thanks, > > Martin Klapetek > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
