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

Notifications: simplify the flow by removing persistence #2924

Merged
merged 16 commits into from
May 29, 2024
Merged
Show file tree
Hide file tree
Changes from 10 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
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ object NotificationConfig {
// TODO EAx Implement and set to true at some point
const val SUPPORT_MARK_AS_READ_ACTION = false

// TODO EAx Implement and set to true at some point
const val SUPPORT_JOIN_DECLINE_INVITE = false

// TODO EAx Implement and set to true at some point
const val SUPPORT_QUICK_REPLY_ACTION = false
}
3 changes: 3 additions & 0 deletions changelog.d/2924.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Simplify notifications by removing the custom persistence layer.

Bump minSdk to 24 (Android 7).
Original file line number Diff line number Diff line change
Expand Up @@ -72,15 +72,10 @@
startForeground(1, notification)
}

@Suppress("DEPRECATION")
override fun onDestroy() {
super.onDestroy()

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) {
stopForeground(STOP_FOREGROUND_REMOVE)
} else {
stopForeground(true)
}
stopForeground(STOP_FOREGROUND_REMOVE)

Check warning on line 78 in features/call/src/main/kotlin/io/element/android/features/call/CallForegroundService.kt

View check run for this annotation

Codecov / codecov/patch

features/call/src/main/kotlin/io/element/android/features/call/CallForegroundService.kt#L78

Added line #L78 was not covered by tests
}

