Skip to content

Conversation

SttApollo
Copy link
Contributor

@SttApollo SttApollo commented Aug 6, 2025

  • Fixes Use application icon in push notification messages #8932
  • Added NotificationIconResourceProvider with K-9 and Thunderbird-specific providers in their respective Koin modules
  • Modified SyncNotificationController and PushNotificationManager to use injected icon provider
  • Added test coverage for both app implementations and updated SyncNotificationControllerTest
  • Included debug notification activity for manual verification
Screenshot 2025-08-05 at 9 05 23 PM

Copy link
Member

@rafaeltonholo rafaeltonholo left a comment

Choose a reason for hiding this comment

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

Hi @SttApollo, thank you for your contribution! Before we can merge your changes, there are a few items that need to be addressed:

  1. Ensure that your code passes the CI checks.
  2. Please rebase your commit messages according to our Git Commit Guide.
  3. Review and consider addressing all the comments made in this PR.

Additionally, regarding notification debugging, we now have a :feature:debug-settings module. This module was introduced to test the new notification system and should include the notification debugging functionality you introduced there.

(click to expand) Here is an example patch of what would be needed to introduce your notification debugging functionality there:
Subject: [PATCH] chore(notifications): add legacy notification test
---
Index: app-k9mail/src/debug/kotlin/app/k9mail/DebugNotificationActivity.kt
===================================================================
diff --git a/app-k9mail/src/debug/kotlin/app/k9mail/DebugNotificationActivity.kt b/app-k9mail/src/debug/kotlin/app/k9mail/DebugNotificationActivity.kt
deleted file mode 100644
--- a/app-k9mail/src/debug/kotlin/app/k9mail/DebugNotificationActivity.kt	(revision a036ca941adae1356fda8beb8d1fc80048578354)
+++ /dev/null	(revision a036ca941adae1356fda8beb8d1fc80048578354)
@@ -1,60 +0,0 @@
-package app.k9mail
-
-import android.app.Activity
-import android.os.Bundle
-import android.util.Log
-import android.widget.Button
-import android.widget.LinearLayout
-import android.widget.TextView
-import android.widget.Toast
-import app.k9mail.core.android.common.provider.NotificationIconResourceProvider
-import com.fsck.k9.notification.NotificationChannelManager
-import org.koin.java.KoinJavaComponent.inject
-
-class DebugNotificationActivity : Activity() {
-
-    override fun onCreate(savedInstanceState: Bundle?) {
-        super.onCreate(savedInstanceState)
-
-        Log.d("DebugNotifActivity", "onCreate called")
-
-        val layout = LinearLayout(this).apply {
-            orientation = LinearLayout.VERTICAL
-            setPadding(64, 128, 64, 64)
-        }
-
-        val title = TextView(this).apply {
-            text = "🔔 Debug Push Notification"
-            textSize = 22f
-        }
-
-        val button = Button(this).apply {
-            text = "Trigger Notification"
-            setOnClickListener {
-                try {
-                    val iconProvider: Lazy<NotificationIconResourceProvider> =
-                        inject(NotificationIconResourceProvider::class.java)
-                    val channelManager: Lazy<NotificationChannelManager> =
-                        inject(NotificationChannelManager::class.java)
-
-                    NotificationTester.showTestPushNotification(
-                        context = this@DebugNotificationActivity,
-                        iconProvider = iconProvider,
-                        channelManager = channelManager,
-                    )
-                    Toast.makeText(this@DebugNotificationActivity, "Notification sent!", Toast.LENGTH_SHORT).show()
-                } catch (e: Exception) {
-                    Log.e("DebugNotifActivity", "Failed to inject dependencies", e)
-                    Toast.makeText(this@DebugNotificationActivity, "Error: ${e.message}", Toast.LENGTH_LONG).show()
-                }
-            }
-        }
-
-        layout.addView(title)
-        layout.addView(button)
-
-        setContentView(layout)
-
-        Log.d("DebugNotifActivity", "Activity setup complete")
-    }
-}
Index: app-k9mail/src/debug/kotlin/app/k9mail/DebugNotificationReceiver.kt
===================================================================
diff --git a/app-k9mail/src/debug/kotlin/app/k9mail/DebugNotificationReceiver.kt b/app-k9mail/src/debug/kotlin/app/k9mail/DebugNotificationReceiver.kt
deleted file mode 100644
--- a/app-k9mail/src/debug/kotlin/app/k9mail/DebugNotificationReceiver.kt	(revision a036ca941adae1356fda8beb8d1fc80048578354)
+++ /dev/null	(revision a036ca941adae1356fda8beb8d1fc80048578354)
@@ -1,25 +0,0 @@
-package app.k9mail
-
-import android.content.BroadcastReceiver
-import android.content.Context
-import android.content.Intent
-import android.util.Log
-import app.k9mail.core.android.common.provider.NotificationIconResourceProvider
-import com.fsck.k9.notification.NotificationChannelManager
-import org.koin.java.KoinJavaComponent.inject
-
-class DebugNotificationReceiver : BroadcastReceiver() {
-    private val iconProvider: Lazy<NotificationIconResourceProvider> =
-        inject(NotificationIconResourceProvider::class.java)
-    private val channelManager: Lazy<NotificationChannelManager> =
-        inject(NotificationChannelManager::class.java)
-
-    override fun onReceive(context: Context, intent: Intent) {
-        Log.d("DebugNotifReceiver", "Received debug notification broadcast")
-        NotificationTester.showTestPushNotification(
-            context,
-            iconProvider,
-            channelManager,
-        )
-    }
-}
Index: app-k9mail/src/debug/kotlin/app/k9mail/NotificationTester.kt
===================================================================
diff --git a/app-k9mail/src/debug/kotlin/app/k9mail/NotificationTester.kt b/app-k9mail/src/debug/kotlin/app/k9mail/NotificationTester.kt
deleted file mode 100644
--- a/app-k9mail/src/debug/kotlin/app/k9mail/NotificationTester.kt	(revision a036ca941adae1356fda8beb8d1fc80048578354)
+++ /dev/null	(revision a036ca941adae1356fda8beb8d1fc80048578354)
@@ -1,26 +0,0 @@
-package app.k9mail
-
-import android.content.Context
-import androidx.core.app.NotificationCompat
-import androidx.core.app.NotificationManagerCompat
-import app.k9mail.core.android.common.provider.NotificationIconResourceProvider
-import com.fsck.k9.notification.NotificationChannelManager
-
-object NotificationTester {
-    fun showTestPushNotification(
-        context: Context,
-        iconProvider: Lazy<NotificationIconResourceProvider>,
-        channelManager: Lazy<NotificationChannelManager>,
-    ) {
-        val n = NotificationCompat.Builder(context, channelManager.value.pushChannelId)
-            .setSmallIcon(iconProvider.value.pushNotificationIcon)
-            .setContentTitle("🔔 Debug Push")
-            .setContentText("This is a test using the app icon")
-            .setPriority(NotificationCompat.PRIORITY_HIGH)
-            .build()
-
-        @Suppress("MissingPermission")
-        NotificationManagerCompat.from(context)
-            .notify(0xDEAD, n)
-    }
-}
Index: feature/debug-settings/src/main/kotlin/net/thunderbird/feature/debug/settings/notification/legacy/DebugLegacyNotificationSection.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/feature/debug-settings/src/main/kotlin/net/thunderbird/feature/debug/settings/notification/legacy/DebugLegacyNotificationSection.kt b/feature/debug-settings/src/main/kotlin/net/thunderbird/feature/debug/settings/notification/legacy/DebugLegacyNotificationSection.kt
new file mode 100644
--- /dev/null	(date 1754486141789)
+++ b/feature/debug-settings/src/main/kotlin/net/thunderbird/feature/debug/settings/notification/legacy/DebugLegacyNotificationSection.kt	(date 1754486141789)
@@ -0,0 +1,58 @@
+package net.thunderbird.feature.debug.settings.notification.legacy
+
+import androidx.compose.foundation.layout.Arrangement
+import androidx.compose.foundation.layout.Column
+import androidx.compose.foundation.layout.fillMaxWidth
+import androidx.compose.runtime.Composable
+import androidx.compose.ui.Alignment
+import androidx.compose.ui.Modifier
+import androidx.compose.ui.platform.LocalContext
+import androidx.compose.ui.res.stringResource
+import app.k9mail.core.ui.compose.common.mvi.observeWithoutEffect
+import app.k9mail.core.ui.compose.designsystem.atom.button.ButtonFilled
+import app.k9mail.core.ui.compose.designsystem.atom.text.TextTitleSmall
+import app.k9mail.core.ui.compose.theme2.MainTheme
+import net.thunderbird.feature.debug.settings.DebugSection
+import net.thunderbird.feature.debug.settings.R
+import org.koin.androidx.compose.koinViewModel
+
+@Composable
+fun DebugLegacyNotificationSection(
+    modifier: Modifier = Modifier,
+    viewModel: DebugLegacyNotificationSectionViewModel = koinViewModel(),
+) {
+    val (_, dispatch) = viewModel.observeWithoutEffect()
+    val context = LocalContext.current
+
+    DebugLegacyNotificationSection(
+        onTriggerNotificationClick = {
+            dispatch(DebugLegacyNotificationSectionContract.Event.TriggerNotification(context))
+        },
+        modifier = modifier,
+    )
+}
+
+@Composable
+private fun DebugLegacyNotificationSection(
+    onTriggerNotificationClick: () -> Unit,
+    modifier: Modifier = Modifier,
+) {
+    DebugSection(
+        title = stringResource(R.string.debug_settings_legacy_notifications_title),
+        modifier = modifier,
+    ) {
+        Column(
+            modifier = Modifier.fillMaxWidth(),
+            verticalArrangement = Arrangement.spacedBy(MainTheme.spacings.default),
+        ) {
+            TextTitleSmall(
+                text = stringResource(R.string.debug_settings_legacy_notifications_debug_push),
+            )
+            ButtonFilled(
+                text = stringResource(R.string.debug_settings_notifications_trigger_notification),
+                onClick = onTriggerNotificationClick,
+                modifier = Modifier.align(Alignment.CenterHorizontally),
+            )
+        }
+    }
+}
Index: feature/debug-settings/src/main/kotlin/net/thunderbird/feature/debug/settings/notification/legacy/DebugLegacyNotificationSectionContract.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/feature/debug-settings/src/main/kotlin/net/thunderbird/feature/debug/settings/notification/legacy/DebugLegacyNotificationSectionContract.kt b/feature/debug-settings/src/main/kotlin/net/thunderbird/feature/debug/settings/notification/legacy/DebugLegacyNotificationSectionContract.kt
new file mode 100644
--- /dev/null	(date 1754485211877)
+++ b/feature/debug-settings/src/main/kotlin/net/thunderbird/feature/debug/settings/notification/legacy/DebugLegacyNotificationSectionContract.kt	(date 1754485211877)
@@ -0,0 +1,16 @@
+package net.thunderbird.feature.debug.settings.notification.legacy
+
+import android.content.Context
+import app.k9mail.core.ui.compose.common.mvi.UnidirectionalViewModel
+
+interface DebugLegacyNotificationSectionContract {
+    interface ViewModel : UnidirectionalViewModel<State, Event, Effect>
+
+    data object State
+
+    sealed interface Event {
+        data class TriggerNotification(val context: Context) : Event
+    }
+
+    sealed interface Effect
+}
Index: feature/debug-settings/src/main/res/values/strings.xml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/feature/debug-settings/src/main/res/values/strings.xml b/feature/debug-settings/src/main/res/values/strings.xml
--- a/feature/debug-settings/src/main/res/values/strings.xml	(revision a036ca941adae1356fda8beb8d1fc80048578354)
+++ b/feature/debug-settings/src/main/res/values/strings.xml	(date 1754485211880)
@@ -15,4 +15,7 @@
     <string name="debug_settings_notification_status_log">Notification status log</string>
     <string name="debug_settings_notifications_status">"Status: "</string>
     <string name="debug_settings_notifications_clear_status_log">Clear status log</string>
