Review Request: big revamp of Device Notifier

Thu Aug 20 20:39:35 CEST 2009

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

First of all thanks for undertaking the difficult task to improve the device notifier. It's far from trivial.

Now, I'm not the device-notifier maintainer so I can't comment on some bits of the patch, which is quite big and makes it cumbersome to review (to be expected when you fork for a while then try to merge back). So I couldn't really comment on the delegate and notifier popup bits.

That said, I attached comments where I could. The issues found range from minor style or whitespace related issues to major architectural problems (if you take the workspace as a whole). As is I'd say it jeopardizes the stability of the workspace by inducing potential hard to fix bugs. The first big change to tackle are to remove anything related to automounting, and remove anything trying to talk to HAL directly.

More details in the comments of this review.

Cheers!

/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/configurationpage.ui
<http://reviewboard.kde.org/r/1370/#comment1414>

    Anything related to automounting should be removed. It is currently worked on independently of Plasma (this feature shouldn't be attached to such an applet anyway).

    The upcoming automounter is likely to be part of the 4.4 release.

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

    Careful with your whitespaces... There's quite a few of those through the patch, I'm not going to comment on them all but that needs to be cleaned up.

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

    Uh-oh... doesn't look too good, I wonder what you need that for.

    Ideally the applet should always go through the data engine or solid.

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

    This line and the following could go in the if.

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

    Should be "} else {" (on the same line) to respect the style.

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

    As above: "} else {"

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

    Urgh! Please no!

    You're hard depending on HAL now. It's doomed to fail if HAL behavior changes a bit, or if distros move away from it in favor of something else.

    We have the solid layer exactly to cope up with those changes. Please use it.

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

    Moreover you're making it a sync call...

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

    And what will happen for computers faster/slower than your? You should probably wait for a notification instead of using a timer with a magic value.

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

    Please use C++ style casts. In this case should probably be a static_cast.

- Kevin

On 2009-08-20 17:26:01, Giulio Camuffo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1370/
> -----------------------------------------------------------
> 
> (Updated 2009-08-20 17:26:01)
> 
> 
> 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/CMakeLists.txt 1010116 
>   /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/configurationpage.ui PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.h 1010116 
>   /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp 1010116 
>   /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicespaceinfodelegate.h PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicespaceinfodelegate.cpp PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierdialog.h 1010116 
>   /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierdialog.cpp 1010116 
>   /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierview.h 1010116 
>   /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierview.cpp 1010116 
> 
> 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
> 
>