stream-chat-flutter
stream-chat-flutter copied to clipboard
refactor(ui): replace package:dart_vlc with package:media_kit
Submit a pull request
CLA
- [x] I have signed the Stream CLA (required).
- [x] The code changes follow best practices
- [x] Code changes are tested (add some information if not applicable)
Description of the pull request
This PR replaces dart_vlc with media_kit (used on Microsoft Windows & GNU/Linux in stream_chat_flutter). media_kit has major benefits over dart_vlc e.g. better stability, improved performance, less bundle size etc.
To simplify & reduce the total size of implementation, video_player_media_kit is used. It automatically adds support for unsupported platforms to video_player. This means existing video_player-based implementation is automatically shared between all platforms without any additional implementation, wrapper or boilerplate.
video_player_media_kit is part of media_kit project itself, where it receives similar support & maintenance.
It also happens to be sponsored by GetStream.IO!
References:
- https://pub.dev/packages/media_kit
- https://pub.dev/packages/video_player_media_kit
- https://github.com/media-kit/media-kit
I tested the stream_chat_flutter/example app on Windows & GNU/Linux after changes, videos seem to play just fine.
name: stream_chat_flutter_example
description: A new Flutter project.
publish_to: none
version: 1.0.0+1
environment:
sdk: ">=3.0.0 <4.0.0"
flutter: ">=3.10.0"
dependencies:
collection: ^1.15.0
cupertino_icons: ^1.0.4
flutter:
sdk: flutter
responsive_builder: ^0.7.0
stream_chat_flutter:
path: ../../stream_chat_flutter
stream_chat_localizations:
path: ../../stream_chat_localizations
stream_chat_persistence:
path: ../../stream_chat_persistence
dependency_overrides:
stream_chat_flutter:
path: ../../stream_chat_flutter
stream_chat_localizations:
path: ../../stream_chat_localizations
stream_chat_persistence:
path: ../../stream_chat_persistence
flutter:
uses-material-design: true
assets:
- assets/background_doodle.png
I really think we should first deprecate FullScreenMedia* classes before removing them. They are part of our public interface, and if we remove them like that. will probably break users' apps.
I hope the changes are according to your expectation.
Thanks!
Hi @alexmercerind, we would like to merge this before the next release. I updated the branch with the latest develop, and it caused the tests to fail. Could you take a look at that?
Hey @alexmercerind !
We have switched our main branch to master, and I took the liberty of switching the base of this PR to that. I see there are a few conflicts. Once you can resolve those and the tests pass, I will merge them.
Thank you for bearing with us for so long. I am really sorry for your inconvenience :pray:
@alexmercerind any updates on this?
@jnelle we have planned it to be part of our v7.1.0, so we will try to drive it to master by end of next week.
Codecov Report
Attention: Patch coverage is 96.55172% with 1 line in your changes missing coverage. Please review.
Project coverage is 60.04%. Comparing base (
226933c) to head (de4bd62). Report is 34 commits behind head on master.
| Files | Patch % | Lines |
|---|---|---|
| ...tter/lib/src/message_widget/parse_attachments.dart | 0.00% | 1 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## master #1743 +/- ##
==========================================
+ Coverage 59.40% 60.04% +0.64%
==========================================
Files 310 306 -4
Lines 17834 17649 -185
==========================================
+ Hits 10594 10598 +4
+ Misses 7240 7051 -189
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hello! I created an issue that I believe could be resolved by this change: https://github.com/GetStream/stream-chat-flutter/issues/1959
Otherwise, I believe this PR would serve as a fix as well in the dart_vlc repo: https://github.com/alexmercerind/dart_vlc/pull/388
This is impacting my production app, so it is a relatively urgent issue. Is there another workaround recommended prior to one of these PR's merging?
Also could I get an estimate of when either/both of these PR's will be merged and deployed?
Thanks in advance!