feat(connectivity)!: support multiple ConnectivityResult values at the same time #2599

suquant · 2024-02-15T20:09:53Z

Description

This PR updates the connectivity_plus plugin to handle multiple connectivity types simultaneously. Previously, checkConnectivity and onConnectivityChanged methods returned a single ConnectivityResult type, which could be either wifi, cellular, ethernet, bluetooth, vpn, or none. Due to this, on iOS/macOS platforms, the types other & vpn were never returned because the presence of wifi or cellular took precedence. This update changes the return type of these methods to a list of ConnectivityResult, allowing for a more accurate representation of the device's connectivity status, especially in scenarios where multiple types of connections are available or in use simultaneously.

Related Issues

Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I titled the PR using Conventional Commits.
I did not modify the CHANGELOG.md nor the plugin version in pubspec.yaml files.
All existing and new tests are passing.
The analyzer (flutter analyze) does not report any problems on my PR.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate that with a ! in the title as explained in Conventional Commits).
No, this is not a breaking change.

Users of the connectivity_plus plugin will need to update their apps to handle the new list of ConnectivityResult types returned by checkConnectivity and onConnectivityChanged methods. This change allows for a more comprehensive understanding of the device's connectivity status, particularly in environments where multiple connectivity options are available or in use.

miquelbeltran

Thanks a lot for the contribution, I think this is a good start for a change that will fix several issues.

First, I'd like to ask you to properly fill the PR template, including the different checkboxes: https://github.com/fluttercommunity/plus_plugins/blob/main/.github/PULL_REQUEST_TEMPLATE.md

We can see CI issues (analytics and tests not building) and those should be fixed as well.

Regarding the code, and before I go and do a more deep review, is there any reason why is preferred to handle the list of connectivity items as a coma-separated string rather than a List of Strings? e.g. in Connectivity.java, the MethodChannel, etc.

suquant · 2024-02-16T15:29:06Z

@miquelbeltran

Regarding the code, and before I go and do a more deep review, is there any reason why is preferred to handle the list of connectivity items as a coma-separated string rather than a List of Strings? e.g. in Connectivity.java, the MethodChannel, etc.

Fair note, agreed!

miquelbeltran · 2024-02-19T06:38:22Z

...ectivity_plus/android/src/main/java/dev/fluttercommunity/plus/connectivity/Connectivity.java

+    List<String> types = new ArrayList<>();
    if (info == null || !info.isConnected()) {
-      return CONNECTIVITY_NONE;
+      types.add(CONNECTIVITY_NONE);
+      return types;
    }
    int type = info.getType();
    switch (type) {
      case ConnectivityManager.TYPE_BLUETOOTH:
-        return CONNECTIVITY_BLUETOOTH;
+        types.add(CONNECTIVITY_BLUETOOTH);
+        break;
      case ConnectivityManager.TYPE_ETHERNET:
-        return CONNECTIVITY_ETHERNET;
+        types.add(CONNECTIVITY_ETHERNET);
+        break;
      case ConnectivityManager.TYPE_WIFI:
      case ConnectivityManager.TYPE_WIMAX:
-        return CONNECTIVITY_WIFI;
+        types.add(CONNECTIVITY_WIFI);
+        break;
      case ConnectivityManager.TYPE_VPN:
-        return CONNECTIVITY_VPN;
+        types.add(CONNECTIVITY_VPN);
+        break;
      case ConnectivityManager.TYPE_MOBILE:
      case ConnectivityManager.TYPE_MOBILE_DUN:
      case ConnectivityManager.TYPE_MOBILE_HIPRI:
-        return CONNECTIVITY_MOBILE;
+        types.add(CONNECTIVITY_MOBILE);
+        break;
      default:
-        return CONNECTIVITY_NONE;
+        types.add(CONNECTIVITY_NONE);


Would it be possible to simplify this by returning a List.of(type)? For what I understand, the array will always have just one item.

I haven't done Java in a long time, so I don't remember if that was possible :)

