notif: Use Zulip's notification sound on Android #717

rajveermalviya · 2024-06-02T19:16:01Z

Updates: #340

For now, only includes the default sound, i.e. it doesn't show multiple sound options in notification settings - this will be done in later PRs.

gnprice · 2024-06-03T06:48:17Z

lib/notifications/display.dart

+
+    // Delete any older channels.
+    var found = false;
+    final channels = await androidPlugin.getNotificationChannels();


Rather than add more parts of the package:flutter_local_notifications API that we use, let's use Pigeon for this. That way in order to be confident of understanding what the code is doing, we only have to read our own code and the Android docs, and not also the flutter_local_notifications code 🙂 (which often introduces a lot of logic of its own).

As a concrete strategy, let's put this issue after #351: so we convert the existing call sites first (the remaining ones, after #592 covered the most complex of them), and then we go on to add Pigeon bindings for more of the notifications API.

sm-sayedi

Thanks @rajveermalviya for this. A few small nits below, otherwise LGTM! I have also tested it on my phone, and the notification sound works fine.

As this PR was opened during the official GSoC period, let's move on to the mentor review with @sumanthvrao.

sm-sayedi · 2024-08-31T04:37:33Z

android/app/src/main/kotlin/com/zulip/flutter/Notifications.g.kt

+   *
+   * The resource ID is retrieved using
+   * `android.content.res.Resources.getIdentifier`, which is then used to
+   * generate a `android.resource://` URI. This URI is then passed


Suggested change

* generate a `android.resource://` URI. This URI is then passed

* generate an `android.resource://` URI. This URI is then passed

sm-sayedi · 2024-08-31T04:40:05Z

lib/host/android_notifications.g.dart

+  ///
+  /// The resource ID is retrieved using
+  /// `android.content.res.Resources.getIdentifier`, which is then used to
+  /// generate a `android.resource://` URI. This URI is then passed


Suggested change

/// generate a `android.resource://` URI. This URI is then passed

/// generate an `android.resource://` URI. This URI is then passed

sm-sayedi · 2024-08-31T04:40:30Z

pigeon/notifications.dart

+  ///
+  /// The resource ID is retrieved using
+  /// `android.content.res.Resources.getIdentifier`, which is then used to
+  /// generate a `android.resource://` URI. This URI is then passed


Suggested change

/// generate a `android.resource://` URI. This URI is then passed

/// generate an `android.resource://` URI. This URI is then passed

rajveermalviya · 2024-09-11T16:31:50Z

Thanks for the review @sm-sayedi, pushed a new revision.

PIG208

Thanks for the PR! Tested manually and it works well for me. Left some comments.

PIG208 · 2024-09-17T20:23:38Z

lib/notifications/display.dart

+  static const kChannelId = 'messages-2';
+
+  @visibleForTesting
+  static const kDefaultSoundResourceName = 'chime3'; // 'Zulip - Chime.m4a'


We should add a comment similar to the one in 785df22.

PIG208 · 2024-09-20T18:10:48Z

pigeon/notifications.dart

+  /// The resource ID is retrieved using
+  /// `android.content.res.Resources.getIdentifier`, which is then used to
+  /// generate an `android.resource://` URI. This URI is then passed
+  /// to the `NotificationChannelCompat` builder.


We could rephrase this to something similar to

zulip-flutter/pigeon/notifications.dart

Lines 172 to 175 in cdbe141

/// The `smallIconResourceName` is passed to `android.content.res.Resources.getIdentifier`

/// to get a resource ID to pass to `Builder.setSmallIcon`.

/// Whatever name is passed there must appear in keep.xml too:

/// see https://github.com/zulip/zulip-flutter/issues/528 .

, mentioning how soundResourceName is expected to be known in keep.xml.

PIG208 · 2024-09-20T18:13:45Z

lib/notifications/display.dart

+    }
+
+    // The channel doesn't exist.  Create it.
+


nit: remove empty line

PIG208 · 2024-09-20T18:23:04Z

lib/notifications/display.dart

