Android: Replace Java INI parser with C++ INI parser #8941

JosJuice · 2020-07-07T12:22:02Z

Fixes https://bugs.dolphin-emu.org/issues/12096 and opens up for future improvements.

I had to touch a lot of the settings code in the second commit, so testing is needed to make sure that all the settings still work correctly, in particular per-game Wii controller settings.

Ebola16 · 2020-07-20T03:52:39Z

Changing emulated Wii Remote's extension doesn't update the displayed "Extension", however the proper selection is saved. This problem is visible when backing out of a changed extension submenu or changing the value to "None" from a different value.

JosJuice · 2020-07-20T11:03:58Z

Thanks! It should be fixed now.

Ebola16 · 2020-08-07T00:46:40Z

I'll test this again when rebased.

Ebola16

I like the premise of this PR. Gave it a first pass but I'll need to look at this again later.

Shouldn't we have getType() overrides in InvertedCheckBoxSetting and PercentSliderSetting?

The following issue also occur on master but it may be a good idea to address it in this PR. If this is too much work for this PR, feel free to ignore it:

Can you make FilePicker paths work with game-specific settings? That would be useful for switching SD paths for modded games.

.../src/main/java/org/dolphinemu/dolphinemu/features/settings/ui/SettingsFragmentPresenter.java

JosJuice · 2020-08-10T09:19:43Z

Shouldn't we have getType() overrides in InvertedCheckBoxSetting and PercentSliderSetting?

I thought there was no need to report a different type for those two since they use the same ViewHolder as their parents and otherwise behave identically to their parents as far as outside observers can tell (without looking at how they load and save to the settings store). Is there are reason why they should report different values from their parents?

Can you make FilePicker paths work with game-specific settings? That would be useful for switching SD paths for modded games.

This is why I thought it would make sense for PR #9014 to stay open :)

The first commit of PR #8975 should fix this. I think I'm going to split that commit out to a separate PR once this PR is merged, but I could do it even earlier (which would require some conflict resolution) if you want me to.

Ebola16 · 2020-08-10T09:49:31Z

I thought there was no need to report a different type for those two since they use the same ViewHolder as their parents and otherwise behave identically to their parents as far as outside observers can tell (without looking at how they load and save to the settings store). Is there are reason why they should report different values from their parents?

We override getType() in IntSliderSetting and FloatSliderSetting so it seemed strange that we didn't for PercentSliderSetting. I can't test removing the override for all of those files right now but it may be possible since they all inherit from SliderSetting.

This is why I thought it would make sense for PR #9014 to stay open :)
The first commit of PR #8975 should fix this. I think I'm going to split that commit out to a separate PR once this PR is merged, but I could do it even earlier (which would require some conflict resolution) if you want me to.

I'm fine with fixing that issue outside of this PR. I'll look at #8975 again once this one is merged. I'm also fine with splitting #8975 but I'd rather not be the one who splits your PR. It is a large and somewhat complex PR so reviewing my changes to code you've already written may not be the best use of your time.

JosJuice · 2020-08-10T09:59:17Z

We override getType() in IntSliderSetting and FloatSliderSetting so it seemed strange that we didn't for PercentSliderSetting. I can't test removing the override for all of those files right now but it may be possible since they all inherit from SliderSetting.

You're right, that is inconsistent. I'll remove it from IntSliderSetting and FloatSliderSetting.

I'm also fine with splitting #8975 but I'd rather not be the one who splits your PR. It is a large and somewhat complex PR so reviewing my changes to code you've already written may not be the best use of your time.

Yes, I will be splitting it myself.

Ebola16

The following issue also occurs on master but it may be a good idea to address it in this PR:

Occurs with game-specific settings only. Not fixed in Android: Hook up the new config system #8975:

Clear Game Settings for your chosen game.
Emulate "Wii Remote 1" and change the extension.
Save settings.
Return to the "Wii Remote 1" submenu and note that the previously changed extension is correctly displayed.
Press back
Emulate "Wii Remote 2"
Note that the extension is incorrectly displayed as the same extension as "Wii Remote 1"
Changing the "Wii Remote 2" extension or backing out of game-specific settings will cause the correct extension to be displayed again.

Ebola16 · 2020-08-11T03:30:59Z

.../src/main/java/org/dolphinemu/dolphinemu/features/settings/ui/SettingsFragmentPresenter.java

+            R.string.wiimote_scanning_description, true));
+    sl.add(new CheckBoxSetting(SettingsFile.FILE_NAME_DOLPHIN, Settings.SECTION_INI_CORE,
+            SettingsFile.KEY_WIIMOTE_SPEAKER, R.string.wiimote_speaker,
+            R.string.wiimote_speaker_description, true));


When Dolphin.ini is created, WiimoteContinuousScanning and WiimoteEnableSpeaker default to false. Should the defaults be false here too?

This is fixed in PR #8975, but in case you want the fix early, I've submitted it as PR #9024.

JosJuice · 2020-08-11T07:19:54Z

The code for the per-game extension setting is a big mess and I'd rather not touch it more in this PR than I already have.

Ebola16

The code for the per-game extension setting is a big mess and I'd rather not touch it more in this PR than I already have.

That's fine.

