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.

Sign up for GitHub

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

Jump to bottom

TIMOB-16080: Added New sound property for views #8852

Merged
merged 2 commits into from Mar 7, 2017
Merged

TIMOB-16080: Added New sound property for views #8852

merged 2 commits into from Mar 7, 2017

Conversation

antw12
Copy link
Contributor

@antw12 antw12 commented Feb 28, 2017 •
edited

JIRA:
https://jira.appcelerator.org/browse/TIMOB-16080

Description:
Added a new property PROPERTY_TOUCH_SOUND_ENABLED allowing a user to set sound effects for views on or off.

Default for property is true

Test Case

var win = Ti.UI.createWindow({
    backgroundColor: 'black',
});

var view = Ti.UI.createView({
    soundEffectsEnabled : false,
    backgroundColor : "Black"
});
win.add(view);
win.open();

hansemannn
hansemannn requested changes Feb 28, 2017
View reviewed changes
android/titanium/src/java/org/appcelerator/titanium/TiC.java Outdated
@@ -2746,6 +2746,11 @@
/**
* @module.api
*/
public static final String PROPERTY_TOUCH_SOUND_ENABLED = "soundEnabled";
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should name it the same (soundEffectsEnabled), which would be mapped to PROPERTY_SOUND_EFFECTS_ENABLED.

android/titanium/src/java/org/appcelerator/titanium/view/TiUIView.java Outdated
@@ -1726,8 +1726,14 @@ protected void registerForTouch(final View touchable)
touchable.setEnabled(false);
}
}
//Checking and setting touch sound for view
if (proxy.hasProperty(TiC.PROPERTY_TOUCH_SOUND_ENABLED)) {
boolean soundEnabled = TiConvert.toBoolean(proxy.getProperty(TiC.PROPERTY_TOUCH_SOUND_ENABLED), true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indentation incorrect.

android/titanium/src/java/org/appcelerator/titanium/view/TiUIView.java Outdated
//Checking and setting touch sound for view
if (proxy.hasProperty(TiC.PROPERTY_TOUCH_SOUND_ENABLED)) {
boolean soundEnabled = TiConvert.toBoolean(proxy.getProperty(TiC.PROPERTY_TOUCH_SOUND_ENABLED), true);
if (!soundEnabled) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is false first, setting it to true will skip every time. Rather call the setter directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is set to true as default as that is the native behavior, I tested it and it works so i am not sure, if I am still wrong please let me know

garymathews
garymathews suggested changes Mar 1, 2017
View reviewed changes
android/titanium/src/java/org/appcelerator/titanium/view/TiUIView.java Outdated
if (proxy.hasProperty(TiC.PROPERTY_TOUCH_ENABLED)) {
boolean enabled = TiConvert.toBoolean(proxy.getProperty(TiC.PROPERTY_TOUCH_ENABLED), true);
if (!enabled) {
touchable.setEnabled(false);
}
}
//Checking and setting touch sound for view
if (proxy.hasProperty(TiC.PROPERTY_SOUND_EFFECTS_ENABLED)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (proxy.hasProperty(TiC.PROPERTY_SOUND_EFFECTS_ENABLED)) {
	boolean soundEnabled = TiConvert.toBoolean(proxy.getProperty(TiC.PROPERTY_SOUND_EFFECTS_ENABLED), true);
	touchable.setSoundEffectsEnabled(soundEnabled);
}

garymathews
garymathews reviewed Mar 1, 2017
View reviewed changes
android/titanium/src/java/org/appcelerator/titanium/proxy/TiViewProxy.java
@@ -116,6 +116,7 @@ public TiViewProxy()
pendingAnimationLock = new Object();

defaultValues.put(TiC.PROPERTY_TOUCH_ENABLED, true);
defaultValues.put(TiC.PROPERTY_SOUND_EFFECTS_ENABLED, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is necessarily needed since it's already enabled by default

@antw12
Copy link
Contributor Author

antw12 commented Mar 1, 2017

for the PROPERTY_TOUCH_ENABLED should i change it to match, as it is set up the same with default as true?

@garymathews
Copy link
Contributor

@antw12 Yes, as that will not allow you to set touchEnabled back to true. Nice catch 👍

@garymathews
Copy link
Contributor

CR: PASS (note: try not to change the formatting of code you are not changing)
FT: PASS

@ssjsamir ssjsamir self-requested a review March 7, 2017 18:07
@ssjsamir
Copy link
Contributor

ssjsamir commented Mar 7, 2017

FR Passed, was able to set soundEffectsEnabled to true or false and can confirm if it was set to to true sound effects can be heard and if set to false sound effects can not be heard.

Steps I took to test:

  • Copy the code provided in the description
  • Ran the program
  • Was not able to hear the sound effects
  • Edited the code and set soundEffectsEnabled to true
  • Was able to hear the sound effects

Environment

Appcelerator Command-Line Interface, version 6.1.0
Google Pixel XL (Android 7.1.1)
Operating System Name: Mac OS X El Capitan
Operating System Version: 10.11.6
Node.js Version: 4.6.0
npm: 4.2.8
Xcode: 8.2
Appcelerator Studio: 4.8.1.201612050850

@ssjsamir ssjsamir merged commit 3e99251 into tidev:master Mar 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garymathews garymathews garymathews requested changes

@hansemannn hansemannn hansemannn requested changes

@ssjsamir ssjsamir Awaiting requested review from ssjsamir

Assignees
No one assigned
Labels
android bug in-qe-testing 🕵
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@antw12 @garymathews @ssjsamir @hansemannn