----------------------------------------------------------- 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 > >
_______________________________________________ Kde-hardware-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-hardware-devel
