Skip to content

Add Simplified Initialize support#129

Merged
cacheung merged 14 commits intoadobe:dev-v5.0.0from
cacheung:simplifed
Feb 21, 2025
Merged

Add Simplified Initialize support#129
cacheung merged 14 commits intoadobe:dev-v5.0.0from
cacheung:simplifed

Conversation

@cacheung
Copy link
Contributor

@cacheung cacheung commented Feb 14, 2025

  • Flutter
    • Add MobileCore.initialize(initOptions: options);
      Add MobileCore.initialize(appId: "Your_APP_ID")
    • Add initOptions class
  • Implement the wrapper for Android and iOS
  • Update the Sample App to use this API

TODO:

  • Add tests

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Add Initialize API support for TS and Android
update main.dart
Update logging to trace
@cacheung cacheung requested a review from praveek February 14, 2025 00:30
Add MobileCore.initialize(appId: "Your_APP_ID") API
class InitOptions {
String? appId;
String? filePath;
bool lifecycleAutomaticTracking;
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this optional and avoid setting a default value. This allows the wrapper to defer to the default value provided by the underlying platform.

@@ -0,0 +1,29 @@
class InitOptions {
String? appId;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add some comments for each property here?

this.lifecycleAutomaticTracking = true, // Default value set to true
this.lifecycleAdditionalContextData = null, // Default value set to null
this.appGroupIOS = null, // Default value set to null
}) : assert(appId != null || filePath != null,
Copy link
Contributor

Choose a reason for hiding this comment

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

If neither value is set, the SDK will default to the bundled configuration.
You should assert that both values are not set at the same time.

/// @param initOptions The [InitOptions] to configure the SDK.
/// @param appId The AEP SDK App ID.
/// @throws [ArgumentError] if neither `initOptions` nor `appId` is provided, or if both
static Future<void> initialize({InitOptions? initOptions, String? appId}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Split it into two APIs:
initialize()
initializeWithAppId()
For initializeWithAppId(), create an InitOptions object using the provided appId, and then call the initialize() API.

result.success(MobileCore.extensionVersion());
} else if ("initialize".equals(call.method)) {
handleInitialize(result, call.arguments);
} else if ("initializeWithAppId".equals(call.method)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove this binding.


private void handleInitialize(Result result, Object arguments) {
if (!(arguments instanceof Map)) {
result.error("INVALID_ARGUMENT", "Initialize failed because arguments is not a Map", null);
Copy link
Contributor

@praveek praveek Feb 14, 2025

Choose a reason for hiding this comment

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

Define INVALID_ARGUMENT and other strings as constants.

return;
}

if (application == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You do this check in Line 113. Can you remove this?

return;
}

Map initOptionsMap = (Map) arguments;
Copy link
Contributor

Choose a reason for hiding this comment

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

Perform a safe cast.

if (arguments instanceof Map) {
  Map initOptionsMap = (Map) arguments;
}

I also recommend moving this logic to FlutterAEPCoreDataBridge.initOptionsFromMap()

Incorporate review comments
/// The file path for the local configuration file
String? filePath;
/// Flag indicating whether automatic lifecycle tracking is enabled
bool? lifecycleAutomaticTracking;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you rename this to lifecycleAutomaticTrackingEnabled to make it consistent with Android and iOS?

appGroupIOS: "group.com.example",
);

await MobileCore.initialize(initOptions: initOptions);
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 incorrect, as it will delay the app load until the SDK initializes. Could you move this to a more relevant place?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to move this to the initState() function of the HomePage class. See my example in the spec.
https://wiki.corp.adobe.com/pages/viewpage.action?spaceKey=adms&title=004+%3A+SDK+Initialization+Simplification#id-004:SDKInitializationSimplification-Wrappers

Add iOS implementation for Simplified initialization
@cacheung cacheung requested a review from kevinlind February 19, 2025 00:21
} else if (filePath != null) {
options = InitOptions.configureWithFileInPath(filePath);
} else {
return null; // Either appId or filePath must be provided
Copy link
Contributor

@praveek praveek Feb 19, 2025

Choose a reason for hiding this comment

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

Don't return here. Instead use options = InitOptions() and remove the comment.
If both appID and filePath are absent, we use bundled config.

@cacheung cacheung changed the title Add Simplified Initialize support for Android Add Simplified Initialize support Feb 19, 2025
return AEP_PRIVACY_STATUS_UNKNOWN;
}

static InitOptions initOptionsFromMap(final Object option) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can you fix the variable names.
option -> arguments
optionsAsMap -> argumentsAsMap
initOptionMap -> initOptionsMap

} else {
return null; // Either appId or filePath must be provided
}
options.setLifecycleAutomaticTrackingEnabled(lifecycleAutomaticTrackingEnabled != null ? lifecycleAutomaticTrackingEnabled : true);
Copy link
Contributor

Choose a reason for hiding this comment

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

if (lifecycleAutomaticTrackingEnabled != null) {
   options.setLifecycleAutomaticTrackingEnabled(lifecycleAutomaticTrackingEnabled)
}

String appId = getNullableString(initOptionMap, APPID_KEY);
String filePath = getNullableString(initOptionMap, FILE_PATH_KEY);
Boolean lifecycleAutomaticTrackingEnabled = getNullableBoolean(initOptionMap, LIFECYCLE_AUTOMATION_TRACKING_KEY);
Map<String, String> lifecycleAdditionalContextData = getNullableMap(initOptionMap, LIFECYCLE_ADDITIONAL_CONTEXTDATA_KEY);
Copy link
Contributor

