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

feature/scalable-integration - Add dynamic and scalable segment integration #18

Closed
wants to merge 7 commits into from
Closed

feature/scalable-integration - Add dynamic and scalable segment integration #18

wants to merge 7 commits into from

Conversation

ariefwijaya
Copy link
Contributor

Purpose

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (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 not work as expected)
  • Documentation Update (if none of the other choices apply)

How to Test

  • Use plugin and run your apps, as usual using the new parameter

Environment

Please define your environment to test this, you can choose multiple options. Put an x in the boxes that apply

  • Production
  • Development
  • Testing
  • Android
  • iOS
Attach your `flutter doctor -v` inside code block:

[✓] Flutter (Channel stable, 2.5.3, on macOS 12.2 21D49 darwin-x64, locale en-ID)
    • Flutter version 2.5.3 at /Users/ittcel/fvm/versions/stable
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision 18116933e7 (5 months ago), 2021-10-15 10:46:35 -0700
    • Engine revision d3ea636dc5
    • Dart version 2.14.4

[✓] Android toolchain - develop for Android devices (Android SDK version 31.0.0)
    • Android SDK at /Users/ittcel/Library/Android/sdk
    • Platform android-31, build-tools 31.0.0
    • Java binary at: /Applications/Android Studio.app/Contents/jre/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 11.0.11+0-b60-7590822)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Xcode 13.2.1, Build version 13C100
    • CocoaPods version 1.11.2

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 2021.1)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build 11.0.11+0-b60-7590822)

[✓] IntelliJ IDEA Community Edition (version 2020.3.4)
    • IntelliJ at /Applications/IntelliJ IDEA CE.app
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart

[✓] VS Code (version 1.65.0)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.36.0

[✓] Connected device (2 available)
    • Android SDK built for x86 64 (mobile) • emulator-5554 • android-x64    • Android 9 (API 28) (emulator)
    • Chrome (web)                          • chrome        • web-javascript • Google Chrome 98.0.4758.109

• No issues found!

Expected Results

  • Plugin working properly

Notes

  • tested and works 100% in android & ios.

@ariefwijaya
Copy link
Contributor Author

ariefwijaya commented Mar 16, 2022

Hi, @danielgomezrico @MaiKaY any update on this?
I've planned to add more integration such as firebase, google analytics, facebook, and so on

Comment on lines -57 to -68

try {
ApplicationInfo ai = applicationContext.getPackageManager()
.getApplicationInfo(applicationContext.getPackageName(), PackageManager.GET_META_DATA);

Bundle bundle = ai.metaData;

FlutterSegmentOptions options = FlutterSegmentOptions.create(bundle);
setupChannels(options);
} catch (Exception e) {
Log.e("FlutterSegment", e.getMessage());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Whats the idea behind this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need this code anymore since these are deprecated:
image

and also in iOS side.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, then it does not relate to this PR right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danielgomezrico I think it's related because it is a breaking change since we can't use an array in the android manifest or .plist. So, We need to remove it then.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I dont understand why this is related to this, can you try to explain it again please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. The old implementation (Integration using AndroidManifest.xml and ios info.plist) is deprecated.
  2. what I propose here is a new method that will not be compatible with the old implementation. Breaking change.

@danielgomezrico ,

Copy link

Choose a reason for hiding this comment

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

  1. The old implementation (Integration using AndroidManifest.xml and ios info.plist) is deprecated.

    1. what I propose here is a new method that will not be compatible with the old implementation. Breaking change.

@danielgomezrico ,

@ariefwijaya there is another issue open where initializing the sdk using the dart initialization results in App Installed events not being tracked on iOS:
#26

So while the old implementation is marked as deprecated, we had to revert back to it and rely on it for our iOS integrations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh Thanks for the info, but in my case, I don't get that problem and not related to my implementation.
Because, I just improve our existing dart only installation..
You can continue the discussion about Installed Events not being tracked in this issue #26

@danielgomezrico
Copy link
Contributor

@ariefwijaya thanks for this! Please take a look at my questions here and in the discussion

@ariefwijaya
Copy link
Contributor Author

@ariefwijaya thanks for this! Please take a look at my questions here and in the discussion

@danielgomezrico Thanks for the reply. look forward to hearing your feedback.

@ariefwijaya
Copy link
Contributor Author

ariefwijaya commented Mar 29, 2022

@danielgomezrico @MaiKaY any feedbacks guys?

@danielgomezrico
Copy link
Contributor

@ariefwijaya since the discussion is not already answered I would continue adding more code, it is better to think a solution a then implement it, there are still some unknowns in the discussion

@ariefwijaya
Copy link
Contributor Author

@ariefwijaya since the discussion is not already answered I would continue adding more code, it is better to think a solution a then implement it, there are still some unknowns in the discussion

I've replied to your question already, have you checked it? @danielgomezrico

@julianfalcionelli
Copy link

No updates on this? Is great having the integration settings to be dynamic and not just supporting Firebase/Amplitude...

@sribuu sribuu closed this by deleting the head repository Nov 7, 2023
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.

[DISCUSSION][Tech Debt] - Fully Dynamic and Scalable Segment Integration
7 participants