-
Notifications
You must be signed in to change notification settings - Fork 68
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
Image and video player improvements #1369
Conversation
I can get rid of it! It just felt weird to not have anything in the box when there's no thumbnail
Hmm, could you provide the community/links so that I can check this out?
I think we can make this into a separate PR! |
Here's an example I found in [email protected] (I know that instance is down, but the community is available via other instances). Another thing I see in this screenshot (and that I've been noticing a lot while browsing lately) is that URLs detected as non-video links with no thumbnail have the play icon instead of the web browser icon as the placeholder. I think that's a regression (maybe not in this PR though). |
Thanks, I'll check it out.
I noticed that too - it does seem to be a regression from the original PR (it'll be fixed in this PR though!) |
Ahh, okay - in this case, there was supposed to be a thumbnail associated with that video post, but it was unable to find it. Presumably, this is because the original instance is down so it can no longer fetch the thumbnail image. When this happens, it shows the error icon with the play icon superimposed on it (so it looks like a circular play icon instead) |
Interesting, good catch!! I think I've seen that error placeholder for articles as well. Do we just need to supply our own widget for error cases (which can be the solid gray color, as though there's image at all)? |
A few updates:
I think it's now at a place where you can try out the new changes and let me know if you see any new issues! |
The youtube player doesn't have an unmute which means should you want to enable audio you have to go to settings to disable auto mute. update We could do something like this Center(
child: ypf.YoutubePlayerBuilder(
player: ypf.YoutubePlayer(
onReady: () => _ypfController.addListener(listener),
aspectRatio: 16 / 10,
topActions: [
IconButton(
onPressed: () {
muted ? _ypfController.unMute() : _ypfController.mute();
setState(() {
muted = !muted;
});
},
icon: Icon(
muted ? Icons.volume_off : Icons.volume_up,
color: Colors.white,
))
], Result. you can move it to the right or left. |
I noticed this as well, and I like your suggestion for adding a button! P.S. One more thing I noticed on Android is that, once you enter fullscreen mode, Thunder stays in fullscreen mode even after exiting the video player. This means that the navigation and notification bars are hidden and have to be "swiped in". @ggichure Any ideas on that? |
Thanks for the feedback! I'll add in the mute button for the youtube player. It would be nice if we could combine both players into one to make everything more consistent but that can be explored in the future.
This also happens on iOS and I'm not too sure why - it's probably related to the internal logic of the video players. |
We can play around with this to achieve desired result. For BetterPlayer , although this seems to be working correctly without this. List<SystemUiOverlay> systemOverlaysAfterFullScreen = SystemUiOverlay.values,
List<DeviceOrientation> deviceOrientationsAfterFullScreen = const [DeviceOrientation.portraitUp, DeviceOrientation.portraitDown, DeviceOrientation.landscapeLeft, DeviceOrientation.landscapeRight], BetterPlayerConfiguration betterPlayerConfiguration = BetterPlayerConfiguration(
aspectRatio: 16 / 10,
fit: BoxFit.cover,
autoPlay: autoPlayVideo(thunderBloc),
fullScreenByDefault: thunderBloc.videoAutoFullscreen,
looping: thunderBloc.videoAutoLoop,
autoDetectFullscreenAspectRatio: true,
useRootNavigator: true,
systemOverlaysAfterFullScreen: [ //New
SystemUiOverlay.bottom, //New
SystemUiOverlay.top, //New
], Youtube Player child: ypf.YoutubePlayerBuilder(
onExitFullScreen: () {
SystemChrome.setEnabledSystemUIMode(SystemUiMode.edgeToEdge);
}, onfull_screen_exit_yt.mov |
…applying immediately, fixed network connectivity checking
Alright, I've applied more changes to fix the mute issue and some other various issues. It took a bit longer to implement the mute because the youtube player doesn't seem to provide a status on whether or not the video is muted, so the mute status is handled by externally.
Thanks for providing those suggestions! I only applied the configuration for the youtube player as that was causing issues for me. Let me know if everything else looks good - I'll only focus on fixing existing issues rather than adding new things as those are better suited to a separate PR! |
I think the best way to review this is to test it. 😊 Since @ggichure approved, you can merge any time, but I will also build this and run it for a little while if that's ok. 😊 |
One super small thing I just noticed is that playing a video doesn't respect the "Mark Read After Viewing Media" setting. Otherwise so far so good... |
Working on this on a different branch |
Pull Request Description
This PR builds upon the great video player work that @ggichure implemented in #1081 and also adds some simplification to the way we determine image dimensions.
To summarize, these are the high level changes:
parsePostView
function to handle embedded video linkspostView.post.embedVideoUrl
. In these cases, thepostView.post.url
may not directly point to a video link, butpostView.post.embedVideoUrl
may. We want these links to also open up the video player (e.g., embedded peertube video links)I've also adjusted
Media
to have athumbnailUrl
parameter. This is to allow us to account for both images/videosmediaUrl
should now point to the url of the media (image or video urls)thumbnailUrl
should point to a valid image url for displaying on the feedoriginalUrl
is the original url of the postIssue Being Fixed
Issue Number: N/A
Screenshots / Recordings
Checklist
semanticLabel
s where applicable for accessibility?