Review Request: big revamp of Device Notifier

[Kevin Ottens]

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


Looks much better regarding the issues I raised. Thanks a lot for your efforts there!
Still found a few things, but that's really minor issues.

Now as I said previously I can't comment on the rest, so it's in the hands of Aaron and Jacopo to provide comments to keep improving this patch. ;-)


/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.h
<http://reviewboard.kde.org/r/1370/#comment1476>

    Use spaces to indent, not tabs, thanks.
    
    Hmm, actually it seems that the alignment of the whole file is funky, should be 8 spaces everywhere for members... but some lines only have 7 spaces for some reason.



/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp
<http://reviewboard.kde.org/r/1370/#comment1478>

    Why removing this line? Maybe commenting out would be enough?



/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp
<http://reviewboard.kde.org/r/1370/#comment1477>

    Why removing this line?



/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp
<http://reviewboard.kde.org/r/1370/#comment1479>

    Should be StorageAccess here really.



/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp
<http://reviewboard.kde.org/r/1370/#comment1480>

    You might want to process errors, in case the setup() failed, otherwise it does nothing and the user is left wondering what happened.
    
    You have the setupDone() signal for that on StorageAccess.



/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierdialog.cpp
<http://reviewboard.kde.org/r/1370/#comment1481>

    Definitely, we probably want to make a request for that to kde-artists or directly to Nuno.


- Kevin


On 2009-08-21 19:26:43, Giulio Camuffo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1370/
> -----------------------------------------------------------
> 
> (Updated 2009-08-21 19:26:43)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> This is a patch that modifies quite heavily the behaviour of the Device Notifier.
> It comes from here: http://kde-look.org/content/show.php/Device+Manager?content=106051
> It can show the not removable devices too, it can mount them automatically or with a click, since the "eject" button is a "mount" button when the volume is umounted. So that guy on the dot will be ok.
> It can hide some items in the same way as Dolphin's places (hide item/ show all).
> Finally, it shows the various opening actions under the device instead of calling that xp-ish window.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicespaceinfodelegate.cpp 1013960 
>   /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicespaceinfodelegate.h 1013960 
>   /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp 1013960 
>   /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.h 1013960 
>   /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/configurationpage.ui PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/CMakeLists.txt 1013960 
>   /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierdialog.h 1013960 
>   /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierdialog.cpp 1013960 
>   /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierview.h 1013960 
>   /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierview.cpp 1013960 
> 
> Diff: http://reviewboard.kde.org/r/1370/diff
> 
> 
> Testing
> -------
> 
> I'm using it every day since I released 0.1 on Kde-look. I tried all the options on my pc and they work. Some people on kde-look posted some comments about some problems, but it seems to me they are very particular cases, so in my opinion it is quite stable to go in trunk, but anyway review it! :)
> 
> 
> Screenshots
> -----------
> 
> screen
>   http://reviewboard.kde.org/r/1370/s/183/
> 
> 
> Thanks,
> 
> Giulio
> 
>