I thinks it's not so necessary cause return executes straight after switch statement.

what I mean is:

the code is creating an empty array on line 74

then adding a single item in the switch statement

then returning the array at the end after the switch.

And I'd prefer if the code could just call return inside the switch case with a List.of(item). It is less code, and less prone to returning an empty array in case of a future code refactor.

@miquelbeltran Thanks for the suggestion! I aimed to optimize by avoiding extra array creation, but I'm open to your perspective. If you think changing it is best, I'm on board.

...roid/src/main/java/dev/fluttercommunity/plus/connectivity/ConnectivityBroadcastReceiver.java

packages/connectivity_plus/connectivity_plus/ios/Classes/ReachabilityConnectivityProvider.swift

miquelbeltran

Thanks again for the changes!

And btw, thanks for working on the Windows code as well, very few people want to touch that part of the project :)

I have left some comments, I need to also test this so it will take some time until we do the final approval, but so far it looks good.

I'd like to ask you too, that instead of force pushing changes, you can push new commits, so that I way I can track the PR changes since the last time I reviewed.

Thanks!

…e same time BREAKING CHANGES: checkConnectivity and onConnectivityChanged returns list of connectivityResult. Users will need to update their apps to handle the new list of ConnectivityResult types returned by checkConnectivity and onConnectivityChanged methods. Signed-off-by: George Kutsurua <[email protected]>

miquelbeltran · 2024-02-26T20:51:08Z

I'll try to run some tests this week and do a deeper code review. Thanks!

miquelbeltran · 2024-02-27T10:35:16Z

Tested Android (also with VPN), iOS, MacOS, Linux and Chrome. I could not test Windows. Seems to work as expected, except in Linux and Web it did not report VPN correctly, but that's another issue.

miquelbeltran

Good refactor unifying the iOS and MacOS codebases into a single one! We need to check if we can afford increasing the min-flutter sdk version.

I have added some other comments, otherwise it seems to be good.

Please, when you do updates, do not force-push, because if not I lose all the "viewed" history and need to review the whole PR again.

packages/connectivity_plus/connectivity_plus/android/gradle/wrapper/gradle-wrapper.properties

packages/connectivity_plus/connectivity_plus/darwin/Classes/ConnectivityPlugin.m

packages/connectivity_plus/connectivity_plus/darwin/Classes/SwiftConnectivityPlugin.swift

miquelbeltran · 2024-02-27T10:54:52Z

packages/connectivity_plus/connectivity_plus/pubspec.yaml

+  # Flutter versions prior to 3.7 did not support the
+  # sharedDarwinSource option.
+  flutter: ">=3.7.0"


@vbuberen What do you think about increasing the min flutter version in order to share the macos and ios codebases? personally I am ok with it, but want to know your thoughts

I am Ok with that here, even though I don't like this merging of iOS and MacOS into one.

I opted to maintain a single codebase for both platforms, aligning with best practices for Apple's products.

I am in favor of having a single code folder for both platform when possible, although it may not be the case for all plugins.

packages/connectivity_plus/connectivity_plus_platform_interface/lib/src/utils.dart

vbuberen

Thanks for the contribution, but I would ask to revert changes not related to the topic of the PR:

Revert Gradle wrapper version.
Revert naming changes for iOS/MacOS part.

Also, please address the review feedback comments provided by Miguel.

packages/connectivity_plus/connectivity_plus/darwin/Classes/ConnectivityPlugin.m

...ges/connectivity_plus/connectivity_plus/darwin/Classes/PathMonitorConnectivityProvider.swift

vbuberen · 2024-02-28T08:57:41Z

packages/connectivity_plus/connectivity_plus/pubspec.yaml

+  # Flutter versions prior to 3.7 did not support the
+  # sharedDarwinSource option.
+  flutter: ">=3.7.0"


I am Ok with that here, even though I don't like this merging of iOS and MacOS into one.

Signed-off-by: George Kutsurua <[email protected]>

miquelbeltran

LGTM, waiting for Volodymyr to comment

