-
Notifications
You must be signed in to change notification settings - Fork 369
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
Feature app-defined language implementation #1334
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments in the code with some nits line-by-line:
Also a number of tests are failing due to null
errors:
- Missing LanguageContext for IAMs in tests. https://travis-ci.com/github/OneSignal/OneSignal-Android-SDK/builds/226256046#L3605-L3611
- Some notification channel tests failing. https://travis-ci.com/github/OneSignal/OneSignal-Android-SDK/builds/226256046#L1845-L1850
Reviewed 12 of 12 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @emawby and @tanaynigam)
OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignal.java, line 1694 at r1 (raw file):
OneSignalStateSynchronizer.updateDeviceInfo(deviceInfo); } catch (JSONException exception) { String operation = language.equals("") ? "remove" : "set";
We can omit this operation
logic as you can only set a language, not remove it.
OneSignalSDK/onesignal/src/main/java/com/onesignal/language/LanguageContext.java, line 19 at r1 (raw file):
public LanguageContext(OSSharedPreferences preferences) { instance = this; if ( preferences.getString(
nit, remove extra space before preferences
.
OneSignalSDK/onesignal/src/main/java/com/onesignal/language/LanguageProviderAppDefined.java, line 4 at r1 (raw file):
import com.onesignal.OSSharedPreferences; public class LanguageProviderAppDefined implements LanguageProvider{
nit, should be space after LanguageProvider
OneSignalSDK/onesignal/src/main/java/com/onesignal/language/LanguageProviderDevice.java, line 5 at r1 (raw file):
import java.util.Locale; public class LanguageProviderDevice implements LanguageProvider{
nit, should be space after LanguageProvider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @emawby)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @tanaynigam)
OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignal.java, line 1677 at r2 (raw file):
} if (shouldLogUserPrivacyConsentErrorMessageForMethodName("setLanguage()"))
Instead of "setLanguage()" string, what about using the constant OSTaskRemoteController.SET_Language?
OneSignalSDK/onesignal/src/main/java/com/onesignal/language/LanguageContext.java, line 19 at r2 (raw file):
public LanguageContext(OSSharedPreferences preferences) { instance = this; if (preferences.getString(
For easier readability, would you mind putting the if condition inside a variable?
OneSignalSDK/onesignal/src/main/java/com/onesignal/language/LanguageProviderAppDefined.java, line 21 at r2 (raw file):
public String getLanguage() { return preferences.getString( preferences.getPreferencesName(), PREFS_OS_LANGUAGE, "en");
Code styling comment, would you mind moving the "en" string to a class constant string?
OneSignalSDK/onesignal/src/main/java/com/onesignal/language/LanguageProviderDevice.java, line 10 at r2 (raw file):
// https://github.com/OneSignal/OneSignal-Android-SDK/issues/64 if (lang.equals("iw"))
would it make sense to have enums instead of strings?
* Added Language Context Interface for getter and setter for Language Provider * Added classes Language Provider, LanguageProviderAppDefined and LanguageProviderDevice * The interface retrieves the default language set by device if a language override is not set
* Fixed the null check in the LanguageContext class * Added getLanguage method to retrieve the language from strategy
* added network call to setLanguage method in OneSignal class * Updated device info to retrieve language from LanguageContext class * Added SET_LANGUAGE to OSTaskController String Array to execute task delays
* Added a Unit test to test setLanguage method executing during the initialization of OneSignal library * Added a Unit test to test setLanguage method executing after the OneSignal library has been initialized * Added a Unit test to test setLanguage method executing after a an app has been restarted and new session has been initialized
* Moved the implementation of OSUtils.getCorrectedLanguage to LanguageProviderDevice.getLanguage
* Made OneSignal.preferences public for accessibility in Language classes * Moved callbacks for OneSignalPrefs from LanguageContext and LanguageProviderAppDefined classes to OneSignal.preferences
* Fix OSInAppMessageController variantIdForMessage retrieves languageIndentifier from OneSignal languageContext * Fix NotificationChannelManager CreateChannel retrieves deviceLanguage from OneSignal languageContext
* Add Singleton pattern to LanguageContext class * Add OSSharedPreference dependency to LanguageProviderAppDefined constructor * Add OSSharedPreference dependency to LanguageContext constructor
* Add LanguageContext dependency to OSInAppMessageController constructor * Add LanguageContext dependency to OSInAppMessageControllerFactory constructor * Add LanguageContext dependency to OSInAppMessageDummyController * Rename languageIdentifier to language in OSInAppMessageController variantIdentifier
* Update NotificationChannelManager createChannel to use LanguageContext instance * Rename deviceLanguage variable to language for consistency
* Convert OneSignal preferences and OneSignal languageContext from public to private since they are being called through dependency injection
* Fix format space LanguageContext class * Fix format space LanguageProviderAppDefined class * Fix format space LanguageProviderDevice class
* Remove unnecessary null check at OneSignal.setLanguage * Remove error logging at JSONException catch at OneSignal.setLanguage
…Language * Add OneSignal.initWithContext to NotificationChannelManagerRunner.processChannelListWithMultiLanguage to allow languageContext to initialize before using languageContext.getLanguage
* Add NonNull annotation to getLanguage implementations in LanguageProviderDevice, LanguageProviderAppDefined and LanguageContext classes
* Move "en" string to constant in LanguageProviderAppDefined class * Move Corrected Language Strings to constant in LanguageProviderDevice class * Add OSTaskRemoteController.SET_LANGUAGE in place of setLanguage() string in OneSignal.setLanguage
* Move preferences.getString to variable instead of if statement for clarity in LanguageContext constructor
89300fd
to
5b4d1a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 7 of 7 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @tanaynigam)
Description
Line Summary
Add feature app-defined language implementation. A user can define their own language setting to be used by OneSignal-SDK
Details
LanguageContext
,LanguageProviderAppDefined
andLanguageProviderDevice
classLanguageContext
uses strategy pattern to retrieve either app-defined or device languageLanguageProviderAppDefined
retrieves app-defined languageLanguageProviderDevice
retrieves device languageLanguageProvider
interface that defines method getLanguageOneSignal.setLanguage
method that saves app-defined language toOneSignal.preferences
OSUtils.getCorrectedLanguage
methodLanguageProviderDevice.getLanguage
method with implementation fromOSUtils.getCorrectedLanguage
OSSharedPreferences
dependency injection toLanguageContext
andLanguageProviderAppDefined
constructorsOneSignal.preferences
intoLanguageContext
initializationLanguageContext
dependency injection inOSInAppMessageController
,OSInAppMessageControllerFactory
andOSInAppMessageDummyController
constructorsLanguageContext.getLanguage
toOSInAppMessageController.variantIdForMessage
LanguageContext
classgetLanguage
method toNotificationChannelManager.createChannel
usingLanguageContext
instanceTest
MainOneSignalClassRunner
testSetLanguageOnPlayerCreate
,testSetLanguagePUTRequest
,testSetLanguageOnSession
unit testsOneSignal.setLanguage
to set it to language different from language defined by the device. OneSignal app dashboard reflects the language set usingOneSignal.setLanguage
This change is