-
Notifications
You must be signed in to change notification settings - Fork 9.7k
Continues removing the assumption that there is a foreground activity #309
Conversation
5a3a346 to
56d76a2
Compare
mravn-google
left a comment
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.
LGTM
| } | ||
|
|
||
| private final Context context; | ||
| private final PluginRegistry.Registrar mRegistrar; |
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.
The mMember naming convention is not used in the existing code. I think we should either keep the current coding style, or apply the m prefix to all members.
Here and elsewhere.
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.
Changed to match existing style in each file.
| final EventChannel eventChannel = | ||
| new EventChannel(registrar.messenger(), "plugins.flutter.io/charging"); | ||
| final BatteryPlugin instance = new BatteryPlugin(registrar.context()); | ||
| final BatteryPlugin instance = new BatteryPlugin(registrar); |
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.
AFAIK, registrar.context() constantly returns the same value. If that is correct, this change isn't strictly necessary. Are we doing it anyway for consistency with plugins that need the registrar to access the current Activity?
Same in other plugins that just need a Context, and not an Activity.
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.
Yes, it's for consistency.
|
|
||
| Activity activity = mRegistrar.activity(); | ||
| if (activity == null) { | ||
| result.error("NO_ACTIVITY", "image_picker plugin requires a foreground activity.", null); |
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.
You've used "no_activity" in all other places in this PR.
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.
Changed to no_activity here
56d76a2 to
ce87583
Compare
ce87583 to
79c24b7
Compare
No description provided.