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: Graphic setting description improvements #7810

Merged
merged 3 commits into from
Sep 1, 2019

Conversation

Ebola16
Copy link
Member

@Ebola16 Ebola16 commented Feb 19, 2019

Follow-up to #7591 and #7121.

Implements SingleChoiceSettingDynamicDescriptions and applies it to the Shader Compilation Mode setting.

Pinging @mahdihijazi @zackhow @MayImilae and @JosJuice in case you're interested in this.

screenshot_20190219-134448_dolphin emulator

@Ebola16 Ebola16 force-pushed the GFXUI branch 2 times, most recently from 487a9dd to 92acf19 Compare February 26, 2019 00:41
@Ebola16
Copy link
Member Author

Ebola16 commented Mar 20, 2019

Any review comments for this? This is the simplest PR I'm proposing.

Copy link
Contributor

@MayImilae MayImilae left a comment

Choose a reason for hiding this comment

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

Looks good! Very simple PR indeed, but for the best.

Source/Android/app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@Ebola16
Copy link
Member Author

Ebola16 commented Apr 23, 2019

... Does anyone else want to review / merge this?

@Ebola16
Copy link
Member Author

Ebola16 commented May 2, 2019

...nobody?

@jordan-woyak
Copy link
Member

@Ebola16 Unfortunately I don't know if we have anyone right now that cares enough about Android to review or even test this.

@Dockolm
Copy link

Dockolm commented May 3, 2019

The code looks good overall.
Though I'm not a fan of writing feature-specific code on SingleChoiceViewHolder. Can we consider to create another generic ViewHolder that is designed to take a description for each choice ?
Also, I'm still more or less a beginner at programming, so feel free to correct me if I'm wrong.

@Ebola16
Copy link
Member Author

Ebola16 commented May 7, 2019

In SettingsAdapter, TYPE_SINGLE_CHOICE is tied to SingleChoiceViewHolder. If I create a new viewholder that displays the descriptions like this PR does, I think I'll need to create a new SettingsItem.TYPE and viewholder for each SettingsItem.TYPE if we want to expand this change to the other TYPEs. Since what I propose is less LOC, I like my approach better. I'm also relatively new to programming so let me know if I'm missing something.

@Dockolm
Copy link

Dockolm commented May 9, 2019

Should be okay then, as long as we don't have many special cases like that.
Tested the GUI, works fine.
I can't test if the ubershader setting is applied correctly, my Android device can't run Dolphin. But it should be fine.

@Ebola16 Ebola16 force-pushed the GFXUI branch 3 times, most recently from c2bf515 to 2e9b869 Compare July 16, 2019 05:55
@Ebola16
Copy link
Member Author

Ebola16 commented Jul 16, 2019

I tried a different approach but I'm not sure if I did it properly. Ready for review.

@Ebola16
Copy link
Member Author

Ebola16 commented Aug 21, 2019

...any other reviews?

@Ebola16
Copy link
Member Author

Ebola16 commented Aug 30, 2019

...if this looks good can it be merged? This is needed for future PRs.

@Helios747
Copy link
Contributor

yolo

@Helios747 Helios747 merged commit ecef374 into dolphin-emu:master Sep 1, 2019
@Ebola16 Ebola16 deleted the GFXUI branch September 2, 2019 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants