Review Request 127263: Add Wireless Energy Saving action

Kai Uwe Broulik kde at privat.broulik.de
Thu Mar 3 14:03:43 UTC 2016 


> On März 3, 2016, 1:36 nachm., Kai Uwe Broulik wrote:
> > > y) Switching to a profile where the action is enabled too and options are turned off -> will turn on the wifi/wwan/bt
> > z) Switching to a profile where the action is disabled -> will recover the previous state of the wifi/wwan/bt
> > 
> > Ah, now we're talking ;) This looks confusing, however; I'd rather prefer if there were a ComboBox for this rather than a CheckBox, for me as a user having the Action enabled but none of its options checked should be the same as having the Action disabled altogether. I think it should always just restore the previous state, never enable if it wasn't enabled before.
> > 
> > ComboBox could be like: [Follow user's preferences (better wording: but whatever is set in Plasna NM) | Turn on (always enable, maybe you really want Bluetooth to always turn on on AC, if you have a desktop with a BT mouse | Turn off (what the checkbox was previously)]
> 
> Jan Grulich wrote:
>     Ok, I think that combobox is a better option. When using combobox, you want to have the action enabled by default and to have "Follow user's preferences" as default option in combobox? I understand that you meant that as a replacement for disabled action.

I think the action should be disabled by default and then maybe have "Turn off" as default option for the ComboBox, I think if you actually enable the action you want it to turn it off usually. Disabled option and "Follow user's preferences" are basically the same but you might want to only enable/disable Bluetooth while leaving everything else unchanged; or enable WiFI only on AC and WWAN only on Battery.

The options should also have an icon, I recognize the blue Bluetooth icon better than the text. I would prefer if you split the keywords thing from the actual feature, so reviewing becomes easier.

Also, can you please include a screenshot for usability, they don't read code ;)


- Kai Uwe


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127263/#review93095
-----------------------------------------------------------


On März 3, 2016, 1:30 nachm., Jan Grulich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127263/
> -----------------------------------------------------------
> 
> (Updated März 3, 2016, 1:30 nachm.)
> 
> 
> Review request for Plasma, Solid, KDE Usability, and Kai Uwe Broulik.
> 
> 
> Repository: powerdevil
> 
> 
> Description
> -------
> 
> This action adds an option to turn off wifi/wwan/bt once you switch profile (e.g you unplugg the power cable and start running on battery).
> 
> One more thing. Due to usage of NetworkManagerQt I had to add add_definitions(-DQT_NO_KEYWORDS) into CMakeLists.txt  like we do in plasma-nm to avoid compilation error (thanks to NetworkManager) and replace all keywords by their Qt equivalent (e.g signals ? Q_SIGNALS).
> 
> How it behaves:
> 1) Switching from "AC" profile to "battery" (or from "battery" to "low battery" which is the same situation):
>    a) When the action is enabled in "AC" profile and options to turn off wifi/wwan/bt are enabled
>       x) Switching to a profile where the action is enabled too and options are turned on ? will do nothing as they should be already turned off
>       y) Switching to a profile where the action is enabled too but options are turned off ? will do nothing as the "battery" profile is more conservative and we have those devices disabled already in less conservative profile
>       z) Switching to a profile where the action is disabled ? will do nothing as there is nothing to do
>    b) When the action is enabled in "AC" profile and options to turn off wifi/wwan/bt are disabled
>       ? this should behave according to the more conservative profile, if the options are enabled then all devices will be disabled too
>    C) When the action is disabled in "AC" profile
>       ? should behave as in the case 1-b
> 2) Switching from "battery" profile to "AC" profile (or from "low battery" to "battery" which is the same situation):
>    a) When the action is enabled in "battery" profile and options to turn off wifi/wwan/bt are enabled
>       x) Switching to a profile where the action is enabled too and options are turned on ? will do nothing as it's same setup
>       y) Switching to a profile where the action is enabled too and options are turned off ? will turn on the wifi/wwan/bt
>       z) Switching to a profile where the action is disabled ? will recover the previous state of the wifi/wwan/bt
>    b) When the action is enabled in "battery" profile and options to turn off wifi/wwan/bt are disabled
>       x) Switching to a profile where the action is enabled too and options are turned on ? will do nothing as the option in "battery" should be ignored in this case due to more conservative profile
>       y) Switching to a profile where the action is enabled too and options are turned off ? will do nothing, same configuration
>       z) Switching to a profile where the action is disabled ? will recover the previous state of the wifi/wwan/bt
>    c) When the action is diabled in "battery" profile
>       ? will just recover the previous state of the wifi/wwan/bt as there is nothing to change according to the new profile
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt e7fff17 
>   daemon/CMakeLists.txt bbfe191 
>   daemon/actions/bundled/CMakeLists.txt 45abea3 
>   daemon/actions/bundled/handlebuttonevents.h 5d602d7 
>   daemon/actions/bundled/handlebuttonevents.cpp 7e1d16e 
>   daemon/actions/bundled/handlebuttoneventsconfig.cpp b1d4ef0 
>   daemon/actions/bundled/org.kde.Solid.PowerManagement.Actions.WirelessPowerSaving.xml PRE-CREATION 
>   daemon/actions/bundled/powerdevilwirelesspowersavingaction.desktop PRE-CREATION 
>   daemon/actions/bundled/suspendsession.h d0fc78d 
>   daemon/actions/bundled/wirelesspowersaving.h PRE-CREATION 
>   daemon/actions/bundled/wirelesspowersaving.cpp PRE-CREATION 
>   daemon/actions/bundled/wirelesspowersavingconfig.h PRE-CREATION 
>   daemon/actions/bundled/wirelesspowersavingconfig.cpp PRE-CREATION 
>   daemon/backends/upower/backlighthelper.h b5ce7dc 
>   daemon/backends/upower/backlighthelper.cpp 58b82be 
>   daemon/backends/upower/powerdevilupowerbackend.h 32f0ee4 
>   daemon/backends/upower/powerdevilupowerbackend.cpp 6133887 
>   daemon/backends/upower/udevqtclient.cpp 52b17f6 
>   daemon/kwinkscreenhelpereffect.h bc1c21b 
>   daemon/kwinkscreenhelpereffect.cpp dec5d65 
>   daemon/powerdevilactionconfig.cpp 2565b0d 
>   daemon/powerdevilactionpool.cpp 2091879 
>   daemon/powerdevilbackendinterface.cpp 7aadb01 
>   daemon/powerdevilcore.cpp f02d474 
>   daemon/powerdevilfdoconnector.cpp d9a3ee6 
>   daemon/powerdevilpolicyagent.cpp e0d1ec6 
>   daemon/powerdevilprofilegenerator.cpp cff3120 
>   kcmodule/activities/activitypage.cpp ec18dee 
>   kcmodule/activities/activitywidget.cpp 5992a4d 
>   kcmodule/common/actioneditwidget.cpp 216399c 
>   kcmodule/global/GeneralPage.h a34d54f 
>   kcmodule/global/GeneralPage.cpp b2d9767 
>   kcmodule/profiles/EditPage.h 4791017 
>   kcmodule/profiles/EditPage.cpp 30973b2 
> 
> Diff: https://git.reviewboard.kde.org/r/127263/diff/
> 
> 
> Testing
> -------
> 
> I did some basic testing like (un)plugging the power cable and checking whether it applied the correct configuration.
> 
> 
> Thanks,
> 
> Jan Grulich
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160303/7b6e4062/attachment.html>

More information about the Plasma-devel mailing list