Implementation of App.beep() for Android #2068

t-arn · 2023-08-07T13:12:09Z

This PR implements App.beep() for Android
The "window" example has been extended to demo this feature.

PR Checklist:

All new features have been tested
All new features have been documented
I have read the CONTRIBUTING.md file
I will abide by the code of conduct

t-arn · 2023-08-07T14:23:57Z

@freakboy3742 Please review this PR

mhsmith · 2023-08-07T14:31:59Z

FYI: rather than waiting for GitHub to run the pre-commit checks, you can set it up on your own machine as shown here.

t-arn · 2023-08-07T15:24:57Z

@mhsmith Thanks for the hint :-)

freakboy3742

When I run this on the Briefcase default emulator, I get:

D/Ringtone: Successfully created local player
E/FileSource: Failed to open file 'content://settings/system/notification_sound'. (No such file or directory)
E/AudioManager: hasHapticChannels failure:java.io.IOException: Failed to instantiate extractor.

This looks like it might be a limitation of the emulator - can you confirm whether you're seeing this as well?

From poking around looking for a quick fix for this, I also saw some suggestions that you should use MediaPlayer, rather than RingtoneManager to play sounds like this, because MediaPlayer will play over any existing sounds (e.g., music playing in the background), rather than interrupting audio.

freakboy3742 · 2023-08-08T00:06:19Z

examples/window/window/app.py

+        self.scroller = toga.ScrollContainer(
+            horizontal=False,
+            vertical=True,
+            style=Pack(flex=1, padding=10),


This puts the padding on the outside of the scroll container, so there's a border around the scrolling content. We don't need any padding there.

ok, removed padding.

freakboy3742 · 2023-08-08T00:07:28Z

examples/window/window/app.py

+            style=Pack(flex=1, padding=10),
+        )
+        self.scroller.content = self.inner_box
+        self.main_box.add(self.scroller)


The widget layout you've got here is: content -> main_box -> scroller -> inner_box -> *buttons

AFAICT, the main box isn't adding anything. You should be able to add the scroller directly as the main content.

ok, removed main_box and renamed scroller to main_scroller, so it becomes obvious that the scroller represents the main content.

t-arn · 2023-08-08T05:28:38Z

E/FileSource: Failed to open file 'content://settings/system/notification_sound'. (No such file or directory)

This is strange. It works on my emulator and I had the code running on my phone as well (in pyPlayground).
I'll try to run the example on the phone again.

freakboy3742 · 2023-08-08T08:50:24Z

FWIW: It does beep on my physical Android device; it's only my emulator where it raises an error. Even there, the app doesn't crash - it's just a bit noisy in the logs. If we can establish that this is a quirk of (some) emulators, I can live with that; if we can catch the error and log something a little more helpful, that's even better.

t-arn · 2023-08-19T06:38:08Z

From poking around looking for a quick fix for this, I also saw some suggestions that you should use MediaPlayer, rather than RingtoneManager to play sounds like this, because MediaPlayer will play over any existing sounds (e.g., music playing in the background), rather than interrupting audio.

I think that interrupting existing sounds is what we want in this case: the user should get notified. If we play the beep over the running sound, it might go unnoticed by the user.

t-arn · 2023-08-19T07:18:25Z

@freakboy3742 Please re-review this PR.

mhsmith · 2023-08-19T14:12:10Z

I have a few devices of various ages, so I can test this one.

mhsmith

I got the same "Failed to open file" error on the Briefcase default emulator with API level 31, and a Pixel 7 with API level 33. But in both cases, the sound still worked, so I think the message is harmless. It's not throwing an exception to us, so there's no way to suppress it.

It also works on an older device and emulator. However, I have noticed that emulators sometimes stop producing any sound at all. You can usually get it back by rebooting the emulator:

Close the emulator.
Open Android Studio and go to Tools > Device Manager.
Open the ellipsis menu next to the emulator in question, and choose "Cold boot now".

The interaction with background music seems reasonable: the notification is clearly audible, but it doesn't interrupt the music any more than necessary.

So I'm fine with this, except for one thing:

mhsmith · 2023-08-19T15:37:21Z

android/src/toga_android/app.py

+        uri_obj = RingtoneManager.getDefaultUri(RingtoneManager.TYPE_NOTIFICATION)
+        uri = java.cast(Uri, uri_obj)
+        ring_obj = RingtoneManager.getRingtone(self.native.getApplicationContext(), uri)
+        ring = java.cast(Ringtone, ring_obj)
+        ring.play()


I don't think these cast calls are necessary. Why did you add them?

You are right: we already get the correct class instances. Somehow, I thought we get Objects back... I'll fix this.

t-arn · 2023-08-19T16:55:51Z

