Review Request: Adding "net usershare" suport for KSambaShare

[David Faure]

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



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

    Missing "@since 4.6" in the documentation (for all new methods)



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

    Can you explain the difference with sharedDirectories()? 
    It only contains the name of the subdir in /var/lib/samba/usershares? Is this useful, given that one has to know about this base path to be able to use the share in any way?
    
    I would be very careful with exposing more implementation details (just like smbConfPath is a problem now that you're not using smb.conf, exposing subdir names is specific to the 'net usershare' solution). Better have an API that does not depend on the implementation - like sharedDirectories() for instance, which works fine in both cases.
    
    Or am I missing something here?



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

    Still no docu...



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

    You can't rename a signal, that's incompatible!!
    Please revert to changed(), as bad as the name was.
    
    If you really want to, you can add a new signal, mark the old one as deprecated, and emit both, but in this case I'm not sure it's worth the trouble.
    
    
    Ah -- actually the docu is wrong too, isn't it? From the code it seems to me that your shareChanged() signal is emitted when a file is added/removed/modified in the usershare directory? Is this useful? (I mean if all changes of sharing are done in this code, then you could just emit shareChanged without using KDirWatch, no?)
    
    My suggestion:
    * Rename the signal back to change() to preserve compatibility.
    * Re-add the KDirWatch on smb.conf, for the benefit of applications relying on this.
    * Also emit the same signal when adding/removing shares, directly (no need for KDirWatch::addDir for that).



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

    No need for contains + value + return empty, value does it all. Just do:
    
       return data.value(shareName);


- David


On 2010-11-17 00:46:45, Rodrigo Belem wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/4320/
> -----------------------------------------------------------
> 
> (Updated 2010-11-17 00:46:45)
> 
> 
> 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 1180108 
>   /trunk/KDE/kdelibs/kio/CMakeLists.txt 1180108 
>   /trunk/KDE/kdelibs/kio/kio/ksambashare.h 1180108 
>   /trunk/KDE/kdelibs/kio/kio/ksambashare.cpp 1180108 
>   /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
> 
>

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