[Room List] Show offline indicator when the device is offline #239

jmartinesp · 2023-03-21T12:31:58Z

What

Adds :feature:networkmonitor module containing a NetworkMonitor component that listens to the network connection status.
Creates a ConnectivityIndicatorView following the designs, that will appear and disappear with an animation.

Why

Implements #141 .

Tests

On both the room list and chat screens, turn airplane mode on and off.

github-actions · 2023-03-30T17:11:04Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/3RH3ba

codecov · 2023-03-30T17:20:56Z

Codecov Report

Patch coverage: 55.55% and project coverage change: +1.34 🎉

Comparison is base (775cf2a) 44.36% compared to head (3dbc173) 45.71%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #239      +/-   ##
===========================================
+ Coverage    44.36%   45.71%   +1.34%     
===========================================
  Files          544      590      +46     
  Lines        12584    13338     +754     
  Branches      2628     2807     +179     
===========================================
+ Hits          5583     6097     +514     
- Misses        5970     6095     +125     
- Partials      1031     1146     +115

Impacted Files	Coverage Δ
...features/networkmonitor/impl/NetworkMonitorImpl.kt	`0.00% <0.00%> (ø)`
.../element/android/samples/minimal/RoomListScreen.kt	`0.00% <0.00%> (ø)`
...networkmonitor/api/ui/ConnectivityIndicatorView.kt	`62.22% <62.22%> (ø)`
...ndroid/features/roomlist/impl/RoomListPresenter.kt	`89.41% <66.66%> (-0.95%)`	⬇️
...ndroid/features/messages/impl/MessagesPresenter.kt	`83.60% <75.00%> (-0.61%)`	⬇️
...ent/android/features/roomlist/impl/RoomListView.kt	`63.04% <77.77%> (+0.99%)`	⬆️
...ent/android/features/messages/impl/MessagesView.kt	`58.00% <88.88%> (+2.32%)`	⬆️
...nt/android/features/messages/impl/MessagesState.kt	`100.00% <100.00%> (ø)`
...id/features/messages/impl/MessagesStateProvider.kt	`94.11% <100.00%> (+0.78%)`	⬆️
...droid/features/networkmonitor/api/NetworkStatus.kt	`100.00% <100.00%> (ø)`
... and 8 more

... and 152 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Florian14 · 2023-03-31T12:47:44Z

Just tested it on my device, the banner pops and disappears immediately

jmartinesp · 2023-03-31T12:50:45Z

Just tested it on my device, the banner pops and disappears immediately

Could you add some more info or upload a video? I haven't seen anything like that while testing it on both an emulator and a real device.

Florian14 · 2023-03-31T12:56:44Z

Just tested it on my device, the banner pops and disappears immediately

Could you add some more info or upload a video? I haven't seen anything like that while testing it on both an emulator and a real device.

Screen_Recording_20230331_145328_ElementX.dbg.mp4

jmartinesp · 2023-03-31T12:59:56Z

Ok, that's super weird. Which device model is it? And its Android version?

Florian14 · 2023-03-31T13:02:51Z

Ok, that's super weird. Which device model is it? And its Android version?

S10e running Android 12, I can debug a bit if you want. I installed using the apk from diawi

Edit: I'll do the code review first, in case I find the problem

Florian14

Some interesting logs:

16:44:34.935 NetworkMon...s$callback  V  Connectivity status (lost): Online
16:44:35.208 NetworkMon...s$callback  V  Connectivity status (available): Online
16:44:35.214 NetworkMon...s$callback  V  Connectivity status (changed): Online
16:44:36.148 NetworkMon...s$callback  V  Connectivity status (lost): Online

But the code looks good... I'll debug to see how the connectivity manager behaves on my device

Florian14 · 2023-03-31T14:32:18Z

...c/main/kotlin/io/element/android/features/networkmonitor/api/ui/ConnectivityIndicatorView.kt

+                modifier = Modifier.size(16.dp),
+            )
+            Spacer(modifier = Modifier.width(8.dp))
+            Text(text = "Offline", style = ElementTextStyles.Regular.bodyMD, color = tint)


I suppose this string should be defined in localazy

Fixed in b7dfd75

…e-status-banner

jmartinesp · 2023-04-13T08:22:47Z

@Florian14 did you have a chance to test it on your device again? Sadly, it seems like I can't reproduce the issue you mentioned 🙁 .

Florian14 · 2023-04-13T08:28:16Z

