Skip to content
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: Add a button for accessing controller mappings #11605

Merged
merged 3 commits into from Mar 2, 2023

Conversation

JosJuice
Copy link
Member

The settings GameCube Controller N and Wii Remote N (where N is a number) have two purposes: You can select what controller type you want to use, and also, when you select a controller type (even if you're selecting the one that already is selected), the mapping settings open. This second part is less discoverable than it ideally should be. I'm changing it so that there now is a button for opening the mapping settings instead.

@JosJuice JosJuice mentioned this pull request Feb 26, 2023
10 tasks
@t895
Copy link
Contributor

t895 commented Feb 27, 2023

I like this change, but could we use a settings cog icon instead? I feel like that fits better for the large set of options you get in the next menu. Then we could use the three dot icon just for the advanced mapping dialog. What do you think?

Edit - I'm also just noticing now, when you have a controller disabled, the button just does nothing. That's not great in its current state. Is there a way you could set that view to GONE when disabled?

The settings GameCube Controller N and Wii Remote N (where N is a number)
have two purposes: You can select what controller type you want to use,
and also, when you select a controller type (even if you're selecting the
one that already is selected), the mapping settings open. This second part
is less discoverable than it ideally should be. I'm changing it so that
there now is a button for opening the mapping settings instead.
@JosJuice
Copy link
Member Author

I like this change, but could we use a settings cog icon instead? I feel like that fits better for the large set of options you get in the next menu. Then we could use the three dot icon just for the advanced mapping dialog. What do you think?

Yes, using the settings icon sounds like a good idea. The three dots made it seem just a bit too insignificant.

Undecided on whether we should still go with the three dots for the advanced mapping dialog or if we should make it more consistent, but that decision doesn't have to be made in this PR.

Edit - I'm also just noticing now, when you have a controller disabled, the button just does nothing. That's not great in its current state. Is there a way you could set that view to GONE when disabled?

It would be possible, but it would be annoying to implement cleanly because the logic for which choices do anything is in a different part of the code base. Currently we're jumping through a good number of method calls when the button is pressed. I was kind of pushing the issue aside because I figured setting the controller type to None and then pressing the button would happen pretty rarely, but what do you think?

@t895
Copy link
Contributor

t895 commented Feb 27, 2023

I figured setting the controller type to None and then pressing the button would happen pretty rarely

You're probably right but it doesn't really feel right to just leave that alone

@JosJuice
Copy link
Member Author

I'll try to get it working then.

It was a bit silly having four functions for effectively the same thing
in all of SettingsFragmentView, SettingsFragment, SettingsActivityView,
SettingsActivity, and SettingsActivityPresenter.

With this change, we split on the four MenuTag types in
SettingsActivityPresenter instead of in SettingsAdapter.
@JosJuice
Copy link
Member Author

Done.

Copy link
Contributor

@t895 t895 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After these changes, LGTM

@JosJuice
Copy link
Member Author

JosJuice commented Mar 1, 2023

@t895 Done. But please check that I did it correctly, because GitHub didn't do a nice line-by-line formatting of your second diff for some reason.

@t895
Copy link
Contributor

t895 commented Mar 1, 2023

Oh sorry, I messed up a bit. Could you please add
android:layout_toStartOf="@+id/button_more_settings"
to text_setting_description in both layouts?

Also removed the make_sure_continuous_scan_enabled message.
It doesn't make sense with the new UX.
@JosJuice
Copy link
Member Author

JosJuice commented Mar 2, 2023

Sure thing.

@lioncash lioncash merged commit 6fcec80 into dolphin-emu:master Mar 2, 2023
@JosJuice JosJuice deleted the android-mappings-button branch March 2, 2023 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants