thunder-app
Experimental support for video player
Pull Request Description
- youtube_player_flutter to play yt videos on android and ios
- youtube_player_iframe to play videos on web , etc
- river_player to play any file with a standard codec
mp4,movetc
Issue Being Fixed
Issue Number: #383
Screenshots / Recordings
https://github.com/thunder-app/thunder/assets/31619962/8e5ecf34-72bb-44ee-b31d-626bbe36310e
Checklist
- [x] Did you update CHANGELOG.md?
- [x] Did you use localized strings where applicable?
- [x] Did you add
semanticLabels where applicable for accessibility?
Here's a post I came across recently that could help with testing!
https://programming.dev/post/9309147
I know this is in draft at the moment, but I'd like to just chime in that there is the option to only add partial support for videos in this PR!
We can extend video player functionality in future PRs to support edge cases 😊
Additionally, I'm not sure if you've noticed, but I've added an extra type to MediaType for videos. All PostViewMedia should be able to distinguish if its a video by checking postViewMedia.media.first.mediaType == MediaType.video. This might help simplify implementation a bit!
It seems like the preview of videos with no image from Lemmy are completely blank. I think we should at least have some icon to stand in, like the web icon for URLs with no preview. However, now that we have an in-app video player, we can use a video icon instead of the globe icon.
Resolved.
From your demo, it looks like some videos play with a white background. Is there any way to make that black?
Resolved
Some videos like this one never play. If there's an easy way to figure out which videos we support and which ones we don't, we can show the latter ones in the browser.
At the moment , I have no way of checking for this.
Once again, thanks for working on this! I'll be testing it out over the next few days to see if I can find any issues running it. I'll do a code review of it once I've tested it out 😄
Just did some quick testing on this and there are a couple of things I'd like to adjust, but they might be better suited for separate PRs given that this one is already quite large!
What are your thoughts on merging this in first, and then applying additional PRs in the future? @micahmo
I guess it depends on what you had in mind (whether the user experience will be poor without your suggestions, or whether they're more like enhancements). I think it also depends on whether you're planning a full release soon. But I'm totally fine with getting this in now. 😊
The main thing I had in mind was to adjust the logic to work alongside parsePostView and MediaView so that all the post parsing logic is centralized. By centralizing the logic to parsePostView, this might also help reduce the instances where we have this issue:
Some videos like this one never play. If there's an easy way to figure out which videos we support and which ones we don't, we can show the latter ones in the browser.
I did a quick code review and I think the core implementation of the video player (video player widgets, video settings, etc.) are good, so there shouldn't be many changes coming from there! Thanks again for implementing all those settings, especially for the first implementation 😁
And of course, @ggichure feel free to let me know if you'd prefer to work on my proposed changes (in which case, I can go into some more details about them) 😅
I think it also depends on whether you're planning a full release soon.
I'd like to get initial/experimental video support in for the next full release, but I don't have a timeline for when the full release will be. I don't think there is a rush to get a full release soon either since the current version should already be compatible with Lemmy 0.19.4!
Thoughts? @micahmo @ggichure
BTW, since this PR introduces new settings, keep in mind it'll have conflicts with #1348. I don't really care which ones goes first. 😊