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

rajveermalviya · 2024-10-04T19:18:08Z

(Stacked on top of #981)
(Split from #717)
Fixes: #340

PIG208

Tested this on my device and this works well. Thanks! Left some comments on the use of "URL".

PIG208 · 2024-10-22T20:04:29Z

lib/notifications/display.dart

+///
+/// For example, for a resource `@raw/chime3`, where `raw` would be the
+/// resource type and `chime3` would be the resource name it generates the
+/// following uri:


nit: Let's use URL instead.

PIG208 · 2024-10-22T20:06:21Z

lib/notifications/display.dart

+}) {
+  const packageName = 'com.zulip.flutter'; // TODO(#407)
+
+  // Uri scheme for Android resource url.


nit: Similar here and we can use URL (uppercase).

I think there are also some other places in the PR where we can use URL instead.

rajveermalviya · 2024-10-30T13:26:49Z

Thanks for the review @PIG208! Addressed them in the new revision.

gnprice

Thanks @rajveermalviya for building this, and @PIG208 for the previous review!

Generally this looks great. I appreciate how it closely follows the structure of the zulip-mobile implementation, right down to comments — all of that really helps in simplifying the review.

Comments below. Because there's a lot to read here, I haven't yet gone through the tests in the final/main commit; but I've read everything else.

gnprice · 2024-11-04T21:27:02Z

test/model/binding.dart

+  final _storedNotificationSounds = <StoredNotificationsSound>[];
+
+  /// Populates the media store with the provided entries.
+  void setupStoredNotificationSounds(List<StoredNotificationsSound> sounds) {


nit: let's be consistent about "notification sound" vs. "notifications sound", by sticking to the singular:

Suggested change

void setupStoredNotificationSounds(List<StoredNotificationsSound> sounds) {

void setupStoredNotificationSounds(List<StoredNotificationSound> sounds) {

Either phrase would be a reasonable term. In general when it's ambiguous whether to name something based on the singular or plural, we go for the singular, because that's the more canonical form of the word.

gnprice · 2024-11-04T21:51:35Z

android/app/src/main/kotlin/com/zulip/flutter/ZulipPlugin.kt

+            selectionArgs,
+            sortOrder,
+        )
+        query?.use { cursor ->


In the zulip-mobile version, if the query method returns null we handle it like this:

) ?: run { ZLog.w(TAG, "ensureInitNotificationSounds: query failed") return defaultSoundUrl }

That means we don't attempt to store our notification sounds into the directory.

In this revision, I think what will end up happening is that this method returns an empty list of already-stored sounds, just the same as it would on first launch in the normal happy case (where the query succeeds but returns an empty list of results). Then we'll attempt to store the sounds into the directory.

I'm not sure how likely it is that the query will fail. But if it does, it's not clear what the situation is — something is clearly wrong. The query method's doc says:

May return null if the underlying content provider returns null, or if it crashes.

So it seems safest to not attempt any further operations on the content provider in that case (like inserting files of our own), and best to fall back to the simple thing of using the resource directly.

In particular if we did insert in that situation, there's a risk that we'd succeed and would just be inserting duplicates, which could then pile up.

Tactically, I think what this calls for is to have this method return null in that failure case, or perhaps throw an exception. Then the caller on the Dart side can see that and abort with return defaultSoundUrl.

inserting duplicates, which could then pile up.

(This shouldn't happen, because once we successfully create the channel we won't be entering this whole sound-file-management area of code on future launches… but once things are going wrong in our interaction with the platform, maybe there's also a failure that we hit before the point of successfully creating the channel.)

or perhaps throw an exception. Then the caller on the Dart side can see that and abort with return defaultSoundUrl.

Updated with this.

gnprice · 2024-11-04T21:54:53Z

android/app/src/main/kotlin/com/zulip/flutter/ZulipPlugin.kt

+        val sortOrder = "${AudioStore._ID} ASC"
+
+        val sounds = mutableListOf<StoredNotificationsSound>()
+        val query = context.contentResolver.query(


nit: the return value of query is a cursor, not a query:

Suggested change

val query = context.contentResolver.query(

val cursor = context.contentResolver.query(

gnprice · 2024-11-04T21:58:31Z

android/app/src/main/kotlin/com/zulip/flutter/ZulipPlugin.kt

+
+                val contentUrl = ContentUris.withAppendedId(collection, id)
+                sounds.add(StoredNotificationsSound(
+                    fileName =  fileName,


nit:

Suggested change

fileName = fileName,

fileName = fileName,

gnprice · 2024-11-04T22:03:49Z

lib/notifications/display.dart

+      // If the file is one we put there, and has the name we give to our
+      // default sound, then use it as the default sound.
+      if (storedSound!.fileName == kDefaultNotificationSound.fileDisplayName
+          && storedSound.isOwner) {


Suggested change

&& storedSound.isOwner) {

&& storedSound.isOwned) {

Or could call it ownedByThisPackage.

Just "is owner", as a field on a sound, reads as if the sound is the owner of something, or an owner of something. (Even though it's not clear what that would mean.)

gnprice · 2024-11-04T22:06:08Z

lib/notifications/display.dart

+      //
+      // This does mean it's possible the file isn't the one we would have
+      // put there... but it probably is, just from a debug vs. release build
+      // of the app (because those have different package names).  And anyway,


Suggested change

// of the app (because those have different package names). And anyway,

// of the app (because those may have different package names). And anyway,

Currently zulip-flutter differs from zulip-mobile in this respect. We might change that in the future, though, so the remark can remain, just needs this hedge.

gnprice · 2024-11-04T22:07:55Z

lib/notifications/display.dart

+      // of the app (because those have different package names).  And anyway,
+      // this is a file we're supplying for the user in case they want it, not
+      // something where the app depends on it having specific content.
+      soundsToAdd.removeWhere((v) => v.fileDisplayName == storedSound.fileName);


Suggested change

soundsToAdd.removeWhere((v) => v.fileDisplayName == storedSound.fileName);

soundsToAdd.removeWhere((v) => v.fileDisplayName == storedSound.fileName);

Cool. The zulip-mobile version uses a hash map instead of a list… but given that this data structure has only a handful of elements in it anyway, there's no performance advantage to a map, so a list is fine (and is certainly simpler).

gnprice · 2024-11-04T22:24:31Z

pigeon/notifications.dart

+  /// Corresponds to `android.content.ContentResolver.query`.
+  ///
+  /// Returns the list of notification sounds present under
+  /// `Notifications/Zulip/` directory in device's shared media storage.


Suggested change

/// Corresponds to `android.content.ContentResolver.query`.

///

/// Returns the list of notification sounds present under

/// `Notifications/Zulip/` directory in device's shared media storage.

/// The list of notification sound files present under `Notifications/Zulip/`

/// in the device's shared media storage,

/// found with `android.content.ContentResolver.query`.

///

/// This is a complex ad-hoc method.

/// For detailed behavior, see its implementation.

Unlike most of our Pigeon bindings, it's not really accurate to say this "corresponds" to any particular method or class in the underlying platform APIs. Rather it bundles together our own logic atop a bunch of underlying methods.

Generally that's something we try to avoid in these Pigeon bindings — our usual preferred approach is to keep them a thin layer that just exposes the underlying APIs, so that the real logic happens in our Dart code.

Here, for pragmatic reasons, it makes a lot of sense to instead have an ad-hoc bundle like this, just the way you did. I think the key point is that there's a lot of different underlying methods that would need bindings if we took our usual approach, compared to a much smaller interface that describes what we actually need. Or maybe even more important: the cursor needs to be a persistent object through all the successive moveToNext and getString etc. calls, and maintaining that across the Dart/Kotlin boundary would be annoying.

(If we were going to be dealing with this ContentResolver query stuff again and again, then it'd start making sense to do the work to provide thin bindings to it after all, like in our usual approach.)

So this structure is good but the doc just needs to match it.

rajveermalviya · 2024-11-06T17:22:50Z

Thanks for the review @gnprice! I've pushed the revision — PTAL.

gnprice

Thanks! Those changes all look good. I've now read through the tests too; just small comments below.

Going AFK for dinner, so posting these now. The one remaining question I haven't thought through is to see if there are other cases I'd want tests for that aren't yet covered here.

gnprice · 2024-11-06T23:18:36Z

test/notifications/display_test.dart

+
+      // Override android version
+      testBinding.deviceInfoResult =
+        const AndroidDeviceInfo(sdkInt: 28, release: '10');


SDK 28 is release 9 (or "Pie"?):
https://en.wikipedia.org/wiki/Android_version_history

10 was SDK 29.

Correct, also verified on the emulator that the reported release string is '9' (not 'Pie').

gnprice · 2024-11-07T01:19:19Z

lib/notifications/display.dart

+      // Before Android 10 Q, we don't attempt to put the sounds in shared media storage.
+      // Just use the resource file directly.
+      // TODO(android-sdk-29): Simplify this away.
+      AndroidDeviceInfo(:var sdkInt) => sdkInt <= 28,


nit:

Suggested change

// Before Android 10 Q, we don't attempt to put the sounds in shared media storage.

// Just use the resource file directly.

// TODO(android-sdk-29): Simplify this away.

AndroidDeviceInfo(:var sdkInt) => sdkInt <= 28,

// Before Android 10 Q, we don't attempt to put the sounds in shared media storage.

// Just use the resource file directly.

// TODO(android-sdk-29): Simplify this away.

AndroidDeviceInfo(:var sdkInt) => sdkInt < 29,

In general I think it's cleanest to express version conditions as a single threshold, which is the first version where the change appeared: so conditions look like "actual >= threshold" or "actual < threshold", but never "actual <= one-less-than-threshold".

That's because if things are instead expressed with two neighboring numbers (like 28 and 29 here), it starts getting harder to keep straight in one's head. I noticed this in the previous round, but was prompted to comment on it by my comment just before this one 🙂

gnprice · 2024-11-07T01:20:41Z

test/notifications/display_test.dart

+      // Override android version
+      testBinding.deviceInfoResult =
+        const AndroidDeviceInfo(sdkInt: 28, release: '10');


nit:

Suggested change

// Override android version

testBinding.deviceInfoResult =

const AndroidDeviceInfo(sdkInt: 28, release: '10');

testBinding.deviceInfoResult =

const AndroidDeviceInfo(sdkInt: 28, release: '10');

The content of this comment is already expressed clearly enough by the code just below it.

gnprice · 2024-11-07T01:25:20Z

test/notifications/display_test.dart

+      check(androidNotificationHost.takeCreatedChannels()).single
+        ..id.equals(NotificationChannelManager.kChannelId)
+        ..name.equals('Messages')
+        ..importance.equals(NotificationImportance.high)
+        ..lightsEnabled.equals(true)
+        ..soundUrl.equals(soundUrl)
+        ..vibrationPattern.isNotNull().deepEquals(
+            NotificationChannelManager.kVibrationPattern);


Suggested change

check(androidNotificationHost.takeCreatedChannels()).single

..id.equals(NotificationChannelManager.kChannelId)

..name.equals('Messages')

..importance.equals(NotificationImportance.high)

..lightsEnabled.equals(true)

..soundUrl.equals(soundUrl)

..vibrationPattern.isNotNull().deepEquals(

NotificationChannelManager.kVibrationPattern);

check(androidNotificationHost.takeCreatedChannels()).single

..soundUrl.equals(soundUrl);

This is the only part of this check that differs between these new test cases, or between them and the corresponding check in the smoke test, right?

The rest of the details of this call aren't what these test cases are meant to focus on. So leaving them out helps the reader focus their attention on the part that the test is about.

gnprice · 2024-11-07T01:26:03Z

test/notifications/display_test.dart

+      );
+
+      await NotificationChannelManager.ensureChannel();
+      check(androidNotificationHost.takeCopySoundResourceToMediaStoreCalls()).length.equals(0);


nit: line too long, with key information past 80 columns; also can simplify to isEmpty:

Suggested change

check(androidNotificationHost.takeCopySoundResourceToMediaStoreCalls()).length.equals(0);

check(androidNotificationHost.takeCopySoundResourceToMediaStoreCalls())

.isEmpty();

gnprice · 2024-11-07T01:26:46Z

test/notifications/display_test.dart

+            NotificationChannelManager.kVibrationPattern);
+    });
+
+    test('new notifications sounds are copied to media store', () async {


nit: use standardized term:

Suggested change

test('new notifications sounds are copied to media store', () async {

test('new notification sounds are copied to media store', () async {

gnprice · 2024-11-07T01:27:39Z

test/notifications/display_test.dart

+        NotificationSound.values.map((e) => StoredNotificationSound(
+          fileName: e.fileDisplayName,
+          isOwned: true,
+          contentUrl: androidNotificationHost.fakeStoredNotificationSoundUrl(e.resourceName)),
+        ).skip(1).toList()


nit:

Suggested change

NotificationSound.values.map((e) => StoredNotificationSound(

fileName: e.fileDisplayName,

isOwned: true,

contentUrl: androidNotificationHost.fakeStoredNotificationSoundUrl(e.resourceName)),

).skip(1).toList()

NotificationSound.values.skip(1).map((e) => StoredNotificationSound(

fileName: e.fileDisplayName,

isOwned: true,

contentUrl: androidNotificationHost.fakeStoredNotificationSoundUrl(e.resourceName)),

).toList()

otherwise the skip(1) gets a bit buried at the end

gnprice

OK, I've thought through that one remaining question from above and don't have anything to add — this already covers all the relevant cases I see.

(In principle I guess we might add test cases for situations like the query method on the resolver failing. But I don't know if that ever happens in practice; and conversely, testing it would require some added complication in the test binding. So I'm content to leave that untested, just relying on our reading the code and seeing that it looks right, and focus our energy on the remaining issues in our queue.)

A couple more small comments below, though, just as a result of having reread the tests more closely.

gnprice · 2024-11-07T02:41:49Z

test/model/binding.dart

+  /// Generates a fake URL for a notification sound present in media store.
+  String fakeStoredNotificationSoundUrl(String resourceName) {


Suggested change

/// Generates a fake URL for a notification sound present in media store.

String fakeStoredNotificationSoundUrl(String resourceName) {

/// A URL that the fake [copySoundResourceToMediaStore] would produce

/// for a resource with the given name.

String fakeStoredNotificationSoundUrl(String resourceName) {

On my previous read I'd missed the connection of this to the copySoundResourceToMediaStore implementation (and in fact wondered if this might better go into example_data.dart instead). On rereading these tests I see the connection, and it makes perfect sense that this is the home of this method — and this connection is important context for a reader making sense of the tests that use this method.

gnprice · 2024-11-07T02:50:16Z

test/notifications/display_test.dart

+      final defaultSoundResourceName =
+        NotificationChannelManager.kDefaultNotificationSound.resourceName;


nit: pull this out as a local for a test group, so the individual cases don't have to repeat the definition

For organizing the tests, we can follow the pattern of the existing groups in this test file, like "NotificationDisplayManager show" and "NotificationDisplayManager open":

rename the existing "NotificationChannelManager" group to a name like "NotificationChannelManager create channel"

add a group "NotificationChannelManager sounds" as a home for the new tests

gnprice · 2024-11-07T02:50:48Z

test/notifications/display_test.dart

+      final soundUrl =
+        androidNotificationHost.fakeStoredNotificationSoundUrl(defaultSoundResourceName);


nit: then also pull out a helper for this:

Suggested change

final soundUrl =

androidNotificationHost.fakeStoredNotificationSoundUrl(defaultSoundResourceName);

final soundUrl = fakeStoredUrl(defaultSoundResourceName);

just so it gets a much shorter name

gnprice · 2024-11-07T02:51:55Z

test/notifications/display_test.dart

+        ..name.equals('Messages')
+        ..importance.equals(NotificationImportance.high)
+        ..lightsEnabled.equals(true)
+        ..soundUrl.equals(soundUrl)


Then in fact with that shorter name, we can comfortably inline the expression for the expected value into this check:

Suggested change

..soundUrl.equals(soundUrl)

..soundUrl.equals(fakeStoredUrl(defaultSoundResourceName))

rajveermalviya · 2024-11-09T05:12:11Z

Thanks for the review @gnprice! Pushed a new revision, PTAL.

gnprice · 2024-11-14T20:36:02Z

Thanks for the revision! All looks good; merging.

…tory

Fixes: zulip#340

rajveermalviya mentioned this pull request Oct 4, 2024

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

Closed

rajveermalviya force-pushed the pr-notif-custom-sounds branch 3 times, most recently from d6a11af to 0327a56 Compare October 9, 2024 20:28

rajveermalviya marked this pull request as ready for review October 9, 2024 20:29

rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label Oct 9, 2024

rajveermalviya force-pushed the pr-notif-custom-sounds branch 2 times, most recently from e1b117b to fc9d6f6 Compare October 10, 2024 13:59

chrisbobbe assigned PIG208 Oct 10, 2024

chrisbobbe requested a review from PIG208 October 10, 2024 18:54

rajveermalviya force-pushed the pr-notif-custom-sounds branch from fc9d6f6 to 1305eb2 Compare October 28, 2024 20:59

PIG208 reviewed Oct 29, 2024

View reviewed changes

PIG208 added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Oct 29, 2024

PIG208 assigned gnprice and unassigned PIG208 Oct 29, 2024

rajveermalviya force-pushed the pr-notif-custom-sounds branch from 1305eb2 to 13ba923 Compare October 30, 2024 12:36

gnprice reviewed Nov 4, 2024

View reviewed changes

rajveermalviya force-pushed the pr-notif-custom-sounds branch 4 times, most recently from f531bda to e58b4a2 Compare November 6, 2024 08:19

rajveermalviya requested a review from gnprice November 6, 2024 17:22

gnprice reviewed Nov 7, 2024

View reviewed changes

rajveermalviya force-pushed the pr-notif-custom-sounds branch 2 times, most recently from 8477154 to 687e5eb Compare November 9, 2024 05:03

rajveermalviya requested a review from gnprice November 9, 2024 05:12

rajveermalviya added 4 commits November 14, 2024 12:36

pigeon: Add a helper binding for listStoredSoundsInNotificationsDirec…

2348fe0

…tory

pigeon: Add helper binding for copySoundResourceToMediaStore

5a8f4bb

pigeon: Add binding for NotificationChannel.soundUrl

9785936

notif: Use Zulip's distinct notification sound on Android

fe16ad1

Fixes: zulip#340

gnprice force-pushed the pr-notif-custom-sounds branch from 687e5eb to fe16ad1 Compare November 14, 2024 20:36

gnprice merged commit fe16ad1 into zulip:main Nov 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rajveermalviya commented Oct 4, 2024

PIG208 left a comment •

edited

Loading

PIG208 Oct 22, 2024

PIG208 Oct 22, 2024 •

edited

Loading

rajveermalviya commented Oct 30, 2024

gnprice left a comment

gnprice Nov 4, 2024

gnprice Nov 4, 2024

gnprice Nov 4, 2024

gnprice Nov 4, 2024

rajveermalviya Nov 6, 2024

gnprice Nov 4, 2024

gnprice Nov 4, 2024

gnprice Nov 4, 2024

gnprice Nov 4, 2024

gnprice Nov 4, 2024

gnprice Nov 4, 2024

rajveermalviya commented Nov 6, 2024

gnprice left a comment

gnprice Nov 6, 2024

rajveermalviya Nov 9, 2024

gnprice Nov 7, 2024

gnprice Nov 7, 2024

gnprice Nov 7, 2024

gnprice Nov 7, 2024

gnprice Nov 7, 2024

gnprice Nov 7, 2024

gnprice left a comment

gnprice Nov 7, 2024

gnprice Nov 7, 2024

gnprice Nov 7, 2024

gnprice Nov 7, 2024

rajveermalviya commented Nov 9, 2024

gnprice commented Nov 14, 2024

	void setupStoredNotificationSounds(List<StoredNotificationsSound> sounds) {
	void setupStoredNotificationSounds(List<StoredNotificationSound> sounds) {

	val query = context.contentResolver.query(
	val cursor = context.contentResolver.query(

	// of the app (because those have different package names). And anyway,
	// of the app (because those may have different package names). And anyway,

	soundsToAdd.removeWhere((v) => v.fileDisplayName == storedSound.fileName);
	soundsToAdd.removeWhere((v) => v.fileDisplayName == storedSound.fileName);

-  /// Corresponds to `android.content.ContentResolver.query`.
-  ///
-  /// Returns the list of notification sounds present under
-  /// `Notifications/Zulip/` directory in device's shared media storage.
+  /// The list of notification sound files present under `Notifications/Zulip/`
+  /// in the device's shared media storage,
+  /// found with `android.content.ContentResolver.query`.
+  ///
+  /// This is a complex ad-hoc method.
+  /// For detailed behavior, see its implementation.

	check(androidNotificationHost.takeCopySoundResourceToMediaStoreCalls()).length.equals(0);
	check(androidNotificationHost.takeCopySoundResourceToMediaStoreCalls())
	.isEmpty();

	test('new notifications sounds are copied to media store', () async {
	test('new notification sounds are copied to media store', () async {

		/// Generates a fake URL for a notification sound present in media store.
		String fakeStoredNotificationSoundUrl(String resourceName) {

		final defaultSoundResourceName =
		NotificationChannelManager.kDefaultNotificationSound.resourceName;

		final soundUrl =
		androidNotificationHost.fakeStoredNotificationSoundUrl(defaultSoundResourceName);

	final soundUrl =
	androidNotificationHost.fakeStoredNotificationSoundUrl(defaultSoundResourceName);
	final soundUrl = fakeStoredUrl(defaultSoundResourceName);

	..soundUrl.equals(soundUrl)
	..soundUrl.equals(fakeStoredUrl(defaultSoundResourceName))

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

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

Conversation

rajveermalviya commented Oct 4, 2024

PIG208 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

rajveermalviya commented Oct 30, 2024

gnprice 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Nov 6, 2024

gnprice 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

gnprice 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 Nov 9, 2024

gnprice commented Nov 14, 2024

PIG208 left a comment •

edited

Loading

PIG208 Oct 22, 2024 •

edited

Loading