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

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 Ebola16:GFXUI branch 2 times, most recently from 487a9dd to 92acf19 Feb 19, 2019

@Ebola16

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2019

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

@MayImilae
Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

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

@Ebola16

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

...nobody?

@jordan-woyak

This comment has been minimized.

Copy link
Member

commented May 2, 2019

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

@Dockolm

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link

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 Ebola16:GFXUI branch 3 times, most recently from c2bf515 to 2e9b869 Jul 16, 2019

@Ebola16

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

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

@Ebola16 Ebola16 force-pushed the Ebola16:GFXUI branch from 2e9b869 to 7d98c4f Aug 21, 2019

@Ebola16

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2019

...any other reviews?

@Ebola16

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2019

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

@Helios747

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2019

yolo

@Helios747 Helios747 merged commit ecef374 into dolphin-emu:master Sep 1, 2019

10 checks passed

default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on builder lint
Details
pr-android Build succeeded on builder pr-android
Details
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
pr-freebsd-x64 Build succeeded on builder pr-freebsd-x64
Details
pr-osx-x64 Build succeeded on builder pr-osx-x64
Details
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
pr-win-dbg-x64 Build succeeded on builder pr-win-dbg-x64
Details
pr-win-x64 Build succeeded on builder pr-win-x64
Details

@Ebola16 Ebola16 deleted the Ebola16:GFXUI branch Sep 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.