Skip to content
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

feat: Add now playing info for video #1324

Merged
merged 22 commits into from
Dec 5, 2024

Conversation

Matthieu-dgl
Copy link
Contributor

No description provided.

@Matthieu-dgl Matthieu-dgl self-assigned this Oct 15, 2024
@Matthieu-dgl Matthieu-dgl changed the title Feat add now playing info for video feat: Add now playing info for video Oct 15, 2024
@Matthieu-dgl Matthieu-dgl marked this pull request as draft October 15, 2024 13:20
@Matthieu-dgl Matthieu-dgl marked this pull request as ready for review October 16, 2024 06:43
@Matthieu-dgl Matthieu-dgl force-pushed the feat-add-now-playing-info-for-video branch 3 times, most recently from 696ce86 to c92cac3 Compare October 18, 2024 08:20
@Matthieu-dgl Matthieu-dgl force-pushed the feat-add-now-playing-info-for-video branch from 915becc to 5a6c505 Compare October 18, 2024 12:05
@Matthieu-dgl Matthieu-dgl force-pushed the feat-add-now-playing-info-for-video branch from 5a6c505 to b2e12e0 Compare October 28, 2024 06:55
Copy link
Member

@PhilippeWeidmann PhilippeWeidmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You only observe .AVPlayerItemDidPlayToEndTime.
This only set the correct metadata at the end of the track.

Maybe try to also remove MPMediaItemPropertyPlaybackDuration and MPNowPlayingInfoPropertyElapsedPlaybackTime because they should be set by the VideoPlayer

@Matthieu-dgl Matthieu-dgl force-pushed the feat-add-now-playing-info-for-video branch 2 times, most recently from ba1c29c to 0181d06 Compare November 4, 2024 05:40
@Matthieu-dgl
Copy link
Contributor Author

There's always the problem that the video title appears for 1 second and then disappears in the notfication. I'm open to an idea!

@Matthieu-dgl Matthieu-dgl force-pushed the feat-add-now-playing-info-for-video branch from 1d83e16 to a4fb80a Compare November 4, 2024 08:50
Copy link
Contributor

@adrien-coye adrien-coye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check the 1 second and then disappears asap.

I'll check the "Title jumps glitch" asap

Copy link
Contributor

@adrien-coye adrien-coye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a couple of small remarks regarding the VideoPlayer.

I don't think your branch can build right now. We usually try to have branches that can build even if the app is broken.

@adrien-coye
Copy link
Contributor

As a "nice to have", we could also set a thumbnail. We already have one, it should be straight forward.

@Matthieu-dgl Matthieu-dgl force-pushed the feat-add-now-playing-info-for-video branch 2 times, most recently from 8e70dee to 254be1d Compare November 19, 2024 07:56
Copy link
Contributor

@adrien-coye adrien-coye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code, it now looks like the VideoPlayer is decoupled enough. I no longer see AVPlayer in the view. Thanks for working on this @Matthieu-dgl .

I'll test the PR again later, including the audio player, when the change asked by @PhilippeWeidmann is done.

@adrien-coye adrien-coye self-requested a review November 21, 2024 14:17
Copy link
Contributor

@adrien-coye adrien-coye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We checked the code together, I need to play with files with metadata.

@Matthieu-dgl Matthieu-dgl force-pushed the feat-add-now-playing-info-for-video branch from a1b7a5d to e8d1f2e Compare November 22, 2024 12:48
@Matthieu-dgl Matthieu-dgl force-pushed the feat-add-now-playing-info-for-video branch from e8d1f2e to 8722a62 Compare December 2, 2024 05:24
@adrien-coye adrien-coye self-requested a review December 3, 2024 14:04
Copy link
Contributor

@adrien-coye adrien-coye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for applying changes, I'll show you tomorrow a patch to finish the last details.

Copy link

sonarqubecloud bot commented Dec 4, 2024

@adrien-coye adrien-coye self-requested a review December 4, 2024 10:28
Copy link
Contributor

@adrien-coye adrien-coye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes. It looks good to me, I'll ask @PhilippeWeidmann for a final review.

@adrien-coye adrien-coye merged commit cf59028 into master Dec 5, 2024
6 checks passed
@adrien-coye adrien-coye deleted the feat-add-now-playing-info-for-video branch December 5, 2024 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants