[Kde-hardware-devel] Review Request: Fstab solid backend

[Kevin Ottens]

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


Good start already. Mostly about style issues in there.

Didn't have time (or necessary setup) to test it yet though. Likely next week.
Obviously missing feature though: reacting to changes in the fstab (to actually emit deviceAdded/deviceRemoved) or the mtab (to emit accessibilityChanged), but that can wait until we have a first committable version.


/trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabdevice.cpp
<http://reviewboard.kde.org/r/4984/#comment7039>

    Why the #ifndef and #define there? Completely unneeded.



/trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabdevice.cpp
<http://reviewboard.kde.org/r/4984/#comment7040>

    Could you collapse those three namespaces into a "using namespace Solid::Backends::Fstab".
    
    I know we kind of used this style in the other backends, but really "using namespace" would be much better...



/trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabdevice.cpp
<http://reviewboard.kde.org/r/4984/#comment7041>

    Please put this opening curly brace on the previous line to follow the style.



/trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabmanager.cpp
<http://reviewboard.kde.org/r/4984/#comment7042>

    See the "using namespace" comment in fstabdevice.cpp



/trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabmanager.cpp
<http://reviewboard.kde.org/r/4984/#comment7043>

    Should likely return allDevices() only if the type is StorageAccess, otherwise an empty list.



/trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabmanager.cpp
<http://reviewboard.kde.org/r/4984/#comment7044>

    What about "Network Shares" for the description of the root device?



/trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabmanager.cpp
<http://reviewboard.kde.org/r/4984/#comment7045>

    Sounds like this slot is unused.



/trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabmanager.cpp
<http://reviewboard.kde.org/r/4984/#comment7046>

    Ditto.



/trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabstorageaccess.h
<http://reviewboard.kde.org/r/4984/#comment7047>

    Trailing whitespace.



/trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabstorageaccess.h
<http://reviewboard.kde.org/r/4984/#comment7048>

    Should be SOLID_BACKENDS_FSTAB_STORAGEACCESS_H I think.



/trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabstorageaccess.cpp
<http://reviewboard.kde.org/r/4984/#comment7049>

    Trailing whitespace.



/trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabstorageaccess.cpp
<http://reviewboard.kde.org/r/4984/#comment7050>

    Same "using namespace" comment than for fstabdevice.cpp



/trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabstorageaccess.cpp
<http://reviewboard.kde.org/r/4984/#comment7051>

    Remove this empty line please.



/trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabstorageaccess.cpp
<http://reviewboard.kde.org/r/4984/#comment7052>

    Remove this empty line please.



/trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabstorageaccess.cpp
<http://reviewboard.kde.org/r/4984/#comment7055>

    No need for an intermediate variable.



/trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabstorageaccess.cpp
<http://reviewboard.kde.org/r/4984/#comment7053>

    Wrongly aligned with the line " on the previous line. Please remove one space.



/trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabstorageaccess.cpp
<http://reviewboard.kde.org/r/4984/#comment7056>

    No need for an intermediate variable.



/trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabstorageaccess.cpp
<http://reviewboard.kde.org/r/4984/#comment7054>

    Wrongly aligned with the line " on the previous line. Please remove one space.



/trunk/KDE/kdelibs/solid/solid/backends/fstab/rootdevice.cpp
<http://reviewboard.kde.org/r/4984/#comment7057>

    Same "using namespace" comment than fstabdevice.cpp



/trunk/KDE/kdelibs/solid/solid/managerbase.cpp
<http://reviewboard.kde.org/r/4984/#comment7038>

    Please keep the empty line after this one.


- Kevin


On 2010-08-11 12:44:30, Mario Bensi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4984/
> -----------------------------------------------------------
> 
> (Updated 2010-08-11 12:44:30)
> 
> 
> Review request for Solid.
> 
> 
> Summary
> -------
> 
> Fstab solid backend. give informations and mechanism (to mount/unmount) on nfs and smb partition.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/solid/solid/CMakeLists.txt 1162113 
>   /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabdevice.h PRE-CREATION 
>   /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabdevice.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabhandling.h PRE-CREATION 
>   /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabhandling.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabmanager.h PRE-CREATION 
>   /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabmanager.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabservice.h PRE-CREATION 
>   /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabstorageaccess.h PRE-CREATION 
>   /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabstorageaccess.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/solid/solid/backends/fstab/rootdevice.h PRE-CREATION 
>   /trunk/KDE/kdelibs/solid/solid/backends/fstab/rootdevice.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/solid/solid/managerbase.cpp 1162113 
> 
> Diff: http://reviewboard.kde.org/r/4984/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Mario
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-hardware-devel/attachments/20100813/02b73bd0/attachment-0001.htm