-
Notifications
You must be signed in to change notification settings - Fork 226
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 episode screen and associated functionality to watch app #821
Conversation
TextP50( | ||
text = podcast.title, | ||
maxLines = 1, | ||
color = Color(0xFFDADCE0), |
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 think I need to create a watch theme similar to what we have for the automotive app (but with compose in mind) to handle these colors. I'll do that in a separate PR though.
// FIXME this is a hack to avoid an issue where this listener says downloads | ||
// on the watch app are enqueued when they are actually still running. | ||
val queriedState = workManager.getWorkInfoById(workInfo.id).get().state | ||
if (Util.isWearOs(context) && queriedState == WorkInfo.State.RUNNING) { | ||
getRequirementsAsync(episode) | ||
} else { | ||
getRequirementsAndSetStatusAsync(episode) | ||
} |
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 don't really have a solid understanding of why this is happening on the watch app. Any thoughts, suggestions you might have would be appreciated.
At a high level, the DownloadEpisodeTask
is what does the work, and while it is waiting on the download, the DownloadManagerImpl
's listener (in ::beginMonitoringWorkManager
) recieves an "enqueued" event, even though the download is not actually enqueued and is actively running in the DownloadEpisodeTask
's download()...blockingAwait()
call (as can be seen by checking workManger.getWorkInfoById
like I do here).
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 trying to understand this part. Maybe @geekygecko can explain it.
@@ -470,6 +484,10 @@ class DownloadManagerImpl @Inject constructor( | |||
} | |||
|
|||
private fun updateNotification() { | |||
|
|||
// Don't show these notifications on wear os | |||
if (Util.isWearOs(context)) return |
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 don't have a good feel for how to use notifications on the watch yet. It feels like we shouldn't use them as much as we do in the app, so I've disabled this one for now (also, it's not working on the watch currently), but perhaps we'll want to add it back later. Let me know if you have any thoughts here.
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.
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.
👍 Thanks for that. I'll make this update in a separate PR since this one is already way too big.
if (!Util.isWearOs(context)) { | ||
setForegroundAsync(createForegroundInfo()) | ||
} |
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 throwing an error on the watch because it tries to create a foreground notification, and notifications aren't working currently.
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.
It should be resolved by setting up notification channels, haven't tested it though.
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 bet you're right. I'll enable notifications in a separate PR.
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.
Great job, @mattchowning! 🥇
I haven’t matched UI with Figma as it is still evolving. I don't have much feedback at this point, just that app crashes on playing an episode after it is marked played:
I/Timber$Forest: Playback: Play now: 4b573437-5881-44b1-813c-3dbe5c8abf7c 194: The Top 6 Ways To Come Up With Your Business Idea
E/AndroidRuntime: FATAL EXCEPTION: main
Process: au.com.shiftyjelly.pocketcasts.debug, PID: 19340
java.lang.IllegalStateException: Cannot access database on the main thread since it may potentially lock the UI for a long period of time.
at androidx.room.RoomDatabase.assertNotMainThread(RoomDatabase.kt:435)
at androidx.room.SharedSQLiteStatement.assertNotMainThread(SharedSQLiteStatement.kt:52)
at androidx.room.SharedSQLiteStatement.acquire(SharedSQLiteStatement.kt:74)
at au.com.shiftyjelly.pocketcasts.models.db.dao.EpisodeDao_Impl.unarchive(EpisodeDao_Impl.java:1169)
at au.com.shiftyjelly.pocketcasts.repositories.podcast.EpisodeManagerImpl.unarchive(EpisodeManagerImpl.kt:720)
at au.com.shiftyjelly.pocketcasts.repositories.playback.PlaybackManager.playNowSync(PlaybackManager.kt:373)
at au.com.shiftyjelly.pocketcasts.repositories.playback.PlaybackManager.playNowSync$default(PlaybackManager.kt:369)
at au.com.shiftyjelly.pocketcasts.wear.ui.episode.EpisodeViewModel$play$1.invokeSuspend(EpisodeViewModel.kt:197)
device-2023-03-09-134346.mp4
@@ -470,6 +484,10 @@ class DownloadManagerImpl @Inject constructor( | |||
} | |||
|
|||
private fun updateNotification() { | |||
|
|||
// Don't show these notifications on wear os | |||
if (Util.isWearOs(context)) return |
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.
if (!Util.isWearOs(context)) { | ||
setForegroundAsync(createForegroundInfo()) | ||
} |
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.
It should be resolved by setting up notification channels, haven't tested it though.
// FIXME this is a hack to avoid an issue where this listener says downloads | ||
// on the watch app are enqueued when they are actually still running. | ||
val queriedState = workManager.getWorkInfoById(workInfo.id).get().state | ||
if (Util.isWearOs(context) && queriedState == WorkInfo.State.RUNNING) { | ||
getRequirementsAsync(episode) | ||
} else { | ||
getRequirementsAndSetStatusAsync(episode) | ||
} |
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 trying to understand this part. Maybe @geekygecko can explain it.
Well that's because you shouldn't be doing that!!! 😆 Seriously though, great catch. I fixed it in 81d847d. |
Description
Adds the episode screen to the watch app along with the associated functionality.
The
EpisodeViewModel
implementation very closely follows the logic in theEpisodeFragment
andEpisodeFragmentViewModel
.Testing Instructions
Verify that you can access the episode screen in each of the following ways:
From the episode screen, you should see the following UI:
From the episode screen, check the following functionality:
Screenshots or Screencast
Checklist
modules/services/localization/src/main/res/values/strings.xml
I have tested any UI changes...