App option to toggle showing podcast artwork embedded in show notes by owenlejeune · Pull Request #1854 · Automattic/pocket-casts-android

owenlejeune · 2024-02-17T17:24:10Z

Description

Recently, a change was made to show artwork that is embedded in a podcast episode's show notes by default. This adds a toggle to appearance settings that functions similarly to the existing toggle for embedded artwork.

Fixes #1709

Testing Instructions

Begin playing an episode that includes artwork embedded in show notes (eg. https://pcast.pocketcasts.net/podcast/ede41160-9eeb-012f-3e7d-525400c11844)
Observe that the default podcast artwork is shown in both the miniplayer and fullscreen player.
Navigate to Settings -> Appearance and toggle "Use episode artwork"
Observe that embedded artwork is show in fullscreen and miniplayer (playback state may need to be toggled for miniplayer to be updated)

Screenshots or Screencast

Checklist

If this is a user-facing change, I have added an entry in CHANGELOG.md
Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
I have considered whether it makes sense to add tests for my changes
All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
Any jetpack compose components I added or changed are covered by compose previews

I have tested any UI changes...

with different themes
with a landscape orientation
with the device set to have a large display and font size
for accessibility with TalkBack

owenlejeune · 2024-02-17T18:11:41Z

@MiSikora I'm happy to make any changes to the strings if you have better suggestions as well

CookieyedCodes · 2024-02-17T19:15:37Z

I'd say instead of calling it episode artwork I would call it RSS episode artwork 🤔🤔 and call Embedded artwork MP3 artwork 🤔🤔, my two cents and thus exclude the extra line 😉

owenlejeune · 2024-02-17T22:45:12Z

@CookieyedCodes that's not a bad idea, I agree that it might make sense to change the copy for "embedded" artwork to distinguish between RSS and MP3 artwork

owenlejeune · 2024-02-18T19:02:56Z

Updated the copy to better distinguish between MP3 and RSS artwork

MiSikora

Thanks for the contribution! I left some comments. There's also two bugs that I found.

Mini player artwork doesn't update with the setting. You can see that when the player is paused toggling the setting doesn't affect the mini player artwork. When the player is playing it works sometimes. I assume it has something to do with the state but I haven't investigated it.

screen-20240221-122019.mp4

Notification center always uses RSS artwork.

screen-20240221-122243.mp4

owenlejeune · 2024-02-22T00:44:22Z

@MiSikora I address the comments you made. One note is that the notification artwork doesn't seem to be reflected immediately if a podcast is playing, but is respected if the podcast is paused. If a podcast is playing when this setting is changed, the change will be reflected when the user changes the playback state. I'm not sure why it's not reflected immediately. I suspect this is not a setting that users will be changing frequently so I'm not sure if that's a blocker or not.

Untitled.mov

MiSikora · 2024-02-22T05:04:18Z

I address the comments you made. One note is that the notification artwork doesn't seem to be reflected immediately if a podcast is playing, but is respected if the podcast is paused. If a podcast is playing when this setting is changed, the change will be reflected when the user changes the playback state. I'm not sure why it's not reflected immediately. I suspect this is not a setting that users will be changing frequently so I'm not sure if that's a blocker or not.

Yeah, this is perfectly fine. 👍

Co-authored-by: Michał Sikora <[email protected]>

MiSikora

I won't be able to test it until Monday but code-wise it looks good. I just left two minor comments.

MiSikora · 2024-02-26T08:21:13Z

The miniplayer is now broken.

Miniplayer artwork doesn't toggle.

screen-20240226-084243.mp4

Miniplayer doesn't switch artwork at all. Even when playing a different podcast.

screen-20240226-084535.mp4

Miniplayer doesn't close.

screen-20240226-085051.mp4

~~Another issue is with the chapter artwork. I'm not 100% if this is should happen but I personally would expect this toggle to disable the chapter artwork as well.~~ No changes need. See #1854 (comment) for an update.

screen-20240226-085250.mp4

CookieyedCodes · 2024-02-26T09:13:17Z

From my understanding the toggle only applys to the next podcast so it makes sence the artwork cache is updated for the miniplayer when it changes episodes 🤔

Miniplayer should close tho hmmmm

Well chapter artwork is embedded in the audio file so while it probably should disable it would be a completely different toggle in my opinion

MiSikora · 2024-02-26T09:29:58Z

From my understanding the toggle only applys to the next podcast so it makes sence the artwork cache is updated for the miniplayer when it changes episodes 🤔

Sorry, I don't understand your point. Are you agreeing with me that it is an issue that the miniplayer doesn't update the artwork when a different episode/podcast is played?

Well chapter artwork is embedded in the audio file so while it probably should disable it would be a completely different toggle in my opinion

If that's the case we should leave this behavior as is. 👍

CookieyedCodes · 2024-02-26T09:38:51Z

Regarding the confusion, from my understanding the miniplayer artwork caches when the episode loaded not during the episode (same with the player from my understanding, which is probably harder to fix tbh) just saying why it may be happening 🤔 🤔

MiSikora · 2024-02-26T09:50:48Z

It might be caching but this change introduced a flag that changes the main player artwork on being toggled. You can see this in my first video in here - #1854 (review). If the main player respects the toggle with live updates so should the mini player. I'm fine with notification center lagging a bit because it is a different beast and would require probably many more changes.

CookieyedCodes · 2024-02-26T09:54:50Z

Agreed 👍👍

owenlejeune · 2024-02-26T14:34:20Z

@MiSikora that's really odd, in my testing I'm pretty sure I was seeing the mini player artwork being updated with that toggle as expected. I'll take another look sometime today

owenlejeune · 2024-02-27T02:54:00Z

@MiSikora I was able to repro your issues. Looks like this was a Coroutine Context issue. Flows in MainActivity were launched using Dispatchers.Default while the mini player uses Dispatchers.Main. It looks like this combined flow is the only member of MainActivity using the coroutine context so I changed it to Dispatchers.Main and I'm not longer seeing any issues with the mini player.

Approved a wrong PR. I will review once again after the changes are made.

MiSikora

LGTM! Thanks for this PR.

You need to sync with the main branch and move your changelog entry to the 7.59 release.

owenlejeune · 2024-02-27T17:34:24Z

@MiSikora sync and CHANGELOG update are complete

add option to toggle podcast artwork embedded in shownotes

c10c655

owenlejeune requested a review from a team as a code owner February 17, 2024 17:24

owenlejeune requested a review from MiSikora February 17, 2024 17:24

update CHANGELOG.md

0900d75

update copy to better distinguish between MP3 and RSS artwork

40f7064

Merge branch 'main' into episode_artwork

e703183

MiSikora requested changes Feb 21, 2024

View reviewed changes

owenlejeune added 2 commits February 21, 2024 06:39

rename episode artwork to RSS artwork and remove server settings

fbe4b0b

update notification image to respect RSS image setting

640887d

MiSikora requested changes Feb 22, 2024

View reviewed changes

owenlejeune and others added 3 commits February 22, 2024 10:18

Update modules/services/localization/src/main/res/values/strings.xml

c87d35e

Co-authored-by: Michał Sikora <[email protected]>

remove unneeded sync task param

3d3d7a3

replace observing rss artwork settings changes with kotlin flows

f03b0ac

MiSikora requested changes Feb 23, 2024

View reviewed changes

Comment thread ...positories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/UpNextQueue.kt Outdated

Comment thread ...positories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/UpNextQueue.kt Outdated

remove leftover dubugging code

9c87571

fix mini player issues

78c5c6b

MiSikora requested changes Feb 27, 2024

View reviewed changes

This comment was marked as off-topic.

Sign in to view

code cleanup

0411236

MiSikora approved these changes Feb 27, 2024

View reviewed changes

MiSikora added the [Type] Enhancement Improve an existing feature. label Feb 27, 2024

MiSikora added this to the 7.59 milestone Feb 27, 2024

owenlejeune added 2 commits February 27, 2024 11:32

Merge branch 'Automattic:main' into episode_artwork

96675ff

move change to 7.59 in changelog

fc22db9

MiSikora reviewed Feb 27, 2024

View reviewed changes

Comment thread CHANGELOG.md Outdated

Update CHANGELOG.md

249f042

MiSikora mentioned this pull request Feb 27, 2024

Dummy PR #1889

Closed

9 tasks

MiSikora merged commit e026b94 into Automattic:main Feb 27, 2024

owenlejeune deleted the episode_artwork branch February 27, 2024 20:38

MiSikora mentioned this pull request Mar 19, 2024

Podcast Standard Tags #1955

Closed

Conversation

owenlejeune commented Feb 17, 2024

Description

Testing Instructions

Screenshots or Screencast

Checklist

I have tested any UI changes...

Uh oh!

owenlejeune commented Feb 17, 2024

Uh oh!

CookieyedCodes commented Feb 17, 2024

Uh oh!

owenlejeune commented Feb 17, 2024

Uh oh!

owenlejeune commented Feb 18, 2024

Uh oh!

MiSikora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

owenlejeune commented Feb 22, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MiSikora commented Feb 22, 2024

Uh oh!

MiSikora left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MiSikora commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CookieyedCodes commented Feb 26, 2024

Uh oh!

MiSikora commented Feb 26, 2024

Uh oh!

CookieyedCodes commented Feb 26, 2024

Uh oh!

MiSikora commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CookieyedCodes commented Feb 26, 2024

Uh oh!

owenlejeune commented Feb 26, 2024

Uh oh!

owenlejeune commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as off-topic.

Uh oh!

MiSikora left a comment

Choose a reason for hiding this comment

Uh oh!

owenlejeune commented Feb 27, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

MiSikora left a comment •

edited

Loading

MiSikora commented Feb 26, 2024 •

edited

Loading

MiSikora commented Feb 26, 2024 •

edited

Loading

owenlejeune commented Feb 27, 2024 •

edited

Loading