Review Request: Adding "net usershare" suport for KSambaShare
Raphael Kubo da Costa
kubito at gmail.com
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>
More information about the kde-core-devel
mailing list