Review Request: big revamp of Device Notifier

Fri Sep 11 21:44:13 CEST 2009

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

is the latest version the most recent available version of the patch? because it doesn't apply cleanly to trunk.

anyways ... i really don't like the "how long to show the popup" configuration item and the other 2 config items do need seem linguistic and layout adjustments, but that's easy to do once this is in. "how long" could remain configurable in the config file, but i don't think it belongs in the UI; that's just an excuse not to make a good default.

having to click twice for items with one action is not great. instead, when there is just one item how about just putting the action description as the sub-title text for the main item and launch it on click/select? so it would be:

ICON Camera
ICON Open with Digikam...

in the case of multiple items, it could look like:

ICON USB Storage Device
ICON 2 available actions ...

and when it's clicked then the options show up. it could happen on hover, but i'm not sure that makes sense in this case.

if the options could also be made smaller that would also help show that these are "detail items that belong to the item above" better IMO; the icon should probably only need to be roughly as tall as the line of text?

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

    the icon should probably change whether or not the popup is shown. this is more consistent and gives nice visual cues.

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

    the opening { of methods belongs on its own line

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

    not that it matters much here (not a hot path or a big list) but this would probably be more efficient as sth like:

    foreach (const QString &name, m_lastPlugged) {
        onSourceRemoved(name);
    }
    m_lastPlugged.clear();

    if m_lastPlugged must be reset for onSourceRemoved, then sth like:

    QStringList lastPlugged = m_lastPlugged;
    m_lastPlugged.clear();
    foreach (const QString &name, lastPlugged) {
        onSourceRemoved(name);
    }

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

    the opening { of methods belongs on its own line

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

    the opening { of methods belongs on its own line

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

    this is only needed if m_showOnlyRemovable changes, correct?

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

    this method is only used once and is a one-liner, it probably could be folded back into that location. easier to read than jumping around the sources chasing one-liner methods that are rarely used anyways ;)

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

    the opening { of methods belongs on its own line

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

    && not 'and'

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

    the opening { of methods belongs on its own line

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

    unnecessary newline

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

    replace the tab with 4 four spaces

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

    replace the tab with 4 four spaces

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

    KMessageBox blocks all of plasma when used; use Plasma::Applet::showMessage instead.

- Aaron

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