----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5077/#review7148 -----------------------------------------------------------
Almost ready to go, spotted mostly minor issues which would qualify for a "Ship It" right away. That said, there's one comment which is more related to a potential small scale design change, that's why I'd like a second round of review once you addressed it (if you feel like it's not a good idea, I'm open to discuss it of course). /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabdevice.h <http://reviewboard.kde.org/r/5077/#comment7284> Really needed? (see at the end of the patch) /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabdevice.h <http://reviewboard.kde.org/r/5077/#comment7285> Really needed? (see at the end of the patch) /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabdevice.h <http://reviewboard.kde.org/r/5077/#comment7286> Really needed? (see at the end of the patch) /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabmanager.cpp <http://reviewboard.kde.org/r/5077/#comment7281> This connect will need to be adjusted (see comment below). /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabmanager.cpp <http://reviewboard.kde.org/r/5077/#comment7280> Please rename to "onFileChanged", mind to port the corresponding connect. /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabmanager.cpp <http://reviewboard.kde.org/r/5077/#comment7279> Spurious space at the end of line. /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabstorageaccess.cpp <http://reviewboard.kde.org/r/5077/#comment7282> Adjust to onFileChanged as well. /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabstorageaccess.cpp <http://reviewboard.kde.org/r/5077/#comment7283> Please rename to onFileChanged. /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabstorageaccess.cpp <http://reviewboard.kde.org/r/5077/#comment7287> I would say that instead of using m_fstabDevice->currentMountPoints() in there, it'd be simpler to have the cache of mount points stored in the FstabStorageAccess object itself. (Hence why my "Really needed?" comments at the beginning of this patch). - Kevin On 2010-08-20 18:26:41, Mario Bensi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviewboard.kde.org/r/5077/ > ----------------------------------------------------------- > > (Updated 2010-08-20 18:26:41) > > > Review request for Solid. > > > Summary > ------- > > - watch fstab to notify when a device is added or removed > - watch mtab to notify when a device is mounted or unmounted > - complete devicesFromQuery > - add description in device > > > Diffs > ----- > > /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabdevice.h 1165721 > /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabdevice.cpp 1165721 > /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabmanager.h 1165721 > /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabmanager.cpp 1165721 > /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabstorageaccess.h 1165721 > /trunk/KDE/kdelibs/solid/solid/backends/fstab/fstabstorageaccess.cpp > 1165721 > > Diff: http://reviewboard.kde.org/r/5077/diff > > > Testing > ------- > > > Thanks, > > Mario > >
_______________________________________________ Kde-hardware-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-hardware-devel