@Florian14 did you have a chance to test it on your device again? Sadly, it seems like I can't reproduce the issue you mentioned 🙁 .

Can you merge/rebase your branch on develop? It seems that the apk is not available anymore :/

…e-status-banner

Florian14 · 2023-04-13T09:46:56Z

@jmartinesp I still have the issue :/ Maybe we can ask someone else to test it. Otherwise, I should probably do the investigation by myself as you're not able to reproduce but I don't have time to do it right now.

jmartinesp · 2023-04-13T13:43:21Z

@jmartinesp I still have the issue :/ Maybe we can ask someone else to test it. Otherwise, I should probably do the investigation by myself as you're not able to reproduce but I don't have time to do it right now.

Ok, I'll re-assign the reviewer. I'll also try to get my hands in a Samsung device to check if it's a vendor issue, I might know someone who can lend me a test device.

bmarty

I do not have the issue reported by Florian (tested on emu API 33), also if I test some of my remark the feature was sort of broken, so be careful, do no trust me too much :).

bmarty · 2023-04-14T09:59:56Z

...c/main/kotlin/io/element/android/features/networkmonitor/api/ui/ConnectivityIndicatorView.kt

+        }
+    }
+
+    // Show missing status bar padding when the indicator is not visible


This comment does not really match the behavior, I would expect to have the code below surrounded by if (!isOnline).

Actually, the message is accurate, although it's a bit difficult to follow: the TopAppBar component has a default top padding equal to the statusbar height, but to present this indicator behind the statusbar we remove it by default. So when no indicator is shown we need a placeholder Spacer with that same height/padding to keep the TopAppBar in its expected position instead of below the statusbar.

OK, I understand. I would have made the padding always visible and the network status bar not include the padding.

It would also fix the weird effect (to me!) to see the status bar moving under the system top status bar. But not a blocker, thanks for the explanaition.

I tried to do that at first with a Box, but as the TopAppBar needs to move down when the indicator appears, it was quite hard to build it that way. There might be a simpler solution though.

bmarty · 2023-04-14T10:07:59Z

...r/impl/src/main/kotlin/io/element/android/features/networkmonitor/impl/NetworkMonitorImpl.kt

+
+    private val connectivityManager: ConnectivityManager by lazy {
+        context.getSystemService(ConnectivityManager::class.java)
+    }


not useful to use lazy, since connectivityManager is used in the constructor.

bmarty · 2023-04-14T10:08:29Z

...r/impl/src/main/kotlin/io/element/android/features/networkmonitor/impl/NetworkMonitorImpl.kt

+    private fun subscribeToConnectionChanges() {
+        if (callback != null) return
+
+        val callback = object : ConnectivityManager.NetworkCallback() {


Why not make this.callback a val and init it at declaration?

bmarty · 2023-04-14T10:10:13Z

.../impl/src/main/kotlin/io/element/android/features/roomlist/impl/components/RoomListTopBar.kt

@@ -71,6 +73,7 @@ fun RoomListTopBar(
    onFilterChanged: (String) -> Unit,
    onOpenSettings: () -> Unit,
    scrollBehavior: TopAppBarScrollBehavior,
+    useStatusBarInsets: Boolean,


bmarty · 2023-04-14T10:24:20Z

...r/impl/src/main/kotlin/io/element/android/features/networkmonitor/impl/NetworkMonitorImpl.kt

+import timber.log.Timber
+import javax.inject.Inject
+
+@ContributesBinding(scope = AppScope::class, boundType = NetworkMonitor::class)


Neat: this is not necessary to specify the boundType, since this class implement only one interface (it was maybe not the case before :))

bmarty · 2023-04-14T10:27:00Z

...c/main/kotlin/io/element/android/features/networkmonitor/api/ui/ConnectivityIndicatorView.kt

+        visible = !isOnline,
+        enter = expandVertically(),
+        exit = shrinkVertically(),
+    ) {


The animation is mixing with the animation when switching between room list and message list, this is a bit weird. It should just animate when the network state changes.

jmartinesp · 2023-04-17T05:46:30Z

@bmarty I think all you comments should have been solved in 3dbc173.

Also, I tested the branch in a Samsung device (although not the same model as @Florian14 's) and I couldn't reproduce the issue, so either it's solved (there's always hope) or it's specific to some device/version of the OS.

manuroe · 2023-04-17T06:09:42Z

If it can help, the offline banner works as expected on my Samsung A50.

bmarty

Thanks for the udpate! (will test again and report issue (if any!) once on the nightly).

Implement the network status indicator

73d12e8

jmartinesp force-pushed the feature/jme/offline-status-banner branch from 1605097 to 73d12e8 Compare March 30, 2023 16:55

jmartinesp requested review from a team and Florian14 and removed request for a team March 30, 2023 16:58

jmartinesp added 2 commits March 30, 2023 19:13

Make networkmonitor a feature, not a service.

1aaf35d

Add changelog

d0765f7

jmartinesp marked this pull request as ready for review March 30, 2023 18:10

Florian14 reviewed Mar 31, 2023

View reviewed changes

jmartinesp added 2 commits April 5, 2023 11:23

Merge remote-tracking branch 'origin/develop' into feature/jme/offlin…

5c3274e

…e-status-banner

Fix hardcoded word

a9940e4

jmartinesp force-pushed the feature/jme/offline-status-banner branch from b7dfd75 to a9940e4 Compare April 5, 2023 09:36

Merge remote-tracking branch 'origin/develop' into feature/jme/offlin…

3171a5f

…e-status-banner

jmartinesp requested a review from Florian14 April 13, 2023 09:15

jmartinesp requested review from a team and bmarty and removed request for Florian14 and a team April 13, 2023 13:43

bmarty reviewed Apr 14, 2023

View reviewed changes

Address review comments

3dbc173

jmartinesp requested a review from bmarty April 17, 2023 05:46

bmarty approved these changes Apr 17, 2023

View reviewed changes

jmartinesp merged commit bfac069 into develop Apr 17, 2023

jmartinesp deleted the feature/jme/offline-status-banner branch April 17, 2023 15:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Room List] Show offline indicator when the device is offline #239

[Room List] Show offline indicator when the device is offline #239

jmartinesp commented Mar 21, 2023 •

edited

Loading

github-actions bot commented Mar 30, 2023 •

edited

Loading

codecov bot commented Mar 30, 2023 •

edited

Loading

Florian14 commented Mar 31, 2023

jmartinesp commented Mar 31, 2023

Florian14 commented Mar 31, 2023

jmartinesp commented Mar 31, 2023

Florian14 commented Mar 31, 2023 •

edited

Loading

Florian14 left a comment •

edited

Loading

Florian14 Mar 31, 2023

jmartinesp Apr 5, 2023

jmartinesp commented Apr 13, 2023

Florian14 commented Apr 13, 2023

Florian14 commented Apr 13, 2023

jmartinesp commented Apr 13, 2023 •

edited

Loading

bmarty left a comment

bmarty Apr 14, 2023

jmartinesp Apr 17, 2023

bmarty Apr 17, 2023

jmartinesp Apr 17, 2023

bmarty Apr 14, 2023

bmarty Apr 14, 2023

bmarty Apr 14, 2023

bmarty Apr 14, 2023

bmarty Apr 14, 2023

jmartinesp commented Apr 17, 2023

manuroe commented Apr 17, 2023

bmarty left a comment

[Room List] Show offline indicator when the device is offline #239

[Room List] Show offline indicator when the device is offline #239

Conversation

jmartinesp commented Mar 21, 2023 • edited Loading

What

Why

Tests

github-actions bot commented Mar 30, 2023 • edited Loading

codecov bot commented Mar 30, 2023 • edited Loading

Codecov Report

Florian14 commented Mar 31, 2023

jmartinesp commented Mar 31, 2023

Florian14 commented Mar 31, 2023

jmartinesp commented Mar 31, 2023

Florian14 commented Mar 31, 2023 • edited Loading

Florian14 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp commented Apr 13, 2023

Florian14 commented Apr 13, 2023

Florian14 commented Apr 13, 2023

jmartinesp commented Apr 13, 2023 • edited Loading

bmarty 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

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

jmartinesp commented Apr 17, 2023

manuroe commented Apr 17, 2023

bmarty left a comment

Choose a reason for hiding this comment

jmartinesp commented Mar 21, 2023 •

edited

Loading

github-actions bot commented Mar 30, 2023 •

edited

Loading

codecov bot commented Mar 30, 2023 •

edited

Loading

Florian14 commented Mar 31, 2023 •

edited

Loading

Florian14 left a comment •

edited

Loading

jmartinesp commented Apr 13, 2023 •

edited

Loading