add gaming_input devices and others to Battery
ClosedPublic

Authored by dollinger on Mar 14 2018, 2:25 PM.

Details

Summary

automatically add new types like gaming_input, as discussed here:
https://forum.kde.org/viewtopic.php?f=204&t=151433&p=395938#p395938

especially necessary for:
https://github.com/atar-axis/xpadneo/issues/21

Diff Detail

Repository
R245 Solid
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dollinger created this revision.Mar 14 2018, 2:25 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 14 2018, 2:25 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
dollinger requested review of this revision.Mar 14 2018, 2:25 PM
aleksejshilin added inline comments.
src/solid/devices/backends/upower/upowerdevice.cpp
98

So you're changing from 'whitelist' to 'blacklist' behavior here. What will happen when another UPower device type is added by the upstream?

dollinger added inline comments.Mar 14 2018, 3:08 PM
src/solid/devices/backends/upower/upowerdevice.cpp
98

Well then it is added automatically, which is what you would expect I think, and that is the reason why gaming_input wasn't detected.

dollinger added inline comments.Mar 14 2018, 3:20 PM
src/solid/devices/backends/upower/upowerdevice.cpp
98

But we can also do it the following way, personally I don't care to be honest:

return (uptype == 2 || uptype == 3 || uptype == 5 || uptype == 6 || uptype == 7 || uptype == 8 || uptype == 12);

While 12 is UP_DEVICE_KIND_GAMING_INPUT

dollinger added a comment.EditedMar 14 2018, 3:33 PM

Btw, this "new" type (UP_DEVICE_KIND_GAMING_INPUT) was added half an year ago, nobody recognized that for six months...

.99.5 vs .99.6

Therefore a blacklist is maybe the way to go - I am not sure - I am new to this code.

I would still prefer a whitelist instead of a balcklist. Recently my mouse has started showing up twice, who knows what else might happen with newer release.
Also, can we use the defines/enums from upower there rather than adding seemingly random numbers?

But then, using headers probably relies on recent upower versions being present, so numbers is fine, but please change it to be a whitelist, then this can surely go in. Thanks for taking care of this!

Okay :) But what's about the other entries, like UP_DEVICE_KIND_TABLET, UP_DEVICE_KIND_MEDIA_PLAYER?
Is there a reason why they are excluded?

Is there a reason why they are excluded?

I don't think there's a particular reason, but there's no type enum in it either, so I suppose HAL didn't have it and it wasns't adjusted for when UPower came around. I also don't think they're as /common, gamepads are much more common. Let's first fix gamepads fully (ie. this patch, then we need a new enum in Solid::Battery and the powermanagement dataengine in Plasma adjusted as well as a new battery icon in the theme etc etc) and then we can look at what else makes sense :)

If you can update your patch to the whitelist you suggested, this could go in.
If you want you can also look at adding a new type enum (separate to this patch I would say), I could guide you through if you want, or just tell me that I should do it

Okay :) But what's about the other entries, like UP_DEVICE_KIND_TABLET, UP_DEVICE_KIND_MEDIA_PLAYER?
Is there a reason why they are excluded?

The reason is probably that they are missing in the documentation (which is likely a bug).

dollinger updated this revision to Diff 29636.EditedMar 15 2018, 9:30 PM

I updated the diff, but unfortunately I'm not sure where to add that enum you @broulik mentioned since there is already one in frontend\battery.h:

enum BatteryType { UnknownBattery, PdaBattery, UpsBattery,
                   PrimaryBattery, MouseBattery, KeyboardBattery,
                   KeyboardMouseBattery, CameraBattery,
                   PhoneBattery, MonitorBattery
                 };

But what we need is this ordering:

typedef enum {
	UP_DEVICE_KIND_UNKNOWN,
	UP_DEVICE_KIND_LINE_POWER,
	UP_DEVICE_KIND_BATTERY,
	UP_DEVICE_KIND_UPS,
	UP_DEVICE_KIND_MONITOR,
	UP_DEVICE_KIND_MOUSE,
	UP_DEVICE_KIND_KEYBOARD,
	UP_DEVICE_KIND_PDA,
	UP_DEVICE_KIND_PHONE,
	UP_DEVICE_KIND_MEDIA_PLAYER,
	UP_DEVICE_KIND_TABLET,
	UP_DEVICE_KIND_COMPUTER,
	UP_DEVICE_KIND_GAMING_INPUT,
	UP_DEVICE_KIND_LAST
} UpDeviceKind;

I updated the diff, but unfortunately I'm not sure where to add that enum you @broulik mentioned since there is already one in frontend\battery.h:

I think you can add an enum to upower.h and use it instead of the defines. I thought we already linked against UPower and could just use its header but we don't and I don't want to change that to avoid a dependency increase.

enum UPowerBatteryType {
    UPowerBatteryTypeBattery,
    UPowerBatteryTypeUps,
    UPowerBatteryTypeMouse,
    ...
    UPowerBatteryTypeGamingInput
};
broulik accepted this revision.Mar 21 2018, 2:25 PM
This revision is now accepted and ready to land.Mar 21 2018, 2:25 PM
broulik added inline comments.Mar 21 2018, 2:27 PM
src/solid/devices/backends/upower/upowerdevice.cpp
93

Sorry, just spotted that you removed the default case, this will cause the compiler to complain about not handled case statements

dollinger updated this revision to Diff 30141.Mar 21 2018, 6:36 PM

Hopefully correct now :D

This revision was automatically updated to reflect the committed changes.

Thanks a lot for yout patience and sorry it had to go through that many revisions.

I took the liberty of altering it to check each value explicitly, so when a new one is added it isn't forgotten to evaluate whether it makes sense to support or not, I hope that's okay.