----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2179 -----------------------------------------------------------
In general I like it quite a lot! Nice nice nice! I've a few comments: - I'd like to see the actions items to be smaller than the deviceitems, this would help giving the idea of hierarchy, moreover text and boxes need to be centered with the icon - I agree with Aaron, the configuration options are definitely an overkill, a sane default is just better. Instead, the configuration window should make possible to invoke the solid-kcm module and the automounter kcm module - I tend to prefer the normal hierarchical structure (i.e. all rows with the name of the action alongside) It's cleaner and it will not lead to any confusion in case of ambiguous actions Also, when just one action is available the behavior is right now quite confusing (see below) Once this patch is committed, there is still some work to do (the KCapacitybar should be shown on hover, emblems are now provided by solid, we need to get rid of that ugly modal dialog when something bad happens (KNotify is our friend), add a busywidget during long unmounting processes, drag and drop devices). Part of these changes are already in my local copy of the notifier, I hope you will stay around if you can help for other improvements. Thanks a lot for your work and your patience; please fix (or discuss) the remaining issues and for me it's good to commit! Regards --J /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp <http://reviewboard.kde.org/r/1370/#comment1488> We probably need to change this; Either show every time %i actions for this device and show the action items in this case as well, or make the action items appear only if there are more than 1 action. I would strongly suggest the second solution /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierdialog.cpp <http://reviewboard.kde.org/r/1370/#comment1487> I am confused about what you mean by column and row.. they seem reversed to me /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierdialog.cpp <http://reviewboard.kde.org/r/1370/#comment1486> Also, this will need to be changed when we will handle correctly AudioCD and BlankCD, which cannot be mounted but can be ejected, We'll take care of it once committed - Jacopo 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 > > _______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
