Review Request: big revamp of Device Notifier

Aaron Seigo aseigo at kde.org
Sun Sep 13 20:55:34 CEST 2009 


> On 2009-09-11 19:44:22, Aaron Seigo wrote:
> > 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?
> > 
> >
> 
> Giulio Camuffo wrote:
>     The problem is that the subtitle could be covered by the KCapacityBar. So we need to find a solution, as to show the actions on hover.
> 
> Aaron Seigo wrote:
>     the capacity bar should be painted below any text.
> 
> Giulio Camuffo wrote:
>     that would solve all, but personally i dislike very much from an aesthetically pov the items as they are painted in the current device notifier, with the increased height. i find them fat. and if the volumes are umounted there would be no capacity bar, and that would result in a lot of wasted white space
> 
> Jacopo De Simoi wrote:
>     Yeah, no unnececessary white space would be definitely a +

why would the items be taller if there is no capacity bar? the items should be as tall as necessary to show the content they have to show. no more, no less. and items do not need to be the same height (so if there is no capacity bar, that item may be shorter than others). there should be no unused whitepsace.

keeping in mind that the entire point of the device notifier is to give quick access to actions, i think it's just fine to have a bit of extra space for text saying what will happen when you click on the device icon.


another possibility might be to show the capacity bar under the icon, so that the top of the icon and the bottom of the capcity bar line up with the top and bottom of the text to the right.


> On 2009-09-11 19:44:22, Aaron Seigo wrote:
> > /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp, line 366
> > <http://reviewboard.kde.org/r/1370/diff/5/?file=10962#file10962line366>
> >
> >     this is only needed if m_showOnlyRemovable changes, correct?
> 
>  wrote:
>     it is used also to hide or show the devices when you (un)check the option "show all the items" in the context menu.
> 
>  wrote:
>     yes, that's what i said :)
> 
>  wrote:
>     no, i mean when you right click the dialog, and you set the option to show or not even devices that normally would be hidden too. like when you right click on the Places panel in dolphin. In fact resetDevices() is called in configAccepted() (if m_showOnlyRemovable changes) and in setAllItemsShown(bool shown) (if m_showAll changes).

ok, let's back up a bit here.

that call to resetDevices is in DeviceNotifier::configAccepted. so i'm talking about when the configuration is changed. that means that in configAccepted, which is what line 365 here is, resetDevices _does not need to be called_ unless m_showOnlyRemovable changes. make sense?

now forward again :)

looking a m_showAll in NotifierView, Show All devices is always in the context menu and always enabled, even when there is nothing to show. probably, if there are no items to show there doesn't need to be a context menu in the view at all.

i'm still not really sure what the real world use cases for hiding/showing individual items in the notifier are other than "it's neat that i can"; i'm trying to imagine someone who has so many items listed there that it's unmanagable otherwise. or someone who keeps two items in one notifier widget and has a second notifier widget with some others? hm.

on the other hand, i can see this resulting in confusion when when someone plugs a device in, marks it to not be shown, then later plugs it in (and hopefully it's impossible to have different devices with the same name?) and it doesn't show up. particularly is days or weeks have passed between those two events.

and why would i NOT want my camera to show up when i plug it in? what's the use case for that?

this is why i'm still really unsure that turning the device *notifier* into a device *listing manager* in this way is the best of ideas.

there's really two similar ideas here it seems: managing devices that are plugged and unplugged (which may or may not be storage devices) and listing storage devices. we actually have a widget for both of those tasks, but the storage device lister doesn't provide access to actions related to the device, which the device notifier does.

this really comes back to use cases. can you provide use cases to go with the hide/show individual items feature?

oh, and the widget should "emit configNeedsSaving()" after calls to writeEntry :)


- Aaron


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


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

More information about the Plasma-devel mailing list