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

Prepare fullscreen support #132

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jnschulze
Copy link
Collaborator

@alexmercerind
I was a bit tired of arguing, so I made this proposal showing how fullscreen handling might be decoupled from the Video widget.

It allows you to pass an optional FullscreenController upon creating a Video widget:

Video(player: _player, fullscreenController: MyFullscreenController());

The button for toggling fullscreen will only appear if a controller was provided. (I've just added your proposed button from yesterday's PR)

A very basic implementation using your window_size fork might look like

class MyFullscreenController extends FullscreenController {
  @override
  void toggleFullscreen() {
    final isFullscreen = value.isFullscreen;
    if (isFullscreen) {
      window.exitFullscreen();
    } else {
      window.enterFullscreen();
    }

    value = value.copyWith(isFullscreen: !value.isFullscreen);
  }
}

Copy link

@panoti panoti left a comment

Choose a reason for hiding this comment

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

I think you should separate refactor code and fullscreen code in different pull requests.

@@ -20,6 +20,7 @@ import 'package:dart_vlc_ffi/src/internal/ffi.dart' as FFI;
import 'package:dart_vlc_ffi/dart_vlc_ffi.dart' as FFI;
export 'package:dart_vlc_ffi/dart_vlc_ffi.dart' hide DartVLC, Player;
export 'package:dart_vlc/src/widgets/video.dart';
export 'package:dart_vlc/src/widgets/controls.dart';
Copy link

Choose a reason for hiding this comment

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

I found that you import controls without using it. All codes below are just for refactor purpose.

@@ -247,8 +297,8 @@ class ControlState extends State<Control> with SingleTickerProviderStateMixin {
),
),
Positioned(
right: 15,
bottom: 12.5,
right: 16,
Copy link

Choose a reason for hiding this comment

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

why we don't keep old position?

Copy link
Owner

Choose a reason for hiding this comment

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

Adding an additional button certainly needs more space, infact this UI code wasn't written by me but a contributor. Ideally, one would write their own custom designed video controls. These "just work" and are very "generic" & here to show the plugin capabilities etc. in the example itself.

@@ -21,7 +21,7 @@ class VideoFrame {
final int videoHeight;
final Uint8List byteArray;

VideoFrame({
const VideoFrame({
Copy link

Choose a reason for hiding this comment

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

I think you should have separately a refactor pull request. Mix refactors and fullscreen feature makes difficult to track the codes

@jnschulze
Copy link
Collaborator Author

jnschulze commented Sep 10, 2021

@panoti

I think you should separate refactor code and fullscreen code in different pull requests.

I don't know who you are and what qualifies you to comment on this. But to give you some context: This PR isn't intended do be merged as is but was rather a response to #127 as a proposal showing how to abstract fullscreen handling. The changed right/bottom offsets were blindly copied from the other PR.

@panoti
Copy link

panoti commented Sep 10, 2021

@panoti
I don't know who you are and what qualifies you to comment on this. But to give you some context: This PR isn't intended do be merged as is but was rather a response to #127 as a proposal showing how to abstract fullscreen handling. The changed right/bottom offsets were blindly copied from the other PR.

I only contribute in the spirit of open source. I found your pull request very useful and it's missing from the library, I installed dart_vlc and can't find the fullscreen control. I think the author can do a faster review for your pull request and we will have fullscreen feature in the next version. If you are annoyed by some of my comments, I am very sorry.

@alexmercerind
Copy link
Owner

alexmercerind commented Sep 10, 2021

@panoti
I don't know who you are and what qualifies you to comment on this. But to give you some context: This PR isn't intended do be merged as is but was rather a response to #127 as a proposal showing how to abstract fullscreen handling. The changed right/bottom offsets were blindly copied from the other PR.

I only contribute in the spirit of open source. I found your pull request very useful and it's missing from the library, I installed dart_vlc and can't find the fullscreen control. I think the author can do a faster review for your pull request and we will have fullscreen feature in the next version. If you are annoyed by some of my comments, I am very sorry.

Most presumably, fullscreen will never be part of this plugin. Its NOT a libVLC feature or exposition but a Flutter / OS thing. (You are not making video output fullscreen but the window).

I made the PR for adding fullscreen to window_size but as Stuart Morgan said, another plugin window_manager implements the same I did in that PR. (Use that).
I expect users to implement their own fullscreen logic now. Infact this PR itself doesn't contain any fullscreen switching code, but a controller to implement own logic (and native calls) (as there might be other plugins that change window style/state. Thus, it can be breaking in some cases for users).

This PR is still open just as a "draft" to discuss more about this in future or possibly change the plans.

@panoti
Copy link

panoti commented Sep 10, 2021

@alexmercerind

Thanks for your useful info. I should probably start with dart_vlc_ffi and try to implement a custom control.

@alexmercerind
Copy link
Owner

alexmercerind commented Sep 10, 2021

@alexmercerind

Thanks for your useful info. I should probably start with dart_vlc_ffi and try to implement a custom control.

dart_vlc_ffi in itself has no meaning to use in Flutter. Its just for Dart CLI (...if someone really wanna use Dart only) & has no native code in it either. Its just FFI mapping & I just decided to split the package into two.

You don't need anything special.

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.

None yet

3 participants