Review Request: big revamp of Device Notifier

Aaron Seigo aseigo at kde.org
Mon Sep 14 01:53:28 CEST 2009 


> 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).
> 
>  wrote:
>     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 :)
> 
>  wrote:
>     ok, i misunderstood a bit what you wanted to say.
>     
>     so, yes. in configAccepted() the call to resetDevices() is needed only if m_showOnlyRemovable changes, so we could actually check if the variable changed value and only in the case call resetDevices() and better yet add or remove the not removable devices without reset all the devices.
>     
>     as regards the show/hide system:
>     saying that Show all devices is always enabled you mean it's always checked? if it is it is definitely a bug in the patch. i don't think the fact that the context menu is shown even when there are no devices is a big deal, but anyway it would be simple to make a check.
>     anyway it uses the uuid provided by solid and not the names to identify the devices, so if you have two devices with the same name and you hide one, the other will remain visible (unless the uuid are equals, but i think this is a really rare case, if it is possible).
>     Actually i decided to develop this feature only because a user on kde-look said to me that the applet was of no use for him because he had lots of devices and, unless he was able to hide some of them, the plasmoid was unusable. (comment #7 on http://kde-look.org/content/show.php?content=106051&forumpage=2).

i still think a better answer is to extend the disk monitor.

however, as the places view does indeed have this behaviour, for consistency i suppose the device notifier can/should too. please make it consistent with the places view (e.g. in dolphin), however, so that the "show all" entry isn't shown unless there are hidden items to show. 

after that, this patch can go in afaic.

do you have an svn account? if not, would you like to continue working on this (and/or other items) in KDE's svn?


- 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