Review Request: Adding "net usershare" suport for KSambaShare

[Aurélien Gâteau]

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


It would be simpler I think if KSambaShareInfo were value-based. It would make it easier to manipulate it without leaks. It inherits from QObject right now, which prevents making it value-based. I suggest the following changes:
- In KSambaSharePrivate::shareChange(), instead of emitting a signal, modify the relevant KSambaShareInfo instance directly
- Remove the slot
- Do not inherit from QObject
- Make KSambaShareInfo inherit from QSharedData

When this is done you can simplify KSambaShare::add() a bit by turning its signature into "void KSambaShare::add(const KSambaShareInfo& info)"

Other suggestions:
- Move KSambaSharePrivate::add() into KSambaShareInfo::save()


/trunk/KDE/kdelibs/kio/kio/ksambadata_p.h
<http://reviewboard.kde.org/r/4320/#comment6270>

    Would be nice to document the content of this "data" member



/trunk/KDE/kdelibs/kio/kio/ksambashare.h
<http://reviewboard.kde.org/r/4320/#comment6271>

    isShareNameAvailable() sounds more english



/trunk/KDE/kdelibs/kio/kio/ksambashare.h
<http://reviewboard.kde.org/r/4320/#comment6272>

    What is the reason for the change from private: to protected: here?



/trunk/KDE/kdelibs/kio/kio/ksambashare.h
<http://reviewboard.kde.org/r/4320/#comment6273>

    You can't add shareList here, it would break binary compatibility. You have to keep it in the KSambaSharePrivate object.



/trunk/KDE/kdelibs/kio/kio/ksambashare.cpp
<http://reviewboard.kde.org/r/4320/#comment6275>

    rawData should be "const QByteArray&" instead of "QByteArray&"



/trunk/KDE/kdelibs/kio/kio/ksambashare.cpp
<http://reviewboard.kde.org/r/4320/#comment6274>

    This method leaks KProcess objects. Have a look at my previous suggestions to fix that.



/trunk/KDE/kdelibs/kio/kio/ksambashare.cpp
<http://reviewboard.kde.org/r/4320/#comment6276>

    Are you sure this test works?
    
    You implemented operator==() for KSambaShareInfo, but since shareList is a list of pointers, I don't think QList::contains() is going to use KSambaShareInfo::operator==() but rather only compares the addresses of the objects.


- Aurélien


On 2010-07-11 00:59:35, Rodrigo Belem wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4320/
> -----------------------------------------------------------
> 
> (Updated 2010-07-11 00:59:35)
> 
> 
> Review request for kdelibs, Jonathan Thomas, Aurélien Gâteau, Jonathan Riddell, and Adenilson Cavalcanti.
> 
> 
> 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/kio/CMakeLists.txt 1137974 
>   /trunk/KDE/kdelibs/kio/kio/ksambadata_p.h PRE-CREATION 
>   /trunk/KDE/kdelibs/kio/kio/ksambashare.h 1137974 
>   /trunk/KDE/kdelibs/kio/kio/ksambashare.cpp 1137974 
>   /trunk/KDE/kdelibs/kio/kio/ksambashare_p.h PRE-CREATION 
>   /trunk/KDE/kdelibs/kio/kio/ksambashareinfo.h PRE-CREATION 
>   /trunk/KDE/kdelibs/kio/kio/ksambashareinfo.cpp PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/4320/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Rodrigo
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20100713/c33c2673/attachment.htm>