+    <!--  Legacy notification system  -->
+    <string name="debug_settings_legacy_notifications_title">Legacy Notifications</string>
+    <string name="debug_settings_legacy_notifications_debug_push">🔔 Debug Push Notification</string>
 </resources>
Index: feature/debug-settings/src/main/AndroidManifest.xml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/feature/debug-settings/src/main/AndroidManifest.xml b/feature/debug-settings/src/main/AndroidManifest.xml
new file mode 100644
--- /dev/null	(date 1754485765103)
+++ b/feature/debug-settings/src/main/AndroidManifest.xml	(date 1754485765103)
@@ -0,0 +1,7 @@
+<manifest
+    xmlns:android="http://schemas.android.com/apk/res/android">
+
+    <!-- Required for legacy notification tests. Must be removed once migration to the new system is done -->
+    <uses-permission android:name="android.permission.POST_NOTIFICATIONS" />
+
+</manifest>
Index: feature/debug-settings/src/main/kotlin/net/thunderbird/feature/debug/settings/notification/legacy/DebugLegacyNotificationSectionViewModel.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/feature/debug-settings/src/main/kotlin/net/thunderbird/feature/debug/settings/notification/legacy/DebugLegacyNotificationSectionViewModel.kt b/feature/debug-settings/src/main/kotlin/net/thunderbird/feature/debug/settings/notification/legacy/DebugLegacyNotificationSectionViewModel.kt
new file mode 100644
--- /dev/null	(date 1754485765101)
+++ b/feature/debug-settings/src/main/kotlin/net/thunderbird/feature/debug/settings/notification/legacy/DebugLegacyNotificationSectionViewModel.kt	(date 1754485765101)
@@ -0,0 +1,34 @@
+package net.thunderbird.feature.debug.settings.notification.legacy
+
+import android.content.Context
+import androidx.core.app.NotificationCompat
+import androidx.core.app.NotificationManagerCompat
+import app.k9mail.core.android.common.provider.NotificationIconResourceProvider
+import app.k9mail.core.ui.compose.common.mvi.BaseViewModel
+import com.fsck.k9.notification.NotificationChannelManager
+import net.thunderbird.feature.debug.settings.notification.legacy.DebugLegacyNotificationSectionContract.Effect
+import net.thunderbird.feature.debug.settings.notification.legacy.DebugLegacyNotificationSectionContract.Event
+import net.thunderbird.feature.debug.settings.notification.legacy.DebugLegacyNotificationSectionContract.State
+import net.thunderbird.feature.debug.settings.notification.legacy.DebugLegacyNotificationSectionContract.ViewModel
+
+class DebugLegacyNotificationSectionViewModel(
+    private val iconProvider: NotificationIconResourceProvider,
+    private val channelManager: NotificationChannelManager,
+) : BaseViewModel<State, Event, Effect>(initialState = State), ViewModel {
+    override fun event(event: Event) {
+        when (event) {
+            is Event.TriggerNotification -> onTriggerNotification(event.context)
+        }
+    }
+
+    private fun onTriggerNotification(context: Context) {
+        val notification = NotificationCompat.Builder(context, channelManager.pushChannelId)
+            .setSmallIcon(iconProvider.pushNotificationIcon)
+            .setContentTitle("🔔 Debug Push")
+            .setContentText("This is a test using the app icon")
+            .setPriority(NotificationCompat.PRIORITY_HIGH)
+            .build()
+
+        NotificationManagerCompat.from(context).notify(0xDEAD, notification)
+    }
+}
Index: feature/debug-settings/src/debug/kotlin/net/thunderbird/feature/debug/settings/inject/FeatureDebugSettingsModule.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/feature/debug-settings/src/debug/kotlin/net/thunderbird/feature/debug/settings/inject/FeatureDebugSettingsModule.kt b/feature/debug-settings/src/debug/kotlin/net/thunderbird/feature/debug/settings/inject/FeatureDebugSettingsModule.kt
--- a/feature/debug-settings/src/debug/kotlin/net/thunderbird/feature/debug/settings/inject/FeatureDebugSettingsModule.kt	(revision a036ca941adae1356fda8beb8d1fc80048578354)
+++ b/feature/debug-settings/src/debug/kotlin/net/thunderbird/feature/debug/settings/inject/FeatureDebugSettingsModule.kt	(date 1754485915733)
@@ -3,7 +3,10 @@
 import net.thunderbird.feature.debug.settings.navigation.DefaultSecretDebugSettingsNavigation
 import net.thunderbird.feature.debug.settings.navigation.SecretDebugSettingsNavigation
 import net.thunderbird.feature.debug.settings.notification.DebugNotificationSectionViewModel