@praveek praveek Feb 19, 2025

Choose a reason for hiding this comment

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

This cast is incorrect. Can you add getNullableStringMap() which verifies the keys and values are all String?

public void onAttachedToEngine(@NonNull final FlutterPluginBinding binding) {
channel = new MethodChannel(binding.getBinaryMessenger(), "flutter_aepcore");
channel.setMethodCallHandler(new FlutterAEPCorePlugin());
application = (Application) binding.getApplicationContext();
Copy link
Contributor

Choose a reason for hiding this comment

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

Context appContext = binding.getApplicationContext()
if (appContext instanceof Application) {
application = (Application) appContext;
}

result([AEPMobileCore extensionVersion]);
} else if ([@"initialize" isEqualToString:call.method]) {
[self handleInitialize:call result:result];
result(nil);
Copy link
Contributor

@praveek praveek Feb 19, 2025

Choose a reason for hiding this comment

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

result(nil) should be called from inside the callback passed to initialize API.

AEPInitOptions *initOptions = [FlutterAEPCoreDataBridge initOptionsFromMap:arguments];
if (!initOptions) {
result([FlutterError errorWithCode:@"INVALID_ARGUMENT"
message:@"Initialize failed because initOptions is not a dictionary"
Copy link
Contributor

Choose a reason for hiding this comment

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

Initialize failed because InitOptions is not valid.

InitOptions({
this.appId,
this.filePath,
this.lifecycleAutomaticTrackingEnabled = 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.lifecycleAutomaticTrackingEnabled = null

this.lifecycleAutomaticTrackingEnabled = true,
this.lifecycleAdditionalContextData = null,
this.appGroup = null,
}) : assert(!(appId != null && filePath != null),
Copy link
Contributor

@praveek praveek Feb 19, 2025

Choose a reason for hiding this comment

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

Can you change your assertion to appId == null || filePath == null?

/// Additional context data to be included in lifecycle start event.
Map<String, String>? lifecycleAdditionalContextData;
/// App group used to share user defaults and files among containing app and extension apps on iOS
String? appGroup;
Copy link
Contributor

Choose a reason for hiding this comment

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

appGroupIOS

retMap['filePath'] = filePath;
retMap['lifecycleAutomaticTrackingEnabled'] = lifecycleAutomaticTrackingEnabled;
retMap['lifecycleAdditionalContextData'] = lifecycleAdditionalContextData;
retMap['appGroup'] = appGroup;
Copy link
Contributor

Choose a reason for hiding this comment

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

appGroupIOS

Incorporate review comments
update format
Update for the missing import for context
Update for the null and isKindOfClass check for appId on iOS
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file still needed? Can you remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not able to remove AppDelegate files with error for the example app. I think we can keep it here. I will test with a brand new app to make sure the initialize API works there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you able to remove the AppDelegate altogether?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

appGroupIOS: "group.com.example",
);

await MobileCore.initialize(initOptions: initOptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to move this to the initState() function of the HomePage class. See my example in the spec.
https://wiki.corp.adobe.com/pages/viewpage.action?spaceKey=adms&title=004+%3A+SDK+Initialization+Simplification#id-004:SDKInitializationSimplification-Wrappers


private final static String APPID_KEY = "appId";
private final static String FILE_PATH_KEY = "filePath";
private final static String LIFECYCLE_AUTOMATION_TRACKING_KEY = "lifecycleAutomaticTrackingEnabled";
Copy link
Contributor

Choose a reason for hiding this comment

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

"AUTOMATIC"

Fixing typo
Remove filePath parameter in initialize API
}

Map rawMap = (Map) data.get(key);
Map<String, String> stringMap = new HashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't create a new map. Instead, after validation, your return value on line 186 should be (Map<String, String>) rawMap.

Context appContext = binding.getApplicationContext();
if (appContext instanceof Application) {
application = (Application) appContext;
}
Copy link
Contributor

@praveek praveek Feb 20, 2025

Choose a reason for hiding this comment

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

Can you also add a call to MobileCore.setWrapperType(WrapperType.Flutter) here?
Additionally, include the same API call within the registerWithRegistrar method of your iOS wrapper.

…per Type Flutter for Android and iOS.

Update SDK initialize in initState() in the example app, add AEP Wrapper Type Flutter for Android and iOS. Fix return map in Core DataBridge Java file.
NSString *appGroup = initOptionsMap[@"appGroup"];

AEPInitOptions *initOptions;
if (appId != nil && ![appId isKindOfClass:[NSNull class]]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of testing if appId is not NSNull class, can you check if it is NSString class?
if (appId != nil && [appId isKindOfClass:[NSString class]])

if (lifecycleAdditionalContextData != nil && [lifecycleAdditionalContextData isKindOfClass:[NSDictionary class]]) {
initOptions.lifecycleAdditionalContextData = lifecycleAdditionalContextData;
} else {
NSLog(@"lifecycleAdditionalContextData is nil or not a dictionary");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend removing this log entry as it's probably more common to not set additional context data. Plus, the Android implementation doesn't log here either.

if (appGroup != nil && [appGroup isKindOfClass:[NSString class]]) {
initOptions.appGroup = appGroup;
} else {
NSLog(@"appGroup is nil or not a string");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend removing this log entry as it's probably more common to not set an app group.

Update AEPCoreDataBridge
@cacheung cacheung merged commit 6ecc70d into adobe:dev-v5.0.0 Feb 21, 2025
2 checks passed
@cacheung cacheung deleted the simplifed branch February 21, 2025 01:01
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.

3 participants