Adding the possibility to disable sounds in app settings #835

Merged
merged 3 commits into from Mar 20, 2016

Projects

None yet

3 participants

@Souigetsu

Yosh !

I just pushed the option in setting to disable sounds , by using the "--disable-audio" that was mentioned in the #575 issue. I hope that fits with what was asked.

Oui-Oui baguette. Cf BlisterB haha

@Ghabry Ghabry and 1 other commented on an outdated diff Mar 20, 2016
builds/android/res/values/strings.xml
@@ -80,6 +80,7 @@ Please tell us in detail what went wrong.\n
<string name="add_game_folder">Add a game folder</string>
<string name="enable_vibration">Enable vibrations</string>
<string name="vibrate_when_sliding_direction">Vibrate when sliding to another direction</string>
+ <string name="disable_sounds">Disable sounds</string>
@Ghabry
Ghabry Mar 20, 2016 Member

Because it's a check box it should be checked by default and have the label "Enable audio" (Disabling by checking is confusing)

@Souigetsu
Souigetsu Mar 20, 2016

Yeah i had a another perspective , ill change that no problem

@Ghabry Ghabry and 1 other commented on an outdated diff Mar 20, 2016
.../android/src/org/easyrpg/player/SettingsActivity.java
import android.widget.Toast;
+import android.media.AudioManager;
@Ghabry
Ghabry Mar 20, 2016 Member

Why are you importing these media libs?

@Souigetsu
Souigetsu Mar 20, 2016

Just forgot to delete this from an older test. i will take it off.

@Ghabry Ghabry and 1 other commented on an outdated diff Mar 20, 2016
.../android/src/org/easyrpg/player/SettingsActivity.java
@@ -285,6 +294,19 @@ public void checkboxVibrateWhenSlidingToAnotherDirection(View v) {
editor.commit();
}
+ // audio
+ public void checkboxDisableSounds(View v) {
+ CheckBox s = (CheckBox) v;
+ if(s.isChecked()){
+ editor.putBoolean(getString(R.string.disable_sounds),true);
+ }
+ else{
+ editor.putBoolean(getString(R.string.disable_sounds),false);
+
+ }
+ editor.commit();
+ }
@Ghabry
Ghabry Mar 20, 2016 Member

Just for Information: Because the if-condition is a bool this can be simplified to a one-liner:

editor.putBoolean(getString(R.string.disable_sounds), s.isChecked());
@Souigetsu
Souigetsu Mar 20, 2016

It's better i agree

@Souigetsu

Alright i'll do the changes soon , my bad, i guess i was not focused enough

@Souigetsu Souigetsu closed this Mar 20, 2016
@Ghabry
Member
Ghabry commented Mar 20, 2016

Instead of closing (and opening a new PR) you can also add another commit oO

@Souigetsu

Sorry i wasn't sure about the right procedure , i've done changes , should i just commit with another name and push like first time ?

@Souigetsu Souigetsu reopened this Mar 20, 2016
@Ghabry
Member
Ghabry commented Mar 20, 2016

Just commit to the branch you used to create this pull request and push. Then the commit will be added to this PR.

PR are improving over time, so this is fine.

AMIRI bachir Enable disable audio in app settings
6371b12
@Souigetsu

Thank you ! hope this one will fit !

@carstene1ns carstene1ns and 1 other commented on an outdated diff Mar 20, 2016
.../android/src/org/easyrpg/player/SettingsActivity.java
@@ -32,6 +32,7 @@
import android.widget.SeekBar;
import android.widget.SeekBar.OnSeekBarChangeListener;
import android.widget.TextView;
+import android.media.MediaPlayer;
@carstene1ns
carstene1ns Mar 20, 2016 Member

Is this one needed? Seems like a leftover.

AMIRI bachir Enable disable audio in app settings _clean
9066f8f
@Ghabry
Member
Ghabry commented Mar 20, 2016

Jenkins: Test this please

@Ghabry Ghabry merged commit 7c7edc8 into EasyRPG:master Mar 20, 2016

5 checks passed

Android Build finished.
Details
Linux Build finished.
Details
OSX Build finished.
Details
Windows Build finished.
Details
web Build finished.
Details
@Ghabry Ghabry modified the milestone: 0.4.2 Apr 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment