-
Notifications
You must be signed in to change notification settings - Fork 32
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
Revert onCreate changes #116
Conversation
|
||
class MainApplication: FlutterApplication() { | ||
import androidx.annotation.CallSuper; |
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.
Is this used?
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.
I'll remove this!
public class MainActivity extends FlutterActivity { | ||
|
||
@Override | ||
public void configureFlutterEngine(@NonNull FlutterEngine flutterEngine) { |
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.
What is this doing?
It looks like the suspect change we're partially rolling back did a brain-hurting rename of Main{Activity,Application} files across each other, but the original code had this on the application not activity (because the files swapped places!), but removed it. I'm not sure that's related to the SDK initialization change? The file doc/get-started.md suggests it's for back-compatibility to Flutter less than 1.12, which---I guess?---we no longer support?
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.
I think (and this is stretching my knowledge on Android) that plugins don't necessarily have access to the main activity, so configureFlutterEngine and registering the plugin so the methods are available on the activity with registerWith
is what this piece of code is doing. However, I'm looking at the docs and I see that on Flutter 2+ we should do this differently? https://docs.flutter.dev/release/breaking-changes/plugin-api-migration
Per that page:
public class MainActivity extends FlutterActivity {
// You can keep this empty class or remove it. Plugins on the new embedding
// now automatically registers plugins.
}
364df91
to
b7dc16e
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.
At least yesterday, CI was red... but as I read this code, it looks good. I'd nag @Nilay-squareup to see if he approves or knows some Flutter thing we're missing. But if we don't hear in another 24hrs (i.e. another Indian day shift) and the CI is green, I'm good with this.
@Nilay-squareup Flutter is sort of outside my domain, so the failing CI is bizarre to me since I didn't change files related to those library files. I looked around and the suggested fix is to bump the Flutter version? |
b7dc16e
to
e52cce7
Compare
@WhatsEmo The CI is failing because some of the properties used in styles have been deprecated in recent version of flutter, And I already have fixed this issue in My PR which is not yet merged. and regarding the code I have seen that you have placed java code in cc: @fka3 |
Revert ReaderSdk.initialize changes in TatsuUkraine:flutter-3. This change causes the application to crash whenever the app is background and the app has been initialized before. Bump version
7a7a362
to
f161196
Compare
Summary
Revert ReaderSdk.initialize changes in TatsuUkraine:flutter-3. This change causes the application to crash whenever the app is background and the app has been initialized before. This is a fix for an on-going issue a developer is facing.
Related issues
Fix #
Changelog
SquareReaderSdkFlutterPlugin.java
)Test Plan
Manual tests will be preformed to test these changes