stream-chat-flutter icon indicating copy to clipboard operation
stream-chat-flutter copied to clipboard

refactor(ui): replace package:dart_vlc with package:media_kit

Open alexmercerind opened this issue 2 years ago • 8 comments

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

alexmercerind avatar Oct 06 '23 10:10 alexmercerind

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

alexmercerind avatar Oct 06 '23 10:10 alexmercerind

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!

alexmercerind avatar Oct 19 '23 17:10 alexmercerind

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?

Brazol avatar Nov 30 '23 10:11 Brazol

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:

esarbanis avatar Dec 04 '23 15:12 esarbanis

@alexmercerind any updates on this?

jnelle avatar Feb 21 '24 16:02 jnelle

@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.

esarbanis avatar Feb 21 '24 16:02 esarbanis

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.

codecov[bot] avatar Feb 27 '24 18:02 codecov[bot]

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!

IShark01 avatar Jun 18 '24 01:06 IShark01