[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Review Request: big revamp of Device Notifier
From: "Aaron Seigo" <aseigo () kde ! org>
Date: 2009-09-14 16:15:03
Message-ID: 20090914161503.17942.61736 () localhost
[Download RAW message or body]
> 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).
> wrote:
> 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?
> wrote:
> ok, i'll do this change and i think this evening i'll send the new revision.
>
> no, i don't have an svn account, and yes, i'd like to continue to work on KDE \
> (preferably plasma), but i don't know how much free time i'll have in the future, \
> because on the first of october i'm starting the university
great! go to http://techbase.kde.org/Contribute/Get_a_SVN_Account and get yourself an \
account and you can make this patch your first commit! :)
"i don't know how much free time i'll have in the future"
few of us do know this for sure, but what i always tell people is that when it comes \
to working on things like this and maintaining that work, an hour a week for a year \
is better than 40 hours all together in one week. i'm sure it'll work out and \
whatever time you do have and feel like spending with KDE will be appreciated.
it already is with this patch, in fact :)
- 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
>
>
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic