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

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

Merged
merged 7 commits into from
Apr 17, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/141.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add a `NetworkMonitor` component to track the network connection status
2 changes: 2 additions & 0 deletions features/messages/impl/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ dependencies {
implementation(projects.libraries.textcomposer)
implementation(projects.libraries.uiStrings)
implementation(projects.libraries.dateformatter.api)
implementation(projects.features.networkmonitor.api)
implementation(libs.coil.compose)
implementation(libs.datetime)
implementation(libs.accompanist.flowlayout)
Expand All @@ -55,6 +56,7 @@ dependencies {
testImplementation(libs.test.turbine)
testImplementation(projects.libraries.matrix.test)
testImplementation(projects.libraries.dateformatter.test)
testImplementation(projects.features.networkmonitor.test)

androidTestImplementation(libs.test.junitext)
ksp(libs.showkase.processor)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import androidx.compose.runtime.Composable
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.MutableState
import androidx.compose.runtime.collectAsState
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.rememberCoroutineScope
Expand All @@ -38,6 +39,8 @@ import io.element.android.libraries.designsystem.components.avatar.AvatarData
import io.element.android.libraries.designsystem.components.avatar.AvatarSize
import io.element.android.libraries.matrix.api.room.MatrixRoom
import io.element.android.libraries.textcomposer.MessageComposerMode
import io.element.android.features.networkmonitor.api.NetworkMonitor
import io.element.android.features.networkmonitor.api.NetworkStatus
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.launch
import timber.log.Timber
Expand All @@ -48,6 +51,7 @@ class MessagesPresenter @Inject constructor(
private val composerPresenter: MessageComposerPresenter,
private val timelinePresenter: TimelinePresenter,
private val actionListPresenter: ActionListPresenter,
private val networkMonitor: NetworkMonitor,
) : Presenter<MessagesState> {

@Composable
Expand All @@ -64,6 +68,9 @@ class MessagesPresenter @Inject constructor(
val roomAvatar: MutableState<AvatarData?> = remember {
mutableStateOf(null)
}

val networkConnectionStatus by networkMonitor.connectivity.collectAsState(initial = NetworkStatus.Online)

LaunchedEffect(syncUpdateFlow) {
roomAvatar.value =
AvatarData(
Expand All @@ -89,6 +96,7 @@ class MessagesPresenter @Inject constructor(
composerState = composerState,
timelineState = timelineState,
actionListState = actionListState,
hasNetworkConnection = networkConnectionStatus == NetworkStatus.Online,
eventSink = ::handleEvents
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,5 +31,6 @@ data class MessagesState(
val composerState: MessageComposerState,
val timelineState: TimelineState,
val actionListState: ActionListState,
val hasNetworkConnection: Boolean,
val eventSink: (MessagesEvents) -> Unit
)
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ open class MessagesStateProvider : PreviewParameterProvider<MessagesState> {
override val values: Sequence<MessagesState>
get() = sequenceOf(
aMessagesState(),
aMessagesState().copy(hasNetworkConnection = false),
)
}

Expand All @@ -47,5 +48,6 @@ fun aMessagesState() = MessagesState(
timelineItems = aTimelineItemList(aTimelineItemContent()),
),
actionListState = anActionListState(),
hasNetworkConnection = true,
eventSink = {}
)
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ import io.element.android.libraries.designsystem.theme.components.Scaffold
import io.element.android.libraries.designsystem.theme.components.Text
import io.element.android.libraries.designsystem.theme.components.TopAppBar
import io.element.android.libraries.designsystem.utils.LogCompositions
import io.element.android.libraries.matrix.api.core.RoomId
import io.element.android.features.networkmonitor.api.ui.ConnectivityIndicatorView
import kotlinx.coroutines.launch
import timber.log.Timber

Expand Down Expand Up @@ -112,12 +112,15 @@ fun MessagesView(
modifier = modifier,
contentWindowInsets = WindowInsets.statusBars,
topBar = {
MessagesViewTopBar(
roomTitle = state.roomName,
roomAvatar = state.roomAvatar,
onBackPressed = onBackPressed,
onRoomDetailsClicked = onRoomDetailsClicked,
)
Column {
ConnectivityIndicatorView(isOnline = state.hasNetworkConnection)
MessagesViewTopBar(
roomTitle = state.roomName,
roomAvatar = state.roomAvatar,
onBackPressed = onBackPressed,
onRoomDetailsClicked = onRoomDetailsClicked,
)
}
},
content = { padding ->
MessagesViewContent(
Expand Down Expand Up @@ -208,7 +211,8 @@ fun MessagesViewTopBar(
overflow = TextOverflow.Ellipsis
)
}
}
},
windowInsets = WindowInsets(0.dp)
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import io.element.android.features.messages.impl.actionlist.ActionListPresenter
import io.element.android.features.messages.impl.actionlist.model.TimelineItemAction
import io.element.android.features.messages.impl.textcomposer.MessageComposerPresenter
import io.element.android.features.messages.impl.timeline.TimelinePresenter
import io.element.android.features.networkmonitor.test.FakeNetworkMonitor
import io.element.android.libraries.matrix.api.room.MatrixRoom
import io.element.android.libraries.matrix.test.AN_EVENT_ID
import io.element.android.libraries.matrix.test.A_ROOM_ID
Expand Down Expand Up @@ -141,6 +142,7 @@ class MessagesPresenterTest {
composerPresenter = messageComposerPresenter,
timelinePresenter = timelinePresenter,
actionListPresenter = actionListPresenter,
networkMonitor = FakeNetworkMonitor(),
)
}
}
Expand Down
30 changes: 30 additions & 0 deletions features/networkmonitor/api/build.gradle.kts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright (c) 2022 New Vector Ltd
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

// TODO: Remove once https://youtrack.jetbrains.com/issue/KTIJ-19369 is fixed
@Suppress("DSL_SCOPE_VIOLATION")
plugins {
id("io.element.android-compose-library")
}

android {
namespace = "io.element.android.features.networkmonitor.api"
}

dependencies {
implementation(libs.coroutines.core)
implementation(projects.libraries.designsystem)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*
* Copyright (c) 2023 New Vector Ltd
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package io.element.android.features.networkmonitor.api

import kotlinx.coroutines.flow.Flow

interface NetworkMonitor {
val connectivity: Flow<NetworkStatus>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Copyright (c) 2023 New Vector Ltd
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package io.element.android.features.networkmonitor.api

enum class NetworkStatus {
Online,
Offline
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
/*
* Copyright (c) 2023 New Vector Ltd
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package io.element.android.features.networkmonitor.api.ui

import androidx.compose.animation.AnimatedVisibility
import androidx.compose.animation.expandVertically
import androidx.compose.animation.shrinkVertically
import androidx.compose.foundation.Image
import androidx.compose.foundation.background
import androidx.compose.foundation.layout.Arrangement
import androidx.compose.foundation.layout.Row
import androidx.compose.foundation.layout.Spacer
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.size
import androidx.compose.foundation.layout.statusBarsPadding
import androidx.compose.foundation.layout.width
import androidx.compose.material.Text
import androidx.compose.material.icons.Icons
import androidx.compose.material.icons.outlined.WifiOff
import androidx.compose.material3.MaterialTheme
import androidx.compose.runtime.Composable
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.graphics.ColorFilter
import androidx.compose.ui.tooling.preview.Preview
import androidx.compose.ui.unit.dp
import io.element.android.libraries.designsystem.ElementTextStyles
import io.element.android.libraries.designsystem.preview.ElementPreviewDark
import io.element.android.libraries.designsystem.preview.ElementPreviewLight
import io.element.android.libraries.designsystem.theme.LocalColors

@Composable
fun ConnectivityIndicatorView(
isOnline: Boolean,
modifier: Modifier = Modifier
) {
// Display the network indicator with an animation
AnimatedVisibility(
visible = !isOnline,
enter = expandVertically(),
exit = shrinkVertically(),
) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Row(
modifier
.fillMaxWidth()
.background(LocalColors.current.gray400)
.statusBarsPadding()
.padding(vertical = 6.dp),
horizontalArrangement = Arrangement.Center,
verticalAlignment = Alignment.Bottom,
) {
val tint = MaterialTheme.colorScheme.primary
Image(
imageVector = Icons.Outlined.WifiOff,
contentDescription = null,
colorFilter = ColorFilter.tint(tint),
modifier = Modifier.size(16.dp),
)
Spacer(modifier = Modifier.width(8.dp))
Text(text = "Offline", style = ElementTextStyles.Regular.bodyMD, color = tint)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose this string should be defined in localazy

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in b7dfd75

}
}

// Show missing status bar padding when the indicator is not visible
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

AnimatedVisibility(
visible = isOnline,
enter = expandVertically(),
exit = shrinkVertically(),
) {
Spacer(modifier = Modifier.statusBarsPadding())
}
}

@Preview
@Composable
internal fun PreviewLightConnectivityIndicatorView() {
ElementPreviewLight {
ConnectivityIndicatorView(isOnline = false)
}
}

@Preview
@Composable
internal fun PreviewDarkConnectivityIndicatorView() {
ElementPreviewDark {
ConnectivityIndicatorView(isOnline = false)
}
}
38 changes: 38 additions & 0 deletions features/networkmonitor/impl/build.gradle.kts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/*
* Copyright (c) 2022 New Vector Ltd
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

// TODO: Remove once https://youtrack.jetbrains.com/issue/KTIJ-19369 is fixed
@Suppress("DSL_SCOPE_VIOLATION")
plugins {
id("io.element.android-library")
alias(libs.plugins.anvil)
}

anvil {
generateDaggerFactories.set(true)
}

android {
namespace = "io.element.android.features.networkmonitor.impl"
}

dependencies {
implementation(libs.coroutines.core)
implementation(libs.dagger)
implementation(projects.libraries.core)
implementation(projects.libraries.di)
api(projects.features.networkmonitor.api)
}
19 changes: 19 additions & 0 deletions features/networkmonitor/impl/src/main/AndroidManifest.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<!--
~ Copyright (c) 2023 New Vector Ltd
~
~ Licensed under the Apache License, Version 2.0 (the "License");
~ you may not use this file except in compliance with the License.
~ You may obtain a copy of the License at
~
~ http://www.apache.org/licenses/LICENSE-2.0
~
~ Unless required by applicable law or agreed to in writing, software
~ distributed under the License is distributed on an "AS IS" BASIS,
~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
~ See the License for the specific language governing permissions and
~ limitations under the License.
-->

<manifest xmlns:android="http://schemas.android.com/apk/res/android">
<uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" />
</manifest>
Loading