New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Android: Use touch emulation of IR by default #8729
Android: Use touch emulation of IR by default #8729
Conversation
0357c99
to
ca3586f
Compare
| enum class CanBeDisabled | ||
| { | ||
| No, | ||
| Yes, | ||
| AlwaysEnabled, | ||
| EnabledByDefault, | ||
| DisabledByDefault, | ||
| }; | ||
|
|
||
| explicit ControlGroup(std::string name, GroupType type = GroupType::Other, | ||
| CanBeDisabled can_be_disabled = CanBeDisabled::No); | ||
| CanBeDisabled can_be_disabled = CanBeDisabled::AlwaysEnabled); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps there is a reason you did it this way, but it seems more readable to leave the CanBeDisabled enum untouched and in stead initialize the ControlGroup's existing enabled member through an additional constructor argument, like this:
ControlGroup(std::string name, GroupType type = GroupType::Other,
CanBeDisabled can_be_disabled = CanBeDisabled::No,
EnabledByDefault enabled_by_default = EnabledByDefault::Yes);
edit: or
Enabled enabled = Enabled::Yes);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your suggestion works too, though I think it's more wordy. Does anyone else have an opinion on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd keep the one-enum solution, but rename CanBeDisabled to something like DefaultState, so that the options become DefaultState::Enabled, DefaultState::Disabled and DefaultState::AlwaysEnabled, which is more readable IMO. Using two enums has the drawback of making it possible to specify a nonsensical combination like (CanBeDisabled::No, EnabledByDefault::No).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made it use @leoetlino's suggestion.
While having motion control emulation of IR enabled by default makes sense in situations like using a DualShock 4 on a PC, Android has the additional option of touch emulation of IR which seems to be better liked, and the default value which was chosen with PC in mind was carried over to Android without any particular consideration. This change disables motion control emulation of IR by default on Android only.
ca3586f
to
18a4afb
Compare
While having motion control emulation of IR enabled by default makes sense in situations like using a DualShock 4 on a PC, Android has the additional option of touch emulation of IR which seems to be better liked, and the default value which was chosen with PC in mind was carried over to Android without any particular consideration. This change disables motion control emulation of IR by default on Android only.