----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/4320/#review9079 -----------------------------------------------------------
dfaure is still the best person to comment on the KDirWatch bits. As for the rest, it looks mostly fine, with these small pieces needing some work. /trunk/KDE/kdelibs/kio/kio/ksambashare.h <http://svn.reviewboard.kde.org/r/4320/#comment9874> Several @return statements here are followed by a @c, even when the following word does not need to use a typewriter font. /trunk/KDE/kdelibs/kio/kio/ksambashare.h <http://svn.reviewboard.kde.org/r/4320/#comment9875> Test -> Tests, for consistency. Missing '.' at the end of the sentence. /trunk/KDE/kdelibs/kio/kio/ksambashare.h <http://svn.reviewboard.kde.org/r/4320/#comment9876> If you want to keep the @c's here, false needs one too. /trunk/KDE/kdelibs/kio/kio/ksambashare.h <http://svn.reviewboard.kde.org/r/4320/#comment9877> an empty string /trunk/KDE/kdelibs/kio/kio/ksambashare.h <http://svn.reviewboard.kde.org/r/4320/#comment9884> If this is deprecated, you can add KDE_DEPRECATED here. /trunk/KDE/kdelibs/kio/kio/ksambashare.cpp <http://svn.reviewboard.kde.org/r/4320/#comment9878> Not sure if this apidox bit will actually be parsed and shown in api.kde.org /trunk/KDE/kdelibs/kio/kio/ksambashare.cpp <http://svn.reviewboard.kde.org/r/4320/#comment9879> This variable can be removed: for (...) { if (condition) { foo(); return true; } } ... return false; /trunk/KDE/kdelibs/kio/kio/ksambashare.cpp <http://svn.reviewboard.kde.org/r/4320/#comment9880> if (...) { return QString::fromLocal8Bit(...); } return QString(); and then we can get rid of result. /trunk/KDE/kdelibs/kio/kio/ksambashare.cpp <http://svn.reviewboard.kde.org/r/4320/#comment9881> const QFileInfo pathInfo(path); /trunk/KDE/kdelibs/kio/kio/ksambasharedata.h <http://svn.reviewboard.kde.org/r/4320/#comment9885> A brief apidox description + @since 4.6 would be good to have here. /trunk/KDE/kdelibs/kio/kio/ksambasharedata.h <http://svn.reviewboard.kde.org/r/4320/#comment9882> Nitpick: use all verbs in the 3rd person? /trunk/KDE/kdelibs/kio/kio/ksambasharedata.cpp <http://svn.reviewboard.kde.org/r/4320/#comment9883> I _think_ we don't. - Raphael On 2010-12-01 01:56:04, Rodrigo Belem wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://svn.reviewboard.kde.org/r/4320/ > ----------------------------------------------------------- > > (Updated 2010-12-01 01:56:04) > > > Review request for kdelibs, Raphael Kubo da Costa, Jonathan Thomas, Aurélien > Gâteau, Jonathan Riddell, Adenilson Cavalcanti, loureiro, and Daniel > Nicoletti. > > > Summary > ------- > > KDE needs to support modern samba tools. With the "net usershare" command > line tool the users can manage their shares. The attached patch intends to > add support for this tool. > > > Diffs > ----- > > /trunk/KDE/kdelibs/includes/CMakeLists.txt 1202449 > /trunk/KDE/kdelibs/includes/KSambaShareData PRE-CREATION > /trunk/KDE/kdelibs/kio/CMakeLists.txt 1202449 > /trunk/KDE/kdelibs/kio/kio/ksambashare.h 1202449 > /trunk/KDE/kdelibs/kio/kio/ksambashare.cpp 1202449 > /trunk/KDE/kdelibs/kio/kio/ksambashare_p.h PRE-CREATION > /trunk/KDE/kdelibs/kio/kio/ksambasharedata.h PRE-CREATION > /trunk/KDE/kdelibs/kio/kio/ksambasharedata.cpp PRE-CREATION > /trunk/KDE/kdelibs/kio/kio/ksambasharedata_p.h PRE-CREATION > > Diff: http://svn.reviewboard.kde.org/r/4320/diff > > > Testing > ------- > > > Thanks, > > Rodrigo > >