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

VBLOCKS-2485: Changes needed due to refactor of react native sdk #129

Merged
merged 13 commits into from
Jan 9, 2024

Conversation

afalls-twilio
Copy link
Contributor

@afalls-twilio afalls-twilio commented Dec 13, 2023

Submission Checklist

  • The README.md reflects new features
  • The CHANGELOG.md reflects any feature, bug fixes, or known issues made in the source code
  • New unit tests and integration tests have beed added
  • Code coverage is more or equal to the previous coverage
  • A visual inspection of the Files changed tab was made prior to submitting the pull request ensuring the style guide was followed
  • CI pipeline is green
  • Included screenshot if necessary

Description

Updated to reflect changes in react native SDK

Breakdown

  • Now it uses both the Application and Activity proxy encapsulating functionality.
  • Now permissions come from the SDK and do not need to be 'copied' over into the AndroidManifest.xml
  • Now uses same version of NDK as the Voice Android SDK

Validation

  • Ran locally, got as far as I could
  • Ran CI test cases

Additional Notes

None

Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

@afalls-twilio afalls-twilio changed the title WIP: Changes needed due to refactor of react native sdk Changes needed due to refactor of react native sdk Dec 13, 2023
}
}

private final VoiceActivityProxy activityProxy = new VoiceActivityProxy(
Copy link
Contributor

Choose a reason for hiding this comment

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

Clarifying Question: Will users who just use the SDK also need to make these changes in their React Native Apps?
I.E: users not using the reference app, but using the sdk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but we are still in beta, and it will make customers lives easier down the line... because we abstract much of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like these changes will be included in beta.4, could we have a guide to updating to beta.4 for users who are on beta.3 and below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if (Manifest.permission.RECORD_AUDIO.equals(permission)) {
Toast.makeText(
MainActivity.this,
"Microphone permissions needed. Please allow in your application settings.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Android doesn't automatically give these prompts?

Copy link
Contributor Author

@afalls-twilio afalls-twilio Dec 13, 2023

Choose a reason for hiding this comment

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

nope, it only places a system pop-up when you request permissions.... but they can always disable it later from the system menus.

import com.microsoft.appcenter.AppCenter;
import com.microsoft.appcenter.distribute.Distribute;
import com.twiliovoicereactnativereferenceapp.newarchitecture.MainApplicationReactNativeHost;

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove extra line

AppCenter.start(this, BuildConfig.APPCENTER_APP_KEY, Distribute.class);
@Override
public void onTerminate() {
// Note: this method is not called when running on device, devies just kill the process.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is devies a typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch!

@@ -25,7 +19,7 @@ buildscript {
classpath("com.facebook.react:react-native-gradle-plugin")
classpath("de.undercouch:gradle-download-task:5.0.1")
classpath("org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlinVersion")
classpath("com.google.gms:google-services:4.3.15")
classpath("com.google.gms:google-services:4.3.0")
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to downgrade from 4.3.15 to 4.3.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, let me test.... I believe I did that to limit issues with overlapping dependencies...

@afalls-twilio afalls-twilio requested a review from kpchoy December 14, 2023 00:14
@afalls-twilio afalls-twilio changed the title Changes needed due to refactor of react native sdk VBLOCKS-2485: Changes needed due to refactor of react native sdk Jan 8, 2024
@afalls-twilio afalls-twilio merged commit 86aea37 into main Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants