[Kde-hardware-devel] Review Request: complete fstab solid backend

[Kevin Ottens]

-----------------------------------------------------------
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
> 
>

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