override fun onBind(intent: Intent?): IBinder? {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ class AcceptDeclineInvitePresenter @Inject constructor(
trigger = JoinedRoom.Trigger.Invite,
)
.onSuccess {
notificationDrawerManager.clearMembershipNotificationForRoom(client.sessionId, roomId, doRender = true)
notificationDrawerManager.clearMembershipNotificationForRoom(client.sessionId, roomId)
}
.map { roomId }
}
Expand All @@ -122,7 +122,7 @@ class AcceptDeclineInvitePresenter @Inject constructor(
suspend {
client.getRoom(roomId)?.use {
it.leave().getOrThrow()
notificationDrawerManager.clearMembershipNotificationForRoom(client.sessionId, roomId, doRender = true)
notificationDrawerManager.clearMembershipNotificationForRoom(client.sessionId, roomId)
}
roomId
}.runCatchingUpdatingState(declinedAction)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,13 @@

package io.element.android.features.login.impl.oidc.webview

import android.annotation.TargetApi
import android.os.Build
import android.webkit.WebResourceRequest
import android.webkit.WebView
import android.webkit.WebViewClient

class OidcWebViewClient(
private val eventListener: WebViewEventListener,
) : WebViewClient() {
@TargetApi(Build.VERSION_CODES.N)
override fun shouldOverrideUrlLoading(view: WebView, request: WebResourceRequest): Boolean {
return shouldOverrideUrl(request.url.toString())
}
Expand All @@ -36,7 +33,6 @@ class OidcWebViewClient(
}

private fun shouldOverrideUrl(url: String): Boolean {
// Timber.d("shouldOverrideUrl: $url")
return eventListener.shouldOverrideUrlLoading(url)
}
}
1 change: 1 addition & 0 deletions features/migration/impl/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ dependencies {
testImplementation(libs.test.junit)
testImplementation(libs.coroutines.test)
testImplementation(libs.molecule.runtime)
testImplementation(libs.test.robolectric)
testImplementation(libs.test.truth)
testImplementation(libs.test.turbine)
testImplementation(projects.libraries.sessionStorage.implMemory)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
* Copyright (c) 2024 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.migration.impl.migrations

import android.content.Context
import com.squareup.anvil.annotations.ContributesMultibinding
import io.element.android.libraries.di.AppScope
import io.element.android.libraries.di.ApplicationContext
import javax.inject.Inject

/**
* Remove notifications.bin file, used to store notification data locally.
Copy link
Member

Choose a reason for hiding this comment

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

Perfect, thanks!

*/
@ContributesMultibinding(AppScope::class)
class AppMigration04 @Inject constructor(
@ApplicationContext private val context: Context,
) : AppMigration {
companion object {
internal const val NOTIFICATION_FILE_NAME = "notifications.bin"
}
override val order: Int = 4

override suspend fun migrate() {
runCatching { context.getDatabasePath(NOTIFICATION_FILE_NAME).delete() }
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* Copyright (c) 2024 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.migration.impl.migrations

import androidx.test.platform.app.InstrumentationRegistry
import com.google.common.truth.Truth.assertThat
import kotlinx.coroutines.test.runTest
import org.junit.Test
import org.junit.runner.RunWith
import org.robolectric.RobolectricTestRunner

@RunWith(RobolectricTestRunner::class)
class AppMigration04Test {
@Test
fun `test migration`() = runTest {
val context = InstrumentationRegistry.getInstrumentation().context

// Create fake temporary file at the path to be deleted
val file = context.getDatabasePath(AppMigration04.NOTIFICATION_FILE_NAME)
file.parentFile?.mkdirs()
file.createNewFile()

val migration = AppMigration04(context)

migration.migrate()

// Check that the file has been deleted
assertThat(file.exists()).isFalse()
Copy link
Member

Choose a reason for hiding this comment

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

Just to ensure that the test behaves as expected, I would add

assertThat(file.exists()).isTrue()

before invoking migration.migrate()

Copy link
Member Author

Choose a reason for hiding this comment

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

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -59,16 +59,16 @@ class AndroidMediaPreProcessorTest {
assertThat(data.file.path).endsWith("image.png")
val info = data as MediaUploadInfo.Image
// Computing thumbnailFile is failing with Robolectric
assertThat(info.thumbnailFile).isNull()
assertThat(info.thumbnailFile).isNotNull()
Copy link
Member

Choose a reason for hiding this comment

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

Maybe remove the comment above?

Copy link
Member Author

Choose a reason for hiding this comment

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

assertThat(info.imageInfo).isEqualTo(
ImageInfo(
height = 1_178,
width = 1_818,
mimetype = MimeTypes.Png,
size = 114_867,
thumbnailInfo = null,
ThumbnailInfo(height = 294, width = 454, mimetype = "image/jpeg", size = 4567),
thumbnailSource = null,
blurhash = null,
blurhash = "K13]7q%zWC00R4of%\$baad"
)
)
assertThat(file.exists()).isTrue()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,5 @@ import io.element.android.libraries.matrix.api.core.SessionId

interface NotificationDrawerManager {
fun clearMembershipNotificationForSession(sessionId: SessionId)
fun clearMembershipNotificationForRoom(sessionId: SessionId, roomId: RoomId, doRender: Boolean)
fun clearMembershipNotificationForRoom(sessionId: SessionId, roomId: RoomId)
}
3 changes: 2 additions & 1 deletion libraries/push/impl/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ dependencies {
implementation(projects.libraries.network)
implementation(projects.libraries.matrix.api)
implementation(projects.libraries.matrixui)
implementation(projects.libraries.preferences.api)
implementation(projects.libraries.uiStrings)
implementation(projects.libraries.troubleshoot.api)
api(projects.libraries.pushproviders.api)
Expand All @@ -63,8 +64,8 @@ dependencies {
implementation(projects.services.toolbox.api)

testImplementation(libs.test.junit)
testImplementation(libs.test.robolectric)
testImplementation(libs.test.mockk)
testImplementation(libs.test.robolectric)
testImplementation(libs.test.truth)
testImplementation(libs.test.turbine)
testImplementation(libs.coil.test)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* Copyright (c) 2024 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.libraries.push.impl.di

import android.content.Context
import androidx.core.app.NotificationManagerCompat
import com.squareup.anvil.annotations.ContributesTo
import dagger.Module
import dagger.Provides
import io.element.android.libraries.di.AppScope
import io.element.android.libraries.di.ApplicationContext

@Module
@ContributesTo(AppScope::class)
object PushModule {
@Provides
fun provideNotificationCompatManager(@ApplicationContext context: Context): NotificationManagerCompat {
return NotificationManagerCompat.from(context)

Check warning on line 32 in libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/di/PushModule.kt

View check run for this annotation

Codecov / codecov/patch

libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/di/PushModule.kt#L32

Added line #L32 was not covered by tests
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe Inject NotificationManagerCompat at some other places? You can look for NotificationManagerCompat.from to see where.

Copy link
Member Author

Choose a reason for hiding this comment

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

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/*
* Copyright (c) 2024 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.libraries.push.impl.notifications

import android.service.notification.StatusBarNotification
import androidx.core.app.NotificationManagerCompat
import com.squareup.anvil.annotations.ContributesBinding
import io.element.android.libraries.di.AppScope
import io.element.android.libraries.matrix.api.core.RoomId
import io.element.android.libraries.matrix.api.core.SessionId
import javax.inject.Inject

interface ActiveNotificationsProvider {
fun getAllNotifications(): List<StatusBarNotification>
fun getMessageNotificationsForRoom(sessionId: SessionId, roomId: RoomId): List<StatusBarNotification>
fun getNotificationsForSession(sessionId: SessionId): List<StatusBarNotification>
fun getMembershipNotificationForSession(sessionId: SessionId): List<StatusBarNotification>
fun getMembershipNotificationForRoom(sessionId: SessionId, roomId: RoomId): List<StatusBarNotification>
fun getSummaryNotification(sessionId: SessionId): StatusBarNotification?
fun count(): Int
}

@ContributesBinding(AppScope::class)
class DefaultActiveNotificationsProvider @Inject constructor(
private val notificationManager: NotificationManagerCompat,
private val notificationIdProvider: NotificationIdProvider,
) : ActiveNotificationsProvider {
override fun getAllNotifications(): List<StatusBarNotification> {
return notificationManager.activeNotifications
}

override fun getNotificationsForSession(sessionId: SessionId): List<StatusBarNotification> {
return notificationManager.activeNotifications.filter { it.groupKey == sessionId.value }
jmartinesp marked this conversation as resolved.
Show resolved Hide resolved
}

override fun getMembershipNotificationForSession(sessionId: SessionId): List<StatusBarNotification> {
val notificationId = notificationIdProvider.getRoomInvitationNotificationId(sessionId)
return getNotificationsForSession(sessionId).filter { it.id == notificationId }
}

override fun getMessageNotificationsForRoom(sessionId: SessionId, roomId: RoomId): List<StatusBarNotification> {
val notificationId = notificationIdProvider.getRoomMessagesNotificationId(sessionId)
return notificationManager.activeNotifications.filter { it.id == notificationId && it.tag == roomId.value }
Copy link
Member

Choose a reason for hiding this comment

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

For consistency, I would use

Suggested change
return notificationManager.activeNotifications.filter { it.id == notificationId && it.tag == roomId.value }
return getNotificationsForSession(sessionId).filter { it.id == notificationId && it.tag == roomId.value }

But the getRoomMessagesNotificationId also considers the sessionId. But again, consistency FTW.

Copy link
Member Author

Choose a reason for hiding this comment

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

}

override fun getMembershipNotificationForRoom(sessionId: SessionId, roomId: RoomId): List<StatusBarNotification> {
val notificationId = notificationIdProvider.getRoomInvitationNotificationId(sessionId)
return getNotificationsForSession(sessionId).filter { it.id == notificationId && it.tag == roomId.value }
}

override fun getSummaryNotification(sessionId: SessionId): StatusBarNotification? {
val summaryId = notificationIdProvider.getSummaryNotificationId(sessionId)
return notificationManager.activeNotifications.find { it.id == summaryId }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return notificationManager.activeNotifications.find { it.id == summaryId }
return getNotificationsForSession(sessionId).find { it.id == summaryId }

Copy link
Member Author

Choose a reason for hiding this comment

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

}

override fun count(): Int {
Copy link
Member

Choose a reason for hiding this comment

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

count should take a sessionId as parameter

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch!

Copy link
Member Author

Choose a reason for hiding this comment

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

return getAllNotifications().size

Check warning on line 71 in libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/ActiveNotificationsProvider.kt

View check run for this annotation

Codecov / codecov/patch

libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/ActiveNotificationsProvider.kt#L71

Added line #L71 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return getAllNotifications().size
return getNotificationsForSession(sessionId).size

Copy link
Member Author

Choose a reason for hiding this comment

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

}
}
Loading
Loading