miquelbeltran · 2024-03-06T07:03:20Z

packages/connectivity_plus/connectivity_plus/darwin/connectivity_plus.podspec

+  s.platform = {
+    :ios => '12.0',
+    :osx => '10.14'
+  }
+  s.ios.deployment_target = '12.0'
+  s.osx.deployment_target = '10.14'


Do you know if it is necessary to define both platform and deployment_target? The docs only mention deployment_target: https://docs.flutter.dev/packages-and-plugins/developing-packages#shared-ios-and-macos-implementations

miquelbeltran · 2024-03-06T07:05:11Z

packages/connectivity_plus/connectivity_plus/pubspec.yaml

+  # Flutter versions prior to 3.7 did not support the
+  # sharedDarwinSource option.
+  flutter: ">=3.7.0"


I am in favor of having a single code folder for both platform when possible, although it may not be the case for all plugins.

vbuberen · 2024-03-06T07:50:43Z

I see that the latest podspec comment is addressed as well.
So with that change all looks good to me. Let's merge it.

Thanks a lot for this contribution, George.

We will need to do some updates to our docs, I believe, but let's do not in this PR.

suquant · 2024-03-06T07:54:35Z

If all is good, I'm gonna squash all commits into a single one for better tracking and well organized. Thanks for review guys @miquelbeltran @vbuberen

vbuberen · 2024-03-06T07:55:26Z

If all is good, I'm gonna squash all commits into a single one for better tracking and well organized. Thanks for review guys @miquelbeltran @vbuberen

Don't do it. We squash it ourselves.

pamafe1976 · 2024-03-21T17:13:02Z

Hi, I just tested this update in Android, and the behavior does not seem to be as intended.
I always receive a List with 1 ConnectivityResult.

If there is neither wifi or mobile I get [ConnectivityResult.none]
Then I turn on mobile and I get [ConnectivityResult.mobile] =>OnConnectionChanged called 3 times
Then I turn on wifi and I get [ConnectivityResult.wifi] =>OnConnectionChanged called 4 times
If at this point I turn off mobile, onConnectionChanged is not called.
Then I turn on wifi and I get [ConnectivityResult.none] =>OnConnectionChanged called 1 time

What I expected was

If there is neither wifi or mobile I get [ConnectivityResult.none]
Then I turn on mobile and I get [ConnectivityResult.mobile]
Then I turn on wifi and I get [ConnectivityResult.wifi, ConnectivityResult.mobile]
Then turn off mobile and get [ConnectivityResult.wifi]
Then turn off wifiand get [ConnectivityResult.none]

Of course the callback should be called only 1 time on each change

vbuberen · 2024-03-21T17:28:33Z

You are on Android, right?

If so, check the API documentation as it is documented that on Android having both wifi and mobile returns only wifi (or, to be correct, the one that is used for network activity). For example, if you do the same test with VPN turned on you would see 2 results instead.

As to number of times you get the event - on Android we listen to onCapabilitiesChanged callback from NetworkManager as well to catch all events from network state changes. So it is how this callback works. It is up to you to distinct changes in the stream.

With that being said feel free to open a new issue instead of discussing here, but from what you described it is expected behavior from the plugin and part about currently active states on Android is described in the API documentation as well.

janniklind · 2024-04-15T08:39:04Z

Is there some documentation around for why CONNECTIVITY_NONE is returned in the list?

I would prefer that in case of CONNECTIVITY_NONE then an empty list is returned, i.e. no available connections. It would be a cleaner API, that is more explicit.

Today when CONNECTIVITY_NONE is returned in the list, is it then guaranteed that it will always be the only item in the list?

I am using it like the following now, and I think many users would probably do the same:

bool _isConnected(List<ConnectivityResult> result) {
    return result.where((e) => e != ConnectivityResult.none).isNotEmpty;
}

miquelbeltran · 2024-04-15T09:50:56Z

Today when CONNECTIVITY_NONE is returned in the list, is it then guaranteed that it will always be the only item in the list?