I got the same "Failed to open file" error on the Briefcase default emulator with API level 31, and a Pixel 7 with API level 33. But in both cases, the sound still worked, so I think the message is harmless. It's not throwing an exception to us, so there's no way to suppress it.

Do you also have the error message on real devices?

mhsmith · 2023-08-19T17:46:01Z

I got the error message on a real Pixel 7 with API level 33, but not on a Nexus 5X with API level 27. The sound worked on both devices.

t-arn · 2023-08-23T18:38:20Z

I got the error message on a real Pixel 7 with API level 33, but not on a Nexus 5X with API level 27. The sound worked on both devices.

Yes, the problem with the error message seems to depend on the Android version. With API level 30, there is no error message. Starting with API level 31, the error message appears. The peep sound works on both API levels.

t-arn · 2023-08-23T20:19:38Z

@mhsmith
I managed to get rid of the error message on Android 13 :-)
Please re-review.

freakboy3742

Looks good; after restarting the emulator as suggested by @mhsmith, I'm now getting a beep (or a twinkle, or whatever you want to call that sound :-).

Interestingly, I'm not getting any file errors any more - I get a warning about offset/length being adjusted:

W/FileSource: offset/length adjusted from 0/576460752303423487 to 0/16905

but the audio plays fine, so I'll call that a win.

I've renamed the changefile to a misc - since the beep() API was added in this release, it will already have a feature indicator (#2018); we don't need a separate feature for Android.

t-arn added 5 commits August 7, 2023 14:59

added App.beep() for Android; updated example "window"

4955534

added newsfragment

476e4a2

fix pre-commit check

11f77da

fix pre-commit check

024a00f

fix pre-commit check

ca78120

freakboy3742 requested changes Aug 8, 2023

View reviewed changes

applied changes requested in the review

bf84b96

mhsmith reviewed Aug 19, 2023

View reviewed changes

removed unnecessary casts

0180452

fixes the error message which appeared during beep()

a088231

Rename 2068.feature.rst to 2068.misc.rst

608ec53

freakboy3742 approved these changes Aug 23, 2023

View reviewed changes

freakboy3742 merged commit 127e8f7 into beeware:main Aug 23, 2023
40 checks passed

t-arn deleted the beep_android_1744a34 branch August 24, 2023 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of App.beep() for Android #2068

Implementation of App.beep() for Android #2068

t-arn commented Aug 7, 2023 •

edited by mhsmith

Loading

t-arn commented Aug 7, 2023

mhsmith commented Aug 7, 2023

t-arn commented Aug 7, 2023

freakboy3742 left a comment

freakboy3742 Aug 8, 2023

t-arn Aug 19, 2023

freakboy3742 Aug 8, 2023

t-arn Aug 19, 2023

t-arn commented Aug 8, 2023

freakboy3742 commented Aug 8, 2023

t-arn commented Aug 19, 2023

t-arn commented Aug 19, 2023

mhsmith commented Aug 19, 2023

mhsmith left a comment

mhsmith Aug 19, 2023

t-arn Aug 19, 2023

t-arn commented Aug 19, 2023

mhsmith commented Aug 19, 2023

t-arn commented Aug 23, 2023 •

edited

Loading

t-arn commented Aug 23, 2023 •

edited

Loading

freakboy3742 left a comment

Implementation of App.beep() for Android #2068

Implementation of App.beep() for Android #2068

Conversation

t-arn commented Aug 7, 2023 • edited by mhsmith Loading

PR Checklist:

t-arn commented Aug 7, 2023

mhsmith commented Aug 7, 2023

t-arn commented Aug 7, 2023

freakboy3742 left a comment

Choose a reason for hiding this comment

freakboy3742 Aug 8, 2023

Choose a reason for hiding this comment

t-arn Aug 19, 2023

Choose a reason for hiding this comment

freakboy3742 Aug 8, 2023

Choose a reason for hiding this comment

t-arn Aug 19, 2023

Choose a reason for hiding this comment

t-arn commented Aug 8, 2023

freakboy3742 commented Aug 8, 2023

t-arn commented Aug 19, 2023

t-arn commented Aug 19, 2023

mhsmith commented Aug 19, 2023

mhsmith left a comment

Choose a reason for hiding this comment

mhsmith Aug 19, 2023

Choose a reason for hiding this comment

t-arn Aug 19, 2023

Choose a reason for hiding this comment

t-arn commented Aug 19, 2023

mhsmith commented Aug 19, 2023

t-arn commented Aug 23, 2023 • edited Loading

t-arn commented Aug 23, 2023 • edited Loading

freakboy3742 left a comment

Choose a reason for hiding this comment

t-arn commented Aug 7, 2023 •

edited by mhsmith

Loading

t-arn commented Aug 23, 2023 •

edited

Loading

t-arn commented Aug 23, 2023 •

edited

Loading