Review Request: Adding "net usershare" suport for KSambaShare

Wed Nov 10 04:30:06 GMT 2010

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

Most of the lines I've reviewed are related to coding style/QLatin1String issues. Please try compiling with QT_NO_CAST_FROM_ASCII to fix the latter, and take a look at the astyle script in TechBase for the former.

Aside from that, I still haven't given my "Ship it" due to the other issues that are present and are more serious than those.

/trunk/KDE/kdelibs/kio/CMakeLists.txt
<http://svn.reviewboard.kde.org/r/4320/#comment9057>

    You'll probably also want to add KSambaSharedData to kdelibs/includes/CMakeLists.txt

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

    @since 4.6

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

    Extra empty line.

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

    @since 4.6

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

    Do you really want pointers here, even when you're using QExplicitlySharedDataPointer? I foresee leaks.

    Plus, missing apidox and @since 4.6

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

    Same thing about using pointers and missing apidox.

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

    I am not 100% sure this set of changes (renaming d to d_ptr, renaming _k_slotFileChange to fileChange) is really binary compatible.

    Anyone?

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

    The trailing ';' will make compilation fail with -pedantic.

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

    kdelibs coding style; surround the inner statement with {}'s.

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

    As we're trying to make kdelibs compile with QT_NO_CAST_FROM_ASCII, please use QLatin1String here.

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

    const QString rawString = ...

    Also, please use QLatin1String.

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

    const QFileInfo ...

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

    kdelibs coding style:

    if (x) {
      y();
    }

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

    Maybe you should set LANG to C to make sure the process spits messages in the language you're expecting (I don't know if Samba translates these messages, though)?

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

    QLatin1String.

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

    QLatin1String.

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

    Same thing about using {}'s even for single-statement ifs.

    Besides, QLatin1String.

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

    QLatin1String.

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

    Still missing the stderr?

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

    Idem.

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

    QLatin1String.

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

    QLatin1String.

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

    Still missing stderr?

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

    {}'s for single-statement ifs.

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

    if () {
    }

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

    Can't shareData be const here?

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

    if () {
    }

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

    QLatin1String in a few places.

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

    QLatin1String.

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

    QLatin1String.

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

    Given my total lack of knowledge of Samba, it makes sense not to remove shareData from data, right?

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

    QLatin1String.

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

    QLatin1String.

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

    QLatin1String

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

    QLatin1String

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

    Still need to output the key name?

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

    Since this is public API, you need apidox, and @since 4.6.

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

    Nitpick: extra empty line.

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

    QLatin1String

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

    You've already included <QtCore/QSharedData>.

- Raphael

On 2010-11-09 21:07:00, Rodrigo Belem wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/4320/
> -----------------------------------------------------------
> 
> (Updated 2010-11-09 21:07:00)
> 
> 
> Review request for kdelibs, Raphael Kubo da Costa, Jonathan Thomas, Aurélien Gâteau, Jonathan Riddell, Adenilson Cavalcanti, and loureiro.
> 
> 
> 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 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/20101110/f774433e/attachment.htm>