Tested one setting from each of our settings model views, both as a general setting and a game-specific setting. Per-game Wii controller settings don’t appear to be made worse but I’ll be able to better test them in #8894. Appears to fix the issue mentioned in this PR. Visually reviewed the rest of the changes.

Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/activities/EmulationActivity.java

Replaces the inflexible INI functions in NativeLibrary.

Fixes https://bugs.dolphin-emu.org/issues/12096.

…ting classes This way we don't have to hardcode any keys inside the classes.

BhaaLseN

Code LGTM

JosJuice force-pushed the android-ini branch 7 times, most recently from 939f56c to 5bad69e Compare July 8, 2020 12:59

JosJuice force-pushed the android-ini branch 6 times, most recently from aec9e0c to 7b8ae75 Compare July 19, 2020 13:33

JosJuice force-pushed the android-ini branch from 7b8ae75 to ed86c05 Compare July 20, 2020 11:03

JosJuice force-pushed the android-ini branch 6 times, most recently from 2da3861 to 58d9b81 Compare July 22, 2020 20:35

JosJuice mentioned this pull request Jul 22, 2020

Android: Hook up the new config system #8975

Merged

JosJuice force-pushed the android-ini branch 2 times, most recently from 7058bbd to cea5e24 Compare August 3, 2020 15:34

JosJuice force-pushed the android-ini branch from cea5e24 to 16dd663 Compare August 7, 2020 08:07

Ebola16 suggested changes Aug 10, 2020

View reviewed changes

.../src/main/java/org/dolphinemu/dolphinemu/features/settings/ui/SettingsFragmentPresenter.java Outdated Show resolved Hide resolved

JosJuice force-pushed the android-ini branch from 16dd663 to c3b2a2c Compare August 10, 2020 09:19

JosJuice force-pushed the android-ini branch from c3b2a2c to 0dec677 Compare August 10, 2020 10:01

Ebola16 reviewed Aug 11, 2020

View reviewed changes

Ebola16 approved these changes Aug 11, 2020

View reviewed changes

JosJuice force-pushed the android-ini branch from 0dec677 to 9bc5f02 Compare August 17, 2020 19:32

Ebola16 mentioned this pull request Aug 30, 2020

Android: Make DSP setting work with custom GameSettings #9057

Closed

JosJuice force-pushed the android-ini branch 3 times, most recently from d6e1d27 to e844496 Compare September 1, 2020 15:59

BhaaLseN reviewed Sep 6, 2020

View reviewed changes

Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/activities/EmulationActivity.java Outdated Show resolved Hide resolved

Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/activities/EmulationActivity.java Outdated Show resolved Hide resolved

JosJuice added 3 commits September 6, 2020 13:29

Android: Expose a proper interface for C++ IniFile class

74f197c

Replaces the inflexible INI functions in NativeLibrary.

Android: Replace Java INI parser with C++ INI parser

c6a3083

Fixes https://bugs.dolphin-emu.org/issues/12096.

Android: Create separate InvertedCheckBoxSetting and PercentSliderSet…

ee9444a

…ting classes This way we don't have to hardcode any keys inside the classes.

JosJuice force-pushed the android-ini branch from e844496 to ee9444a Compare September 6, 2020 11:30

BhaaLseN approved these changes Sep 6, 2020

View reviewed changes

JMC47 merged commit a390640 into dolphin-emu:master Sep 6, 2020
10 checks passed

JosJuice deleted the android-ini branch September 6, 2020 12:27

JosJuice mentioned this pull request Sep 10, 2021

FEATURE REQUEST: GameSettings option. citra-emu/citra-android#244

Open

Android: Replace Java INI parser with C++ INI parser #8941

Android: Replace Java INI parser with C++ INI parser #8941

JosJuice commented Jul 7, 2020 •

edited

Ebola16 commented Jul 20, 2020

JosJuice commented Jul 20, 2020

Ebola16 commented Aug 7, 2020

Ebola16 left a comment

JosJuice commented Aug 10, 2020

Ebola16 commented Aug 10, 2020 •

edited

JosJuice commented Aug 10, 2020

Ebola16 left a comment

Ebola16 Aug 11, 2020

JosJuice Aug 11, 2020

JosJuice commented Aug 11, 2020

Ebola16 left a comment

BhaaLseN left a comment

Android: Replace Java INI parser with C++ INI parser #8941

Android: Replace Java INI parser with C++ INI parser #8941

Conversation

JosJuice commented Jul 7, 2020 • edited

Ebola16 commented Jul 20, 2020

JosJuice commented Jul 20, 2020

Ebola16 commented Aug 7, 2020

Ebola16 left a comment

Choose a reason for hiding this comment

JosJuice commented Aug 10, 2020

Ebola16 commented Aug 10, 2020 • edited

JosJuice commented Aug 10, 2020

Ebola16 left a comment

Choose a reason for hiding this comment

Ebola16 Aug 11, 2020

Choose a reason for hiding this comment

JosJuice Aug 11, 2020

Choose a reason for hiding this comment

JosJuice commented Aug 11, 2020

Ebola16 left a comment

Choose a reason for hiding this comment

BhaaLseN left a comment

Choose a reason for hiding this comment

JosJuice commented Jul 7, 2020 •

edited

Ebola16 commented Aug 10, 2020 •

edited