+import net.thunderbird.feature.debug.settings.notification.legacy.DebugLegacyNotificationSectionContract
+import net.thunderbird.feature.debug.settings.notification.legacy.DebugLegacyNotificationSectionViewModel
 import org.koin.core.module.dsl.viewModel
+import org.koin.dsl.bind
 import org.koin.dsl.module
 
 val featureDebugSettingsModule = module {
@@ -16,4 +19,11 @@
             notificationReceiver = get(),
         )
     }
+
+    viewModel {
+        DebugLegacyNotificationSectionViewModel(
+            iconProvider = get(),
+            channelManager = get(),
+        )
+    }
 }
Index: feature/debug-settings/build.gradle.kts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/feature/debug-settings/build.gradle.kts b/feature/debug-settings/build.gradle.kts
--- a/feature/debug-settings/build.gradle.kts	(revision a036ca941adae1356fda8beb8d1fc80048578354)
+++ b/feature/debug-settings/build.gradle.kts	(date 1754485384586)
@@ -16,4 +16,9 @@
     implementation(projects.core.outcome)
     implementation(projects.feature.mail.account.api)
     implementation(projects.feature.notification.api)
+
+    // legacy.core and core.android.common are required for legacy notification tests.
+    // Must be removed once migration to the new system is done
+    implementation(projects.core.android.common)
+    implementation(projects.legacy.core)
 }
