PE-4583: Video Preview (In-app) #1380
Conversation
… to check if mounted
…lay" issue on chrome
|
Visit the preview URL for this PR (updated for commit 693ce98): https://ardrive-web--pr1380-pe-4583-video-previe-295jplwc.web.app (expires Wed, 18 Oct 2023 17:55:30 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: a224ebaee2f0939e7665e7630e7d3d6cd7d0f8b0 |
…en video on both mobile and desktop
…deo-preview-in-app
assets/config/prod.json
Outdated
| @@ -8,6 +8,7 @@ | |||
| "enableQuickSyncAuthoring": true, | |||
| "enableMultipleFileDownload": true, | |||
| "enableVideoPreview": false, | |||
| "enableAudioPreview": false, | |||
There was a problem hiding this comment.
We will need to update it to true when releasing
assets/config/staging.json
Outdated
| @@ -8,6 +8,7 @@ | |||
| "enableQuickSyncAuthoring": true, | |||
| "enableMultipleFileDownload": true, | |||
| "enableVideoPreview": false, | |||
| "enableAudioPreview": false, | |||
There was a problem hiding this comment.
The same for the previous comment
| @@ -175,6 +184,16 @@ class FsEntryPreviewCubit extends Cubit<FsEntryPreviewState> { | |||
| case 'image': | |||
| emitImagePreview(file, previewUrl); | |||
| break; | |||
|
|
|||
| case 'audio': | |||
There was a problem hiding this comment.
@kunstmusik , it seems this branch is out to date. Let's sync with dev
There was a problem hiding this comment.
Merged latest from dev
lib/dev_tools/app_dev_tools.dart
Outdated
| @@ -233,6 +233,19 @@ class AppConfigWindowManagerState extends State<AppConfigWindowManager> { | |||
| type: ArDriveDevToolOptionType.bool, | |||
| ); | |||
|
|
|||
| ArDriveDevToolOption enableAudioPreviewOption = ArDriveDevToolOption( | |||
| @@ -1,5 +1,7 @@ | |||
| part of '../drive_detail_page.dart'; | |||
|
|
|||
| const List<double> _speedOptions = [.25, .5, .75, 1, 1.25, 1.5, 1.75, 2]; | |||
There was a problem hiding this comment.
Should we use the normal option here, as well?
There was a problem hiding this comment.
I'm using the numbers to set the speed; could use 'normal' here but it it wouldn't make the code that uses it much different (would have to check for 'normal' and use 1 for speed param; current code checks for 1 and uses 'normal' for speed label).
| if (isPlaying) { | ||
| await _lock.synchronized(() async { | ||
| await _videoPlayerController.play().catchError((e) { | ||
| logger.d('Error playing video: $e'); |
There was a problem hiding this comment.
nit: use logger.e
| _videoPlayerController.value.isPlaying) { | ||
| await _lock.synchronized(() async { | ||
| await _videoPlayerController.pause().catchError((error) { | ||
| logger.d('Error pausing video: $error'); |
There was a problem hiding this comment.
nit: use the logger.e
| child: Container(color: Colors.black.withOpacity(0.0)), | ||
| ), | ||
| ), | ||
| AnimatedOpacity( |
There was a problem hiding this comment.
We can improve the readability of this widget by splitting its sub-widgets into private method or classes.
There was a problem hiding this comment.
Agreed; do you think we should do it for this PR or do as tech debt?
| setState(() { | ||
| if (_videoPlayerController.value.hasError) { | ||
| logger.d('>>> ${_videoPlayerController.value.errorDescription}'); | ||
|
|
||
| // In case of emergency, reinitialize the video player. | ||
| _videoPlayerController.removeListener(_listener); | ||
| _videoPlayerController.dispose(); | ||
|
|
||
| _videoPlayerController = | ||
| VideoPlayerController.networkUrl(Uri.parse(widget.videoUrl)); | ||
| _videoPlayerController.initialize(); | ||
| _videoPlayerController.addListener(_listener); | ||
|
|
||
| _videoPlayer = | ||
| VideoPlayer(_videoPlayerController, key: const Key('videoPlayer')); | ||
| } | ||
| }); |
There was a problem hiding this comment.
We have almost the same logic on the VideoPlayerWidget. We could implement an abstraction for that, so we don't need to repeat the code. I'm not requesting for now, we can address it in a tech debt in the future.
There was a problem hiding this comment.
Added PE-4697 tech debt task to look at refactoring for clarity and code reuse.
| }); | ||
| _videoPlayerController.addListener(_listener); | ||
|
|
||
| SystemChrome.setPreferredOrientations([ |
kunstmusik
dismissed
thiagocarvalhodev’s stale review
The merge-base changed after approval.
pubspec.lock
Outdated
There was a problem hiding this comment.
Did we intend to update these versions?
…-accept-all-mov-and-mpg-formats PE-4732: video-preview-doesnt-accept-all-mov-and-mpg-formats
…-issue PE-4745: add pre-initialization spinner and buffer loading secondary value to …
…ack-option-video-preview PE-4778: missing-normal-playback-option-video-preview
|
Closing this one. The changes here were merged on this PR: #1425 |
Video player w/full screen view.
--- Releases ---
Android release: https://appdistribution.firebase.google.com/testerapps/1:305132849030:android:6cf0cd5ec064fad3ffce07/releases/0prio1hor6ls0