Yes, and that is documented (not sure if this has been published, but it is in main)

/// The returned list is never empty. In case of no connectivity, the list contains
/// a single element of [ConnectivityResult.none]. Note also that this is the only
/// case where [ConnectivityResult.none] is present.

You should be able to safely do result.first != ConnectivityResult.none or similar.

janniklind · 2024-04-16T12:17:59Z

Thanks for pointing to the documentation :)
May I ask what is the reasoning behind never returning an empty list?

miquelbeltran · 2024-04-17T06:44:41Z

Since the plugin returned a ConnectivityResult.none originally, we kept that.

We could add a convenience extension method that returns a single value and wraps the response with a .first, and/or one that just tells if there's connectivity or not.

miquelbeltran changed the title ~~feat(connectivity): support multiple types~~ feat(connectivity)!: support multiple types Feb 16, 2024

suquant force-pushed the feat/connectivity-multiple-types branch from 3785b4b to d59dcc4 Compare February 16, 2024 07:53

miquelbeltran reviewed Feb 16, 2024

View reviewed changes

miquelbeltran changed the title ~~feat(connectivity)!: support multiple types~~ feat(connectivity)!: support multiple ConnectivityResult values at the same time Feb 16, 2024

suquant force-pushed the feat/connectivity-multiple-types branch from d59dcc4 to 9e080b1 Compare February 16, 2024 08:11

suquant marked this pull request as draft February 16, 2024 08:13

suquant force-pushed the feat/connectivity-multiple-types branch 2 times, most recently from 5d3d371 to 6d6b11a Compare February 16, 2024 15:21

suquant force-pushed the feat/connectivity-multiple-types branch from 6d6b11a to 82d30e6 Compare February 16, 2024 15:35

suquant marked this pull request as ready for review February 16, 2024 15:37

suquant force-pushed the feat/connectivity-multiple-types branch from 82d30e6 to e654491 Compare February 16, 2024 15:43

suquant requested a review from miquelbeltran February 16, 2024 15:59

miquelbeltran reviewed Feb 19, 2024

View reviewed changes

...roid/src/main/java/dev/fluttercommunity/plus/connectivity/ConnectivityBroadcastReceiver.java Outdated Show resolved Hide resolved

miquelbeltran reviewed Feb 19, 2024

View reviewed changes

packages/connectivity_plus/connectivity_plus/ios/Classes/ReachabilityConnectivityProvider.swift Outdated Show resolved Hide resolved

miquelbeltran reviewed Feb 19, 2024

View reviewed changes

packages/connectivity_plus/connectivity_plus/ios/Classes/ReachabilityConnectivityProvider.swift Show resolved Hide resolved

miquelbeltran reviewed Feb 19, 2024

View reviewed changes

This was referenced Feb 19, 2024

[connectivity_plus][iOS] checkConnectivity returns None unexpectedly occasionally #858

Closed

checkConnectivity always return none on iOS 15.4 #852

Closed

suquant force-pushed the feat/connectivity-multiple-types branch 7 times, most recently from bc05022 to a3ab732 Compare February 22, 2024 10:19

suquant force-pushed the feat/connectivity-multiple-types branch 2 times, most recently from 20b2450 to 2abc3b1 Compare February 22, 2024 10:24

miquelbeltran self-assigned this Feb 26, 2024

miquelbeltran added 2 commits February 27, 2024 10:48

Merge branch 'main' into feat/connectivity-multiple-types

a3b3897

Merge branch 'main' into feat/connectivity-multiple-types

b64f8b8

miquelbeltran reviewed Feb 27, 2024

View reviewed changes

miquelbeltran mentioned this pull request Feb 28, 2024

[Bug]: Repeated Connectivity result - ConnectivityResult.mobile #2142

Closed

7 tasks

vbuberen requested changes Feb 28, 2024

View reviewed changes

suquant force-pushed the feat/connectivity-multiple-types branch from 74cc9ac to b64f8b8 Compare March 4, 2024 11:07