Index: feature/debug-settings/src/main/kotlin/net/thunderbird/feature/debug/settings/SecretDebugSettingsScreen.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/feature/debug-settings/src/main/kotlin/net/thunderbird/feature/debug/settings/SecretDebugSettingsScreen.kt b/feature/debug-settings/src/main/kotlin/net/thunderbird/feature/debug/settings/SecretDebugSettingsScreen.kt
--- a/feature/debug-settings/src/main/kotlin/net/thunderbird/feature/debug/settings/SecretDebugSettingsScreen.kt	(revision a036ca941adae1356fda8beb8d1fc80048578354)
+++ b/feature/debug-settings/src/main/kotlin/net/thunderbird/feature/debug/settings/SecretDebugSettingsScreen.kt	(date 1754486141794)
@@ -1,6 +1,8 @@
 package net.thunderbird.feature.debug.settings
 
+import androidx.compose.foundation.layout.Arrangement
 import androidx.compose.foundation.layout.Column
+import androidx.compose.foundation.layout.fillMaxWidth
 import androidx.compose.foundation.layout.padding
 import androidx.compose.foundation.rememberScrollState
 import androidx.compose.foundation.verticalScroll
@@ -13,6 +15,7 @@
 import app.k9mail.core.ui.compose.designsystem.template.Scaffold
 import app.k9mail.core.ui.compose.theme2.MainTheme
 import net.thunderbird.feature.debug.settings.notification.DebugNotificationSection
+import net.thunderbird.feature.debug.settings.notification.legacy.DebugLegacyNotificationSection
 
 @Composable
 fun SecretDebugSettingsScreen(
@@ -38,8 +41,10 @@
                 .padding(paddingValues)
                 .verticalScroll(rememberScrollState())
                 .padding(MainTheme.spacings.double),
+            verticalArrangement = Arrangement.spacedBy(MainTheme.spacings.double),
         ) {
             DebugNotificationSection()
+            DebugLegacyNotificationSection()
         }
     }
 }

Please let me know if you have any questions

Copy link
Member

Choose a reason for hiding this comment

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

We now have a debug-settings module where we use to test notifications. The notification debugging should be done there.

Additionally, there is no need to add a new activity or a broadcast receiver to debug the notification. Please consider a different approach.

Copy link
Contributor Author

@SttApollo SttApollo Aug 9, 2025

Choose a reason for hiding this comment

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

Hi! Thanks for pointing me to the debug-settings module. I’ve removed the manual DebugNotificationActivity + receiver and added a new PushIconTestNotification entry to the System-notification dropdown in the Secret Debug Settings screen to verify NotificationIconResourceProvider for each app without touching existing notification code.
The test works in K-9 Mail. Thunderbird still shows an unrelated DI issue, so I kept that outside the scope of this PR.
Please let me know if this works or if you'd like any further adjustments!
Screen_recording_20250808_211847.webm
Screenshot 2025-08-08 at 7 01 19 PM

