Re: Review Request: big revamp of Device Notifier
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=106051forumpage=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? 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
Re: Review Request: big revamp of Device Notifier
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=106051forumpage=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
Re: Review Request: big revamp of Device Notifier
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/ --- (Updated 2009-09-14 21:26:48.596052) Review request for Plasma. Changes --- -doesn't draw anymore the Show all items actions in the context menu if there aren't hidden items. -removed the useSvg in freespacedelegate.cpp Now i'll go and get an SVN account, and after that i'll commit it, ok? 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 (updated) - /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/devicenotifier.cpp 1022457 /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicespaceinfodelegate.h 1022457 /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicespaceinfodelegate.cpp 1022457 /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierdialog.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
Re: Review Request: big revamp of Device Notifier
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2366 --- Ship it! this can go in as a draft of what will be there for 4.4 final; changes that will still need to be made including clearing up the config dialog (i'll do that) and making it so that items with only one action associated/available will launch that action immediately on click. only items with multiple actions should expand a listing of possible actions. - Aaron On 2009-09-14 21:26:48, Giulio Camuffo wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/ --- (Updated 2009-09-14 21:26:48) 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/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/devicenotifier.cpp 1022457 /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicespaceinfodelegate.h 1022457 /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicespaceinfodelegate.cpp 1022457 /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierdialog.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
Re: Review Request: big revamp of Device Notifier
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? 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. 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? it is used also to hide or show the devices when you (un)check the option show all the items in the context menu. - Giulio --- 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
Re: Review Request: big revamp of Device Notifier
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. the capacity bar should be painted below any text. 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. yes, that's what i said :) - 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
Re: Review Request: big revamp of Device Notifier
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. 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 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 :) 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). - Giulio --- 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
Re: Review Request: big revamp of Device Notifier
On 2009-09-11 19:44:22, Aaron Seigo wrote: /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierdialog.cpp, line 374 http://reviewboard.kde.org/r/1370/diff/5/?file=10966#file10966line374 KMessageBox blocks all of plasma when used; use Plasma::Applet::showMessage instead. wrote: What about using notifications? (I have a half patch ready) wouldn't it be better to have the notification associated with the widget itself than pop up somewhere disembodied from the widget? that why i suggested Plasma::Applet::showMessage - it keeps it with the applet. i improved it the other day to work nicely in panels too :) - 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
Re: Review Request: big revamp of Device Notifier
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 :) 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=106051forumpage=2). - Giulio --- 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
Re: Review Request: big revamp of Device Notifier
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=106051forumpage=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
Re: Review Request: big revamp of Device Notifier
On 2009-09-10 18:18:27, Jacopo De Simoi wrote: /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/configurationpage.ui, line 63 http://reviewboard.kde.org/r/1370/diff/5/?file=10960#file10960line63 I am not a native speaker either.. but this the seems strange to me well, i tried with google translate but it leaves me quite confused: http://translate.google.it/translate_t?hl=itie=UTF-8text=mostra+le+periferiche+rimovibilisl=ittl=en#it|en|mostra%20anche%20le%20periferiche%20rimovibili and http://translate.google.it/translate_t?hl=itie=UTF-8text=mostra+le+periferiche+rimovibilisl=ittl=en#it|en|mostra%20le%20periferiche%20rimovibili. I suppose we need to wait for a native speaker :) On 2009-09-10 18:18:27, Jacopo De Simoi wrote: /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierview.cpp, line 194 http://reviewboard.kde.org/r/1370/diff/5/?file=10968#file10968line194 May I ask why do you need to go backward? Sincerely, I don't know. I changed this months ago and I don't remember why. But now i tried to revert it and it seems to work anyway. I really should comment more the code! On 2009-09-10 18:18:27, Jacopo De Simoi wrote: /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierview.cpp, line 342 http://reviewboard.kde.org/r/1370/diff/5/?file=10968#file10968line342 ManagerDialog should be NotifierDialog i suppose, here yes, a little oversight. On 2009-09-10 18:18:27, Giulio Camuffo wrote: I still don't like the fact that you have to click twice to select the action if there is only one available action.. we should find a solution for this.. I agree. Maybe we could show the actions on hover. This would require then a single click, but on the other hand it could be a bit annoying if you have many devices and you have to get to the one on the other side of the applet. - Giulio --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2266 --- 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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
On 2009-09-10 18:18:27, Giulio Camuffo wrote: I still don't like the fact that you have to click twice to select the action if there is only one available action.. we should find a solution for this.. Giulio Camuffo wrote: I agree. Maybe we could show the actions on hover. This would require then a single click, but on the other hand it could be a bit annoying if you have many devices and you have to get to the one on the other side of the applet. Maybe we could show the actions on hover. What about showing them with delay (so fast mouse move won't trigger showing)? - Michal --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2266 --- 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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
On Friday 11 September 2009 05:56:52 Giulio Camuffo wrote: On 2009-09-10 18:18:27, Jacopo De Simoi wrote: /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/configurationpage.ui, line 63 http://reviewboard.kde.org/r/1370/diff/5/?file=10960#file10960line63 I am not a native speaker either.. but this the seems strange to me well, i tried with google translate but it leaves me quite confused: http://translate.google.it/translate_t?hl=itie=UTF-8text=mostra+le+periferiche+rimovibilisl=ittl=en#it|en|mostra%20anche%20le%20periferiche%20rimovibili and http://translate.google.it/translate_t?hl=itie=UTF-8text=mostra+le+periferiche+rimovibilisl=ittl=en#it|en|mostra%20le%20periferiche%20rimovibili. I suppose we need to wait for a native speaker :) IMH(native speaker's)O, that the should not be there, as it makes it sound like there is only one particular removable device that the user wishes to be shown. Matthew signature.asc Description: This is a digitally signed message part. ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
On 2009-09-10 18:18:27, Jacopo De Simoi wrote: /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierview.cpp, line 194 http://reviewboard.kde.org/r/1370/diff/5/?file=10968#file10968line194 May I ask why do you need to go backward? wrote: Sincerely, I don't know. I changed this months ago and I don't remember why. But now i tried to revert it and it seems to work anyway. I really should comment more the code! i noticed now that after i modified the cycle to go forward the button to mount or eject the volume is printed on the row under the device. this happens because going forward the view calculates the rect of the device item, than the ones of its actions. But the actions are painted under the device, so when it starts calculating the button, it puts it under them because the actions increase the verticalOffset' s value. Calculating the button' s rect first and then the devices' and their actions' solves this. On 2009-09-10 18:18:27, Giulio Camuffo wrote: I still don't like the fact that you have to click twice to select the action if there is only one available action.. we should find a solution for this.. Giulio Camuffo wrote: I agree. Maybe we could show the actions on hover. This would require then a single click, but on the other hand it could be a bit annoying if you have many devices and you have to get to the one on the other side of the applet. Michal Dutkiewicz wrote: Maybe we could show the actions on hover. What about showing them with delay (so fast mouse move won't trigger showing)? This could be a solution, but could make this feature less discoverable - Giulio --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2266 --- 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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
On 2009-09-10 18:18:27, Giulio Camuffo wrote: I still don't like the fact that you have to click twice to select the action if there is only one available action.. we should find a solution for this.. Giulio Camuffo wrote: I agree. Maybe we could show the actions on hover. This would require then a single click, but on the other hand it could be a bit annoying if you have many devices and you have to get to the one on the other side of the applet. Michal Dutkiewicz wrote: Maybe we could show the actions on hover. What about showing them with delay (so fast mouse move won't trigger showing)? Giulio Camuffo wrote: This could be a solution, but could make this feature less discoverable This could be a solution, but could make this feature less discoverable Depends how long is that delay. ;-) I think that it could be something like 200 milliseconds, but could be adjusted after some testing. - Michal --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2266 --- 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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2289 --- /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/configurationpage.ui http://reviewboard.kde.org/r/1370/#comment1602 Just my 2 cents: s/insterted/inserted/ - Gilles 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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
On 2009-09-10 18:18:27, Giulio Camuffo wrote: I still don't like the fact that you have to click twice to select the action if there is only one available action.. we should find a solution for this.. Giulio Camuffo wrote: I agree. Maybe we could show the actions on hover. This would require then a single click, but on the other hand it could be a bit annoying if you have many devices and you have to get to the one on the other side of the applet. Michal Dutkiewicz wrote: Maybe we could show the actions on hover. What about showing them with delay (so fast mouse move won't trigger showing)? Giulio Camuffo wrote: This could be a solution, but could make this feature less discoverable Michal Dutkiewicz wrote: This could be a solution, but could make this feature less discoverable Depends how long is that delay. ;-) I think that it could be something like 200 milliseconds, but could be adjusted after some testing. 300ms is the standard we tend to use in Plasma, and this is based on quite a bit of testing. having things show just when you move the mouse through them to get to something else is visually annoying/tiring, but you don't want to wait. 300ms (just under a 1/3rd of a second) seems to be a fairly magic compromise that works well. in any case, i need to download and try the latest version of this patch to see how it's going will do so today. - Aaron --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2266 --- 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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
---
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
Re: Review Request: big revamp of Device Notifier
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1370/#review2300
---
/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicespaceinfodelegate.cpp
http://reviewboard.kde.org/r/1370/#comment1617
when is this not the case?
/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierdialog.cpp
http://reviewboard.kde.org/r/1370/#comment1618
{ should be on a new line
/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierview.cpp
http://reviewboard.kde.org/r/1370/#comment1619
{}s even on one-liners
/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierview.cpp
http://reviewboard.kde.org/r/1370/#comment1620
{ should be on a new line
- 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
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
On 2009-09-11 19:47:12, Aaron Seigo wrote: /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicespaceinfodelegate.cpp, line 87 http://reviewboard.kde.org/r/1370/diff/5/?file=10964#file10964line87 when is this not the case? i took that piece of code from plasmadelegate.cpp. i suppose when composite is disabled? - Giulio --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2300 --- 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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/ --- (Updated 2009-09-11 21:32:17.946758) Review request for Plasma. Changes --- -updated the patch to the latest svn -using Plasma::Applet::showMessage instead of KMessageBox -removed hidePopupAfter(int) -change icon on the insertion of a new device even if the popup is shown -many others style issues and oversights fixed 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 (updated) - /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
Re: Review Request: big revamp of Device Notifier
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2266 --- Much better as for code and configuration, there are just a few remarks down here I'd like you to have a look at /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/configurationpage.ui http://reviewboard.kde.org/r/1370/#comment1594 I am not a native speaker either.. but this the seems strange to me /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp http://reviewboard.kde.org/r/1370/#comment1595 These includes are not needed /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierdialog.cpp http://reviewboard.kde.org/r/1370/#comment1596 Please remove this bit of code, it is most probably a leftover from some previous trunk revisions. /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierview.cpp http://reviewboard.kde.org/r/1370/#comment1597 May I ask why do you need to go backward? /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierview.cpp http://reviewboard.kde.org/r/1370/#comment1566 ManagerDialog should be NotifierDialog i suppose, here I still don't like the fact that you have to click twice to select the action if there is only one available action.. we should find a solution for this.. - Jacopo 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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/ --- (Updated 2009-09-01 16:55:56.303861) Review request for Plasma. Changes --- -moved the mounting code in notifierdialog.cpp -manages the mount errors -a bug fix for the hide/show item system -removed the horizontal layout for the actions -now it's possible to open the actions for one item only at a time -many style fixes 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 (updated) - /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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
On 2009-08-29 21:00:29, Jacopo De Simoi wrote: /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp, line 204 http://reviewboard.kde.org/r/1370/diff/4/?file=10494#file10494line204 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 wrote: I know it. It is simply unuseful to show the action when you already know what it is, but it was far in my priorities list :) wrote: now that I think about it, in my patch the KCapacityBar hides the subtitle, so it would still be needed to show the action item. maybe i should check if the subtitle is visible and in the case call directly the action. wrote: The KCapacityBar will eventually need to be shown only on hover, so the proposed behavior should take into account this fact as well yes, but the subtitle too is shown on hover only. So the problem remains. We could make the items bigger and paint the subtitle along with the KCapacityBar, as in the original patch for the capacity bar, but aesthetically I dislike it very much, because they become fat, or keep showing the item for a single action. Either show every time %i actions for this device and show the action items in this case as well I don't understand what you mean with this phrase. I'm not a native english speaker and some times I have some problems :) - Giulio --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2179 --- 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
Re: Review Request: big revamp of Device Notifier
...last but not least, the bug I reported about this thing sitting on hald/dbus. This too me is a showstopper. The original device notifier certainly does not do this. ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
On Sunday 30 August 2009 08:43:52 David Baron wrote: ...last but not least, the bug I reported about this thing sitting on hald/dbus. This too me is a showstopper. The original device notifier certainly does not do this. Uhm, afaiu the dependency has been removed as requested by Kevin... ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
On 2009-08-24 12:43:32, Aaron Seigo wrote: i'm not going to look at the code just yet. instead i'd like to focus on what's exposed in the configuration dialog first. then we can get to code :) automounting: that's already been discussed on the mailing list so we can skip that one :) show only removable devices: why would we want to show non-removable devices here? show popup when device is inserted: what is the benefit to this that a configuration option is worth while? actions layout: this is a very technical entry, e.g. what is an action? can we just put them in a vertical list, like the rest of the widget, and be done with it? lists with a single dimension to them tend to allow for better visual scanning in most cases anyways. when inserting a device show popup for: this really belongs with the show popup when option (so should be moved closer to it :) and the 0 for never is redundant to the show popup option isn't it? anyways, what is the benefit to this? would it make more sense to have a sensible default and just autohide it at some point? is there really a great benefit to being able to tweak it down to the second? i REALLY like how this gets rid of the ugly old-school popup dialog when there is more than one option, so there is real value to this patch and i think it should go into svn at some point. however, i don't currently see the benefits to any of the configuration options provided here. keeping widgets simple and configuration down to just what is really beneficial is what we should be striving for, otherwise plasma-desktop becomes clumsy to use and the code becomes more difficult to maintain. show only removable devices: why would we want to show non-removable devices here? I find it really useful to be able to have them shown too: I don't use the automount, and I often use the console to browse through the directories. So i need to mount easily the partitions I need to access to without having to open dolphin and go to the place, or mount manually in console. Actually this is maybe the main thing that pushed me to develop it, and I find a bit annoying that the dataengine doesn't shows the partitions managed by fstab too. show popup when device is inserted: what is the benefit to this that a configuration option is worth while? I'm not an usability expert, I simply thought that maybe some people find the popup annoying. actions layout: this is a very technical entry, e.g. what is an action? can we just put them in a vertical list, like the rest of the widget, and be done with it? lists with a single dimension to them tend to allow for better visual scanning in most cases anyways. I like better the vertical listing too, but if the actions are many, and the devices too, the applet could get really long and uncomfortable to use. when inserting a device show popup for: this really belongs with the show popup when option (so should be moved closer to it :) and the 0 for never is redundant to the show popup option isn't it? anyways, what is the benefit to this? would it make more sense to have a sensible default and just autohide it at some point? is there really a great benefit to being able to tweak it down to the second? The 0 means that it won't hide until you hide it manually, I know I'm not able to make interfaces :). Maybe it is not useful to be able to tweak the seconds, but I think we could keep the ability to autohide or not. - Giulio --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2133 --- 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
Re: Review Request: big revamp of Device Notifier
On 2009-08-29 21:00:29, Jacopo De Simoi wrote: /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp, line 204 http://reviewboard.kde.org/r/1370/diff/4/?file=10494#file10494line204 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 I know it. It is simply unuseful to show the action when you already know what it is, but it was far in my priorities list :) On 2009-08-29 21:00:29, Jacopo De Simoi wrote: /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierdialog.cpp, line 183 http://reviewboard.kde.org/r/1370/diff/4/?file=10498#file10498line183 I am confused about what you mean by column and row.. they seem reversed to me you're right, it is a bit ambiguous, maybe it had to be layActionsIn*One*Column. On 2009-08-29 21:00:29, Jacopo De Simoi wrote: /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierdialog.cpp, line 219 http://reviewboard.kde.org/r/1370/diff/4/?file=10498#file10498line219 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 well, I really don't know why, but here on my 4.3 with the applet from kde-look, when I insert an AudioCD there's no button at all. - Giulio --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2179 --- 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
Re: Review Request: big revamp of Device Notifier
In data lunedì 24 agosto 2009 14:32:03, David Baron ha scritto: : Is this the one that has been on kde-look for a while? Sounds like it. This is a worthy revamp of the original device-notifier plasmoid and in 4.3 it is actually readable. The last mounted tooltip is of questionable usefulness. This baby, at least the last kde-look.org version of it, is exercising dbus and hald and plasma-desktop to significant percentage of CPU activity. This indicates a big code problem because this adds to the overall daemon CPU glut that can slow plasma and kde4 to a fast crawl. The kde4-supllied applet does NOT have this problem. I can look myself but am no expert (OK, time for more documentation as plasma's own API and all these activities and delegates and such are making my head swim--ok with anticipation!). If someone is reviewing this code, this problem must be checked and solved as this is a major bug in my mind. I will contact the author. ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel Could you please try the last revision of the patch? Because the version on kde-look uses dbus to say directly to HAL to mount the devices, while the last patch uses the KDE api. ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
On 2009-08-29 21:00:29, Jacopo De Simoi wrote: /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp, line 204 http://reviewboard.kde.org/r/1370/diff/4/?file=10494#file10494line204 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 wrote: I know it. It is simply unuseful to show the action when you already know what it is, but it was far in my priorities list :) now that I think about it, in my patch the KCapacityBar hides the subtitle, so it would still be needed to show the action item. maybe i should check if the subtitle is visible and in the case call directly the action. - Giulio --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2179 --- 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
Re: Review Request: big revamp of Device Notifier
On 2009-08-24 12:43:32, Aaron Seigo wrote: i'm not going to look at the code just yet. instead i'd like to focus on what's exposed in the configuration dialog first. then we can get to code :) automounting: that's already been discussed on the mailing list so we can skip that one :) show only removable devices: why would we want to show non-removable devices here? show popup when device is inserted: what is the benefit to this that a configuration option is worth while? actions layout: this is a very technical entry, e.g. what is an action? can we just put them in a vertical list, like the rest of the widget, and be done with it? lists with a single dimension to them tend to allow for better visual scanning in most cases anyways. when inserting a device show popup for: this really belongs with the show popup when option (so should be moved closer to it :) and the 0 for never is redundant to the show popup option isn't it? anyways, what is the benefit to this? would it make more sense to have a sensible default and just autohide it at some point? is there really a great benefit to being able to tweak it down to the second? i REALLY like how this gets rid of the ugly old-school popup dialog when there is more than one option, so there is real value to this patch and i think it should go into svn at some point. however, i don't currently see the benefits to any of the configuration options provided here. keeping widgets simple and configuration down to just what is really beneficial is what we should be striving for, otherwise plasma-desktop becomes clumsy to use and the code becomes more difficult to maintain. Giulio Camuffo wrote: show only removable devices: why would we want to show non-removable devices here? I find it really useful to be able to have them shown too: I don't use the automount, and I often use the console to browse through the directories. So i need to mount easily the partitions I need to access to without having to open dolphin and go to the place, or mount manually in console. Actually this is maybe the main thing that pushed me to develop it, and I find a bit annoying that the dataengine doesn't shows the partitions managed by fstab too. show popup when device is inserted: what is the benefit to this that a configuration option is worth while? I'm not an usability expert, I simply thought that maybe some people find the popup annoying. actions layout: this is a very technical entry, e.g. what is an action? can we just put them in a vertical list, like the rest of the widget, and be done with it? lists with a single dimension to them tend to allow for better visual scanning in most cases anyways. I like better the vertical listing too, but if the actions are many, and the devices too, the applet could get really long and uncomfortable to use. when inserting a device show popup for: this really belongs with the show popup when option (so should be moved closer to it :) and the 0 for never is redundant to the show popup option isn't it? anyways, what is the benefit to this? would it make more sense to have a sensible default and just autohide it at some point? is there really a great benefit to being able to tweak it down to the second? The 0 means that it won't hide until you hide it manually, I know I'm not able to make interfaces :). Maybe it is not useful to be able to tweak the seconds, but I think we could keep the ability to autohide or not. actions layout: this is a very technical entry, e.g. what is an action? can we just put them in a vertical list, like the rest of the widget, and be done with it? lists with a single dimension to them tend to allow for better visual scanning in most cases anyways. I like better the vertical listing too, but if the actions are many, and the devices too, the applet could get really long and uncomfortable to use. +1 for the vertical listing only; to avoid cluttering I would suggest that only one device could show actions at once. - Jacopo --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2133 --- 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:
Re: Review Request: big revamp of Device Notifier
On 2009-08-29 21:00:29, Jacopo De Simoi wrote: /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp, line 204 http://reviewboard.kde.org/r/1370/diff/4/?file=10494#file10494line204 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 wrote: I know it. It is simply unuseful to show the action when you already know what it is, but it was far in my priorities list :) wrote: now that I think about it, in my patch the KCapacityBar hides the subtitle, so it would still be needed to show the action item. maybe i should check if the subtitle is visible and in the case call directly the action. The KCapacityBar will eventually need to be shown only on hover, so the proposed behavior should take into account this fact as well On 2009-08-29 21:00:29, Jacopo De Simoi wrote: /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierdialog.cpp, line 219 http://reviewboard.kde.org/r/1370/diff/4/?file=10498#file10498line219 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 wrote: well, I really don't know why, but here on my 4.3 with the applet from kde-look, when I insert an AudioCD there's no button at all. Yeah, that's a bug, I've a fix, but I prefer to merge it after your patch, so you don't have to mess around again with that piece of code - Jacopo --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2179 --- 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
Re: Review Request: big revamp of Device Notifier
--- 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
Re: Review Request: big revamp of Device Notifier
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2180 --- - 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
Re: Review Request: big revamp of Device Notifier
The last mounted tooltip is of questionable usefulness. I would like very much to get rid of it, also the information it shows is (mostly) redundant and if the applet is not in a panel it looks plain awful. The last plugged in device is usually the one on top of all the others. There is an exception; if the last plugged device (X) belongs to the same category of some previously plugged in device (Y) AND another device (Z) belonging to another category was plugged in after (Y) but before (X), then the order will be (Z) --- (X) (Y) Imho a tooltip should be shown ONLY if just the icon is visible (i.e. when on the panel) and it could show not only the last plugged devices but a simple list of devices, pretty much what the pager does; If somebody feels that the last plugged in device should be explicitly marked I believe we can show a sort of status line on the bottom of the applet saying: last plugged in device: X, but not more than that Regards --J ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2172 --- Looks much better regarding the issues I raised. Thanks a lot for your efforts there! Still found a few things, but that's really minor issues. Now as I said previously I can't comment on the rest, so it's in the hands of Aaron and Jacopo to provide comments to keep improving this patch. ;-) /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.h http://reviewboard.kde.org/r/1370/#comment1476 Use spaces to indent, not tabs, thanks. Hmm, actually it seems that the alignment of the whole file is funky, should be 8 spaces everywhere for members... but some lines only have 7 spaces for some reason. /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp http://reviewboard.kde.org/r/1370/#comment1478 Why removing this line? Maybe commenting out would be enough? /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp http://reviewboard.kde.org/r/1370/#comment1477 Why removing this line? /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp http://reviewboard.kde.org/r/1370/#comment1479 Should be StorageAccess here really. /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp http://reviewboard.kde.org/r/1370/#comment1480 You might want to process errors, in case the setup() failed, otherwise it does nothing and the user is left wondering what happened. You have the setupDone() signal for that on StorageAccess. /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierdialog.cpp http://reviewboard.kde.org/r/1370/#comment1481 Definitely, we probably want to make a request for that to kde-artists or directly to Nuno. - Kevin 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
Re: Review Request: big revamp of Device Notifier
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2143 --- Hi there, thanks for working on this, I've been mostly disconnected for the last two weeks and until friday I'll not be able to test/review this patch. However, I've been working on it as well lately and part of our work overlap, so it's probably a good idea to wait until I'm back online and then I'll be able to say something meaningful. There are quite a lot of things to be merged together and we have quite a lot of time. Thanks --J - 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
Re: Review Request: big revamp of Device Notifier
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2133 --- i'm not going to look at the code just yet. instead i'd like to focus on what's exposed in the configuration dialog first. then we can get to code :) automounting: that's already been discussed on the mailing list so we can skip that one :) show only removable devices: why would we want to show non-removable devices here? show popup when device is inserted: what is the benefit to this that a configuration option is worth while? actions layout: this is a very technical entry, e.g. what is an action? can we just put them in a vertical list, like the rest of the widget, and be done with it? lists with a single dimension to them tend to allow for better visual scanning in most cases anyways. when inserting a device show popup for: this really belongs with the show popup when option (so should be moved closer to it :) and the 0 for never is redundant to the show popup option isn't it? anyways, what is the benefit to this? would it make more sense to have a sensible default and just autohide it at some point? is there really a great benefit to being able to tweak it down to the second? i REALLY like how this gets rid of the ugly old-school popup dialog when there is more than one option, so there is real value to this patch and i think it should go into svn at some point. however, i don't currently see the benefits to any of the configuration options provided here. keeping widgets simple and configuration down to just what is really beneficial is what we should be striving for, otherwise plasma-desktop becomes clumsy to use and the code becomes more difficult to maintain. - Aaron 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
Re: Review Request: big revamp of Device Notifier
Is this the one that has been on kde-look for a while? Sounds like it. This is a worthy revamp of the original device-notifier plasmoid and in 4.3 it is actually readable. The last mounted tooltip is of questionable usefulness. This baby, at least the last kde-look.org version of it, is exercising dbus and hald and plasma-desktop to significant percentage of CPU activity. This indicates a big code problem because this adds to the overall daemon CPU glut that can slow plasma and kde4 to a fast crawl. The kde4-supllied applet does NOT have this problem. I can look myself but am no expert (OK, time for more documentation as plasma's own API and all these activities and delegates and such are making my head swim--ok with anticipation!). If someone is reviewing this code, this problem must be checked and solved as this is a major bug in my mind. I will contact the author. ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
2009/8/22 Giulio Camuffo giuliocamu...@gmail.com 2009/8/21 Alessandro Diaferia alediaferia at gmail.com 2009/8/21 Giulio Camuffo giuliocamuffo at gmail.com On 2009-08-21 17:23:18, Alessandro Diaferia wrote: /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifie r.h, line 106 http://reviewboard.kde.org/r/1370/diff/3/?file=10433#file10433line106 consider deleting white spaces eh, the problem with white spaces is that they are... white :p they escape! That's why reviewboard shows them :p On 2009-08-21 17:23:18, Alessandro Diaferia wrote: /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifie r.cpp, line 335 http://reviewboard.kde.org/r/1370/diff/3/?file=10434#file10434line335 No need for this anymore: i've committed a fixed for the bug and closed it. whooo! great! thanks =) no problem - Giulio --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2112 --- On 2009-08-21 14:05:09, Giulio Camuffo wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/ --- (Updated 2009-08-21 14:05:09) 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/CMakeLists.tx t 1013960 /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/configuration page.ui PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifie r.h 1013960 /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifie r.cpp 1013960 /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicespacein fodelegate.h 1013960 /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicespacein fodelegate.cpp 1013960 /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierdialo g.h 1013960 /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierdialo g.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 -- Alessandro Diaferia KDE Developer Oh one last thing that comes to my mind is that the configuration dialog does not follow GUI usability guidelines. But that shouldn't be a problem, there can be a patch for it later :) -- Alessandro Diaferia KDE Developer Yes, I know that I'm not good at all at drawing interfaces, and I don't like very much the designer. I prefer very much to work underground :) Anyway, since I don't have an svn account, if you judge the patch ready, feel free to commit it. ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel I've tested the patch and it works correctly. I'd clean indentation a little before committing. Just waiting until sunday before committing so, if someone is against the patch, speak now or shut up forever =) -- Alessandro Diaferia KDE Developer ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
On Saturday 22 August 2009 10:33:56 Alessandro Diaferia wrote: I've tested the patch and it works correctly. I'd clean indentation a little before committing. Just waiting until sunday before committing so, if someone is against the patch, speak now or shut up forever =) Please wait for me to let me a chance of reviewing the new version. Also we should probably wait for the opinion of Jacopo who got appointed as maintainer of this applet (although he might be lacking some time right now). In the end, he's the one who will end up dealing with the bug reports. ;-) No need to rush, we've plenty of time until KDE 4.4. :-) Regards. -- Kévin 'ervin' Ottens, http://ervin.ipsquad.net Ni le maître sans disciple, Ni le disciple sans maître, Ne font reculer l'ignorance. signature.asc Description: This is a digitally signed message part. ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
On 2009-08-20 18:39:43, Kevin Ottens wrote: /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp, line 338 http://reviewboard.kde.org/r/1370/diff/1/?file=10366#file10366line338 Urgh! Please no! You're hard depending on HAL now. It's doomed to fail if HAL behavior changes a bit, or if distros move away from it in favor of something else. We have the solid layer exactly to cope up with those changes. Please use it. wrote: In fact I looked in the Solid documentation for a class to easily mount the devices, but I didn't find anything. ok, found it, Solid::StorageAccess::setup(). But setup isn't very clear as a name, because setup could mean lots of things. I expected mount - Giulio --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2100 --- On 2009-08-21 12:47:32, Giulio Camuffo wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/ --- (Updated 2009-08-21 12:47:32) 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/CMakeLists.txt 1013960 /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/configurationpage.ui PRE-CREATION /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/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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/ --- (Updated 2009-08-21 14:05:09.360260) Review request for Plasma. Changes --- -updated the patch against the new revision, so I modified a bit devicespaceinfodelegate.cpp according to the one on the svn -removed the automount stuff -using Solid::StorageAccess::setup() to mount the devices instead of calling HAL via DBUS 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 (updated) - /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/devicenotifier.h 1013960 /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp 1013960 /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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1370/#review2112
---
The patch looks pretty good to me and really a great step forward in usability
imho. Just fix the little issues i've found and +1 in committing it from my
side. =)
/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.h
http://reviewboard.kde.org/r/1370/#comment1434
consider deleting white spaces
/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp
http://reviewboard.kde.org/r/1370/#comment1435
please try to always use brackets:
if (m_hidePopupAfter != 0) {
hidePopupAfter(m_hidePopupAfter);
}
/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp
http://reviewboard.kde.org/r/1370/#comment1436
No need for this anymore: i've committed a fixed for the bug and closed it.
- Alessandro
On 2009-08-21 14:05:09, Giulio Camuffo wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1370/
---
(Updated 2009-08-21 14:05:09)
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/CMakeLists.txt
1013960
/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/configurationpage.ui
PRE-CREATION
/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/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
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
On 2009-08-21 17:23:18, Alessandro Diaferia wrote: /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.h, line 106 http://reviewboard.kde.org/r/1370/diff/3/?file=10433#file10433line106 consider deleting white spaces eh, the problem with white spaces is that they are... white :p they escape! On 2009-08-21 17:23:18, Alessandro Diaferia wrote: /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp, line 335 http://reviewboard.kde.org/r/1370/diff/3/?file=10434#file10434line335 No need for this anymore: i've committed a fixed for the bug and closed it. whooo! great! thanks - Giulio --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2112 --- On 2009-08-21 14:05:09, Giulio Camuffo wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/ --- (Updated 2009-08-21 14:05:09) 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/CMakeLists.txt 1013960 /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/configurationpage.ui PRE-CREATION /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/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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
2009/8/21 Alessandro Diaferia alediafe...@gmail.com 2009/8/21 Giulio Camuffo giuliocamu...@gmail.com On 2009-08-21 17:23:18, Alessandro Diaferia wrote: /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.h, line 106 http://reviewboard.kde.org/r/1370/diff/3/?file=10433#file10433line106 consider deleting white spaces eh, the problem with white spaces is that they are... white :p they escape! That's why reviewboard shows them :p On 2009-08-21 17:23:18, Alessandro Diaferia wrote: /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp, line 335 http://reviewboard.kde.org/r/1370/diff/3/?file=10434#file10434line335 No need for this anymore: i've committed a fixed for the bug and closed it. whooo! great! thanks =) no problem - Giulio --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/#review2112 --- On 2009-08-21 14:05:09, Giulio Camuffo wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/ --- (Updated 2009-08-21 14:05:09) 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/CMakeLists.txt 1013960 /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/configurationpage.ui PRE-CREATION /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/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 -- Alessandro Diaferia KDE Developer Oh one last thing that comes to my mind is that the configuration dialog does not follow GUI usability guidelines. But that shouldn't be a problem, there can be a patch for it later :) -- Alessandro Diaferia KDE Developer ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: big revamp of Device Notifier
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1370/ --- (Updated 2009-08-21 19:26:43.299357) Review request for Plasma. Changes --- -some style fixes -now NotifierDialog emits a signal deviceClicked() to prevent the popup to desappear while you're using it and a signal actionClicked() that closes it. -removed workaround for bug https://bugs.kde.org/show_bug.cgi?id=199860 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 (updated) - /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
Re: Review Request: big revamp of Device Notifier
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1370/#review2100
---
First of all thanks for undertaking the difficult task to improve the device
notifier. It's far from trivial.
Now, I'm not the device-notifier maintainer so I can't comment on some bits of
the patch, which is quite big and makes it cumbersome to review (to be expected
when you fork for a while then try to merge back). So I couldn't really comment
on the delegate and notifier popup bits.
That said, I attached comments where I could. The issues found range from minor
style or whitespace related issues to major architectural problems (if you take
the workspace as a whole). As is I'd say it jeopardizes the stability of the
workspace by inducing potential hard to fix bugs. The first big change to
tackle are to remove anything related to automounting, and remove anything
trying to talk to HAL directly.
More details in the comments of this review.
Cheers!
/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/configurationpage.ui
http://reviewboard.kde.org/r/1370/#comment1414
Anything related to automounting should be removed. It is currently worked
on independently of Plasma (this feature shouldn't be attached to such an
applet anyway).
The upcoming automounter is likely to be part of the 4.4 release.
/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.h
http://reviewboard.kde.org/r/1370/#comment1415
Careful with your whitespaces... There's quite a few of those through the
patch, I'm not going to comment on them all but that needs to be cleaned up.
/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp
http://reviewboard.kde.org/r/1370/#comment1416
Uh-oh... doesn't look too good, I wonder what you need that for.
Ideally the applet should always go through the data engine or solid.
/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp
http://reviewboard.kde.org/r/1370/#comment1417
This line and the following could go in the if.
/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp
http://reviewboard.kde.org/r/1370/#comment1418
Should be } else { (on the same line) to respect the style.
/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp
http://reviewboard.kde.org/r/1370/#comment1419
As above: } else {
/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp
http://reviewboard.kde.org/r/1370/#comment1420
Urgh! Please no!
You're hard depending on HAL now. It's doomed to fail if HAL behavior
changes a bit, or if distros move away from it in favor of something else.
We have the solid layer exactly to cope up with those changes. Please use
it.
/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp
http://reviewboard.kde.org/r/1370/#comment1421
Moreover you're making it a sync call...
/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp
http://reviewboard.kde.org/r/1370/#comment1422
And what will happen for computers faster/slower than your? You should
probably wait for a notification instead of using a timer with a magic value.
/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/notifierview.cpp
http://reviewboard.kde.org/r/1370/#comment1423
Please use C++ style casts. In this case should probably be a static_cast.
- Kevin
On 2009-08-20 17:26:01, Giulio Camuffo wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1370/
---
(Updated 2009-08-20 17:26:01)
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/CMakeLists.txt
1010116
/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/configurationpage.ui
PRE-CREATION
/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.h
1010116
/trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp
1010116