suquant added 2 commits March 4, 2024 15:11

feat(connectivity)!: merge with main

1cf52b9

feat(connectivity)!: rename apple plugin

abb25ac

Signed-off-by: George Kutsurua <[email protected]>

suquant force-pushed the feat/connectivity-multiple-types branch from 474b154 to abb25ac Compare March 4, 2024 11:33

Merge branch 'main' into feat/connectivity-multiple-types

701b707

miquelbeltran approved these changes Mar 6, 2024

View reviewed changes

feat(connectivity)!: fix podspec

02ff194

vbuberen approved these changes Mar 6, 2024

View reviewed changes

vbuberen merged commit 5b47746 into fluttercommunity:main Mar 6, 2024
18 of 19 checks passed

miquelbeltran mentioned this pull request Mar 12, 2024

[Bug]: When you connect to wifi and turn on vpn #2677

Closed

8 tasks

erdemyerebasmaz mentioned this pull request Mar 21, 2024

Update dependencies to latest breez/c-breez#846

Merged

miquelbeltran mentioned this pull request Mar 21, 2024

docs(connectivity_plus): Improve documentation onConnectivityChanged method #2746

Merged

7 tasks

Brazol mentioned this pull request May 1, 2024

feat(core): migrate connectivity_plus library GetStream/stream-chat-flutter#1913

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(connectivity)!: support multiple ConnectivityResult values at the same time #2599

feat(connectivity)!: support multiple ConnectivityResult values at the same time #2599

suquant commented Feb 15, 2024 •

edited

Loading

miquelbeltran left a comment

suquant commented Feb 16, 2024

miquelbeltran Feb 19, 2024

suquant Feb 22, 2024 •

edited

Loading

miquelbeltran Feb 26, 2024

suquant Mar 4, 2024

miquelbeltran left a comment

miquelbeltran commented Feb 26, 2024

miquelbeltran commented Feb 27, 2024

miquelbeltran left a comment

miquelbeltran Feb 27, 2024

vbuberen Feb 28, 2024

suquant Mar 4, 2024

miquelbeltran Mar 6, 2024

vbuberen left a comment

vbuberen Feb 28, 2024

miquelbeltran left a comment

miquelbeltran Mar 6, 2024

miquelbeltran Mar 6, 2024

vbuberen commented Mar 6, 2024

suquant commented Mar 6, 2024

vbuberen commented Mar 6, 2024

pamafe1976 commented Mar 21, 2024 •

edited

Loading

vbuberen commented Mar 21, 2024

janniklind commented Apr 15, 2024 •

edited

Loading

miquelbeltran commented Apr 15, 2024

janniklind commented Apr 16, 2024

miquelbeltran commented Apr 17, 2024

feat(connectivity)!: support multiple ConnectivityResult values at the same time #2599

feat(connectivity)!: support multiple ConnectivityResult values at the same time #2599

Conversation

suquant commented Feb 15, 2024 • edited Loading

Description

Related Issues

Checklist

Breaking Change

miquelbeltran left a comment

Choose a reason for hiding this comment

suquant commented Feb 16, 2024

Choose a reason for hiding this comment

suquant Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miquelbeltran left a comment

Choose a reason for hiding this comment

miquelbeltran commented Feb 26, 2024

miquelbeltran commented Feb 27, 2024

miquelbeltran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbuberen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miquelbeltran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbuberen commented Mar 6, 2024

suquant commented Mar 6, 2024

vbuberen commented Mar 6, 2024

pamafe1976 commented Mar 21, 2024 • edited Loading

vbuberen commented Mar 21, 2024

janniklind commented Apr 15, 2024 • edited Loading

miquelbeltran commented Apr 15, 2024

janniklind commented Apr 16, 2024

miquelbeltran commented Apr 17, 2024

suquant commented Feb 15, 2024 •

edited

Loading

suquant Feb 22, 2024 •

edited

Loading

pamafe1976 commented Mar 21, 2024 •

edited

Loading

janniklind commented Apr 15, 2024 •

edited

Loading