@SttApollo SttApollo force-pushed the refactor/common-notification-icon branch from a036ca9 to daaef4d Compare August 9, 2025 22:15
@SttApollo
Copy link
Contributor Author

Hi @rafaeltonholo - thank you for the speedy and thorough review! 🙏🏽

  1. I have ensured that my contribution passed the CI checks
  2. I was having trouble with my editor to reword the commit messages, so I ended up doing a soft reset and one latest commit, hoping that's ok.
  3. I have reviewed and addressed all of them
  4. Re: manual debug, please see my comment here

Feel free to share additional thoughts, and I'll make the updates

Copy link
Member

@rafaeltonholo rafaeltonholo left a comment

Choose a reason for hiding this comment

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

Hi @SttApollo,

I apologize for the delay in reviewing this pull request again; I haven't had much time to go through PRs recently.

Thank you for addressing the previous comments and for using the new notification system to verify the icon notification. However, before we can merge this pull request, we need to make a few adjustments:

  • We need to ensure that we only include changes that are necessary to address the problem from the specified GitHub Issue. There are several files with unrelated changes. I have left comments on those files; please revert the changes made to them.
  • You are updating the app's icon in the manifest by using a drawable instead of the mipmap version. This change has caused the badging to fail. You will need to revert both the icon change in the manifest and the badging change.
  • There are some files with code style issues that may cause the build to fail through Detekt. Please ensure you run ./gradlew spotlessCheck and ./gradlew detekt to fix all reported issues before pushing the code.

I have tested the changes, and for both K-9 and Thunderbird, the icon is updated correctly for the "Waiting for new emails" notification. We just need to address the issues mentioned above before proceeding.

Thank you!

@SttApollo SttApollo force-pushed the refactor/common-notification-icon branch from 25b4400 to c6c1978 Compare September 15, 2025 12:45
Introduces NotificationIconResourceProvider to allow different app
flavors to supply their own push notification icon.

This change includes:
- The NotificationIconResourceProvider interface.
- Implementations for K-9 Mail and Thunderbird.
- A new PushIconTestNotification in the debug settings to verify the
  provider.
- Removal of the old manual DebugNotificationActivity.

Fixes thunderbird#8932
@SttApollo SttApollo force-pushed the refactor/common-notification-icon branch from c6c1978 to 0893cdc Compare September 23, 2025 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use application icon in push notification messages
2 participants