+    var found = false;
+    final channels = await _androidHost.getNotificationChannels();
+    for (final channel in channels) {
+      assert(channel != null); // TODO(flutter#97848)


nit: we can refer to #942 here

Updates: zulip#340

rajveermalviya · 2024-10-04T19:28:00Z

I've split this PR into separate ones: #981 for the notification migration and #982 to support custom notification sounds. The reason for this is that unlike the current PR, #982 fully resolves issue #340 by allowing users to select multiple notification sounds from the settings. Because the full implementation commits became substantial, it seemed like it need to be split in separate PRs.

Thus closing this in favor of #981 and #982.

gnprice reviewed Jun 3, 2024

View reviewed changes

gnprice marked this pull request as draft June 4, 2024 00:44

rajveermalviya force-pushed the notif-sound branch from aafef49 to 0ffeefa Compare June 6, 2024 09:45

rajveermalviya force-pushed the notif-sound branch from 0ffeefa to eaf25e6 Compare July 8, 2024 13:07

rajveermalviya force-pushed the notif-sound branch 3 times, most recently from c82baa1 to a018fe3 Compare August 23, 2024 22:43

rajveermalviya marked this pull request as ready for review August 23, 2024 22:43

rajveermalviya added the buddy review GSoC buddy review needed. label Aug 23, 2024

rajveermalviya requested a review from sm-sayedi August 23, 2024 22:44

rajveermalviya force-pushed the notif-sound branch from a018fe3 to 4020759 Compare August 23, 2024 23:40

sm-sayedi approved these changes Sep 2, 2024

View reviewed changes

sm-sayedi added mentor review GSoC mentor review needed. and removed buddy review GSoC buddy review needed. labels Sep 2, 2024

sm-sayedi requested a review from sumanthvrao September 2, 2024 10:35

gnprice added the maintainer review PR ready for review by Zulip maintainers label Sep 10, 2024

rajveermalviya force-pushed the notif-sound branch 3 times, most recently from c5d12fb to 16b9f47 Compare September 11, 2024 16:28

gnprice requested a review from PIG208 September 12, 2024 18:51

gnprice assigned PIG208 Sep 12, 2024

PIG208 reviewed Sep 20, 2024

View reviewed changes

rajveermalviya added 5 commits September 28, 2024 01:06

pigeon: Add binding for getNotificationChannels

2771f58

pigeon: Add binding for deleteNotificationChannel

7eef966

pigeon: Add binding for NotificationChannel.soundResourceName

dcb3b36

notif: Add setup for notification channel migrations

2af661c

notif: Use Zulip's notification sound on Android

ca1eefd

Updates: zulip#340

rajveermalviya force-pushed the notif-sound branch from 16b9f47 to ca1eefd Compare September 27, 2024 19:36

rajveermalviya mentioned this pull request Oct 4, 2024

notif: Support migration of Android notification channels #981

Merged

rajveermalviya mentioned this pull request Oct 4, 2024

notif: Use Zulip's custom notification sound on Android #982

Merged

rajveermalviya closed this Oct 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

notif: Use Zulip's notification sound on Android #717

notif: Use Zulip's notification sound on Android #717

rajveermalviya commented Jun 2, 2024 •

edited

Loading

gnprice Jun 3, 2024

sm-sayedi left a comment •

edited

Loading

sm-sayedi Aug 31, 2024

sm-sayedi Aug 31, 2024

sm-sayedi Aug 31, 2024

rajveermalviya commented Sep 11, 2024

PIG208 left a comment

PIG208 Sep 17, 2024

PIG208 Sep 20, 2024

PIG208 Sep 20, 2024

PIG208 Sep 20, 2024

rajveermalviya commented Oct 4, 2024

	* generate a `android.resource://` URI. This URI is then passed
	* generate an `android.resource://` URI. This URI is then passed

	/// generate a `android.resource://` URI. This URI is then passed
	/// generate an `android.resource://` URI. This URI is then passed

	/// The `smallIconResourceName` is passed to `android.content.res.Resources.getIdentifier`
	/// to get a resource ID to pass to `Builder.setSmallIcon`.
	/// Whatever name is passed there must appear in keep.xml too:
	/// see https://github.com/zulip/zulip-flutter/issues/528 .

notif: Use Zulip's notification sound on Android #717

notif: Use Zulip's notification sound on Android #717

Conversation

rajveermalviya commented Jun 2, 2024 • edited Loading

Choose a reason for hiding this comment

sm-sayedi 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

Choose a reason for hiding this comment

rajveermalviya commented Sep 11, 2024

PIG208 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

rajveermalviya commented Oct 4, 2024

rajveermalviya commented Jun 2, 2024 •

edited

Loading

sm-sayedi left a comment •

edited

Loading