----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/4320/#review9386 -----------------------------------------------------------
Ship it! Looks good, this can be committed to trunk. I just have a final set of minor things that would improve the code, especially for performance. /trunk/KDE/kdelibs/kio/kio/ksambashare.h <http://svn.reviewboard.kde.org/r/4320/#comment10394> addittion ->addition /trunk/KDE/kdelibs/kio/kio/ksambashare.h <http://svn.reviewboard.kde.org/r/4320/#comment10395> diretories -> directories /trunk/KDE/kdelibs/kio/kio/ksambashare.cpp <http://svn.reviewboard.kde.org/r/4320/#comment10396> Iterating over values() is slow, because it means interating, building a temporary qlist, and iterating again. Better use a QMap const_iterator. /trunk/KDE/kdelibs/kio/kio/ksambashare.cpp <http://svn.reviewboard.kde.org/r/4320/#comment10397> Same here, use a QMap const_iterator. /trunk/KDE/kdelibs/kio/kio/ksambashare.cpp <http://svn.reviewboard.kde.org/r/4320/#comment10398> Interesting, what happens if you create a share named "toto", and later on create a user named "toto"? :-) Just curious. /trunk/KDE/kdelibs/kio/kio/ksambashare.cpp <http://svn.reviewboard.kde.org/r/4320/#comment10399> I suggest a java-style QMapMutableIterator here, so that you can just it.remove() to avoid iterating (inside take()) inside an iteration (over keys()). /trunk/KDE/kdelibs/kio/kio/ksambashare.cpp <http://svn.reviewboard.kde.org/r/4320/#comment10400> Wouldn't it be faster to do something like getSharesByPath().isEmpty(), to avoid building the complete list of sharedDirectories? Or there could even be a faster d->isDirectoryShared() that just stops when it finds it. Minor issue, though. - David On 2010-12-04 15:28:00, Rodrigo Belem wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://svn.reviewboard.kde.org/r/4320/ > ----------------------------------------------------------- > > (Updated 2010-12-04 15:28:00) > > > 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 > >
