-
Notifications
You must be signed in to change notification settings - Fork 135
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
base: master
Are you sure you want to change the base?
Prepare fullscreen support #132
Conversation
There was a problem hiding this 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'; |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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({ |
There was a problem hiding this 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 have separately a refactor pull request. Mix refactors and fullscreen feature makes difficult to track the codes
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 This PR is still open just as a "draft" to discuss more about this in future or possibly change the plans. |
Thanks for your useful info. I should probably start with dart_vlc_ffi and try to implement a custom control. |
You don't need anything special. |
@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 aVideo
widget: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