-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
Add a homescreen widget #144
base: master
Are you sure you want to change the base?
Conversation
101e1f8
to
2c94781
Compare
8301183
to
d9b576d
Compare
d9b576d
to
ac09a01
Compare
First impression - this works crazy awesome! |
android/app/src/main/kotlin/com/nt4f04und/sweyer/MusicPlayerAppWidget.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/kotlin/com/nt4f04und/sweyer/MusicPlayerAppWidget.kt
Show resolved
Hide resolved
android/app/src/main/kotlin/com/nt4f04und/sweyer/MusicPlayerAppWidget.kt
Show resolved
Hide resolved
android/app/src/main/kotlin/com/nt4f04und/sweyer/MusicPlayerAppWidget.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious, how did you generate this preview? Can we document this process somehow somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we need to update the preview image after you added padding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious, how did you generate this preview? Can we document this process somehow somewhere?
Android emulators with an API <= 26 have a Widget Preview
app that can generate the preview. I added a a comment in the preview XML layout with a reminder and instructions on how to update it.
test/routes/home_route_test.dart
Outdated
@@ -133,6 +135,11 @@ void main() { | |||
|
|||
testWidgets('home screen - shows when permissions are granted and not searching for tracks', | |||
(WidgetTester tester) async { | |||
late AppWidgetChannelObserver appWidgetChannelObserver; | |||
await tester.runAsync(() => tester.pump()); // Wait for widget events from old app start to process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a lot of problems getting these tests to work reliably. This is a workaround because due to our singleton architecture and because tests can end before all futures complete, we were getting events from previous tests on the appWidgetChannelObserver
.
old app start
is poorly worded, other tests
might be better, I'll rephrase it.
test/routes/home_route_test.dart
Outdated
@@ -141,6 +148,9 @@ void main() { | |||
tester.getRect(find.byType(App)).height, | |||
reason: 'Player route must be offscreen', | |||
); | |||
await tester.runAsync(() => tester.pump()); // Wait for widget events from start to process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is even more confusing than the one above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reworded it. This is another workaround to let some of the callback on streams called in ContentControl.init
fire, so the widget events reach the app method channel.
I think it's this chain that needs to complete: ContentControl.init
-> MusicPlayer.instance.init
-> restoreLastSong
-> setSong
-> PlaybackControl.instance.changeSong
-> PlaybackControl.onSongChange.add
-> triggers AppWidgetControl.update
.
As a side note - we probably would like to show title and artist in the widget, but I'm in favor of doing this in a separate PR |
Co-authored-by: nt4f04uNd <nt4f04uNd@gmail.com>
<resources> | ||
<dimen name="musicPlayerWidgetButtonSize">48dp</dimen> | ||
<dimen name="appWidgetRadius">16dp</dimen> | ||
<dimen name="appWidgetInnerRadius">8dp</dimen> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do they differ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also help me understand what do these radiuses practically mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is part of the rounded corner implementation, see the docs.
The appWidgetRadius
refers to the corner radius of the background element of the widget (in our case it's the album art), and the appWidgetInnerRadius
is the corner radius of any foreground element in the widget (in our case it's the button bar).
Prior to Android 31 there was no guideline about these, but on 31 and up they are given by the launcher via @android:dimen/system_app_widget_background_radius
and @android:dimen/system_app_widget_inner_radius
, and the description of system_app_widget_inner_radius
says:
This is exactly 8 dp less than the background radius, to align nicely when using an 8 dp padding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed appWidgetRadius
to appWidgetBackgroundRadius
which hopefully makes it a bit clearer.
android/app/src/main/res/drawable/music_player_widget_button_bar_background.xml
Outdated
Show resolved
Hide resolved
While I was testing the buld from this PR, one time after changing a song and then immediately minimizing the app to the home page, where there was a widget visible, I got the "The app is not responding notification" from the system. It was also the default art. I didn't get it anymore since, but maybe it's something to dig into. Maybe it was reported to firebase, I didn't check |
I suppose that's the error I got
It's almost an exact duplicate of the other one that we have
|
I personally am not a fan of having any text in the widget, but we can definiert provide a second widget with more information, or maybe even allow to configure it when creating the widget. |
06c3309
to
7a1dee0
Compare
intent.type = keyEvent.toString() | ||
} | ||
return if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | ||
PendingIntent.getForegroundService(context, 0, intent, flags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm looking into the ANR, but we definitely need to use getForegroundService
, because otherwise the intent will be ignored on newer Android versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we might be able to work around this issue by using the broadcast receiver that listens to media playback button presses and that starts the service for us.
During my testing this worked very well, except for one time where we failed to create a notification with an ForegroundServiceStartNotAllowedException
. This was after killing the app and I think it was caused by an extremely slow startup, which might have caused us to exceed some timeframe. But I have not been able to replicate that, it worked every other attempt.
android:layout_width="match_parent" | ||
android:layout_height="@dimen/musicPlayerWidgetButtonSize" | ||
android:layout_alignParentStart="true" | ||
android:layout_alignParentEnd="true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
android:layout_width="match_parent" | |
android:layout_height="@dimen/musicPlayerWidgetButtonSize" | |
android:layout_alignParentStart="true" | |
android:layout_alignParentEnd="true" | |
android:layout_width="wrap_content" | |
android:layout_height="@dimen/musicPlayerWidgetButtonSize" | |
android:layout_centerHorizontal="true" |
We could also not stretch the button bar over the entire width, which would look like this:
Would you prefer that?
Depends on #130.
Adds a adaptable homescreen widget. Closes #111.
Pictures