----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112982/#review42150 -----------------------------------------------------------
This would have been easier to review if the coding style changes had been a different commit (not uploaded to reviewboard), to get only the actual changes here... kioslave/smb/kio_smb_dir.cpp <http://git.reviewboard.kde.org/r/112982/#comment30703> this TODO can be removed now, right? kioslave/smb/kio_smb_dir.cpp <http://git.reviewboard.kde.org/r/112982/#comment30704> The space before '(' is strange (although this file is pretty inconsistent in the first place...) kioslave/smb/kio_smb_dir.cpp <http://git.reviewboard.kde.org/r/112982/#comment30705> 0|S_IWUSR is funny :) kioslave/smb/kio_smb_dir.cpp <http://git.reviewboard.kde.org/r/112982/#comment30706> You could move this before the if(), and use it like if (finfo.exists()), it would be faster than the separate call to QFile::exists, since you'd be reusing the QFileInfo, which can cache the stat() result. kioslave/smb/kio_smb_dir.cpp <http://git.reviewboard.kde.org/r/112982/#comment30707> I'm pretty sure this cast is wrong. QFile::Permissions doesn't map to mode_t, at least not with a simple cast. Either do like kio_ftp does (KDE::open() instead of QFile), or add a call to chmod() at the end of the operation, or use a function to convert from mode_t to QFile::Permissions (maybe we want to have that in KFileItem, in fact). I prefer solution 2 or 3 above solution 1, so that QFile can be used as much as possible. kioslave/smb/kio_smb_dir.cpp <http://git.reviewboard.kde.org/r/112982/#comment30708> There's also the case where n (as returned) is smaller than n (as passed to write). So better use a different variable, and error if ret < n. This can happen when the partition is full. - David Faure On Oct. 5, 2013, 3:07 p.m., Dawit Alemayehu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/112982/ > ----------------------------------------------------------- > > (Updated Oct. 5, 2013, 3:07 p.m.) > > > Review request for KDE Runtime. > > > Bugs: 176271 and 291835 > http://bugs.kde.org/show_bug.cgi?id=176271 > http://bugs.kde.org/show_bug.cgi?id=291835 > > > Repository: kde-runtime > > > Description > ------- > > The attach patch adds support for the following to kio_smb: > > - copyToFile optimization so downloading files from window shares is faster. > - partial download resumption as part of the copyToFile implementation. > - preservation of modified file timstamp. Again as part of the copyToFile > implementation. > > Note that in this patch the latter two features only apply to "smb" -> "file" > downloads. The second part of this patch will that will follow soon will add > support for the other half, the "copyFromFile" optimization. > > > Diffs > ----- > > kioslave/smb/kio_smb.h 55efb44 > kioslave/smb/kio_smb_dir.cpp 5573266 > kioslave/smb/smb.protocol 654bcfb > > Diff: http://git.reviewboard.kde.org/r/112982/diff/ > > > Testing > ------- > > > Thanks, > > Dawit Alemayehu > >
