Review Request: Adding "net usershare" suport for KSambaShare

Thu Jul 8 03:39:17 BST 2010

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

I've never had to use Samba, so take this with a grain of salt. The API still looks a bit weird, and I think we should still give it some more love as we won't be able to change it once it's added. People will need to remember to either use some Q*Pointer or manually delete KSambaShareInfo instances returned by add(), but I suspect some leaks will show up. KSambaShareInfo is also only used by add(), which is bit weird as well (I haven't thought of where else to use it, though).

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

    LGPLv2 is incompatible with KDE's Licensing Policy for kdelibs; see http://techbase.kde.org/Policies/Licensing_Policy for more information.

    Can you please use LGPLV2.1+ or the other allowed licenses listed on the TechBase article for the files you've created?

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

    Minor nitpick: extra empty line.

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

    Shouldn't this be KSambaDataPrivate?

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

    existant -> existing? (I'm not a native speaker, so I might be wrong)

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

    This doesn't return a bool.

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

    Why is this commented out?

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

    slots -> Q_SLOTS in kdelibs

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

    Why not remove the declarations from lines 108 and 109 and do:

    const QString key = ...
    const QString value = ...

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

    No need for __func__ here.

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

    This "{" should be in the same line as the if clause.

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

    No need for __func__ here either.

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

    const QString& name

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

    Remove this unused line?

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

    const?

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

    const?

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

    const QString& name

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

    No need for the trailing ;

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

    const QString& name

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

    Why not move the implementation to the cpp file?

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

    No need for a commented out line.

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

    QLatin1String("guest_ok=y")

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

    QLatin1String("guest_ok=n")

- Raphael

On 2010-07-08 01:27:14, Rodrigo Belem wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4320/
> -----------------------------------------------------------
> 
> (Updated 2010-07-08 01:27:14)
> 
> 
> 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/20100708/3a7148b2/attachment.htm>