Skip to content

lightbox test: Add video player regression tests #694

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Jun 15, 2024
Merged

lightbox test: Add video player regression tests #694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8ff5f7f
lightbox: Apply correct fix for slider flickering
rajveermalviya Jun 11, 2024
44c734c
lightbox test: Factor out VideoDurationLabel for testing visibility
rajveermalviya Jun 11, 2024
71d679e
lightbox test [nfc]: Refactor FakeVideoPlayerPlatform
rajveermalviya Jun 11, 2024
8f018d4
lightbox test: Add video player regression tests
rajveermalviya Jun 13, 2024
1e2143c
lightbox test [nfc]: Tighten up checkPositions doc
gnprice Jun 15, 2024
97109a0
lightbox test [nfc]: Tighten test cases using a checkPositionsRelative
gnprice Jun 15, 2024
b9065e8
lightbox test [nfc]: Extract findSliderPosition
gnprice Jun 15, 2024
ff029af
lightbox test: Revise no-flicker-upon-drag test, and add explanatory …
gnprice Jun 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 38 additions & 33 deletions lib/widgets/lightbox.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,31 @@ class _ImageLightboxPageState extends State<_ImageLightboxPage> {
}
}

class VideoDurationLabel extends StatelessWidget {
Copy link
Member

Choose a reason for hiding this comment

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

Nit in commit structure: in this first commit:
349b2be lightbox [nfc]: Factor out VideoDurationLabel for testing visibility

the VideoDurationLabel group of tests can be squashed in, and then call it a lightbox test: commit.

That reduces the number of things that are going on in the main commit, and it also provides some motivation right within this commit to explain why it needs to be a public widget.

const VideoDurationLabel(this.duration, {
super.key,
this.semanticsLabel,
});

final Duration duration;
final String? semanticsLabel;

@visibleForTesting
static String formatDuration(Duration value) {
Copy link
Member

Choose a reason for hiding this comment

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

Oh speaking of which, this should get a @visibleForTesting.

final hours = value.inHours.toString().padLeft(2, '0');
final minutes = value.inMinutes.remainder(60).toString().padLeft(2, '0');
final seconds = value.inSeconds.remainder(60).toString().padLeft(2, '0');
return '${hours == '00' ? '' : '$hours:'}$minutes:$seconds';
}

@override
Widget build(BuildContext context) {
return Text(formatDuration(duration),
semanticsLabel: semanticsLabel,
style: const TextStyle(color: Colors.white));
}
}

class _VideoPositionSliderControl extends StatefulWidget {
final VideoPlayerController controller;

Expand Down Expand Up @@ -277,27 +302,7 @@ class _VideoPositionSliderControlState extends State<_VideoPositionSliderControl
}

void _handleVideoControllerUpdate() {
setState(() {
// After 'controller.seekTo' is called in 'Slider.onChangeEnd' the
// position indicator switches back to the actual controller's position
// but since the call 'seekTo' completes before the actual controller
// updates are notified, the position indicator that switches to controller's
// position can show the older position before the call to 'seekTo' for a
// single frame, resulting in a glichty UX.
//
// To avoid that, we delay the position indicator switch from '_sliderValue' to
// happen when we are notified of the controller update.
if (_isSliderDragging && _sliderValue == widget.controller.value.position) {
_isSliderDragging = false;
}
});
}

static String _formatDuration(Duration value) {
final hours = value.inHours.toString().padLeft(2, '0');
final minutes = value.inMinutes.remainder(60).toString().padLeft(2, '0');
final seconds = value.inSeconds.remainder(60).toString().padLeft(2, '0');
return '${hours == '00' ? '' : '$hours:'}$minutes:$seconds';
setState(() {});
}

@override
Expand All @@ -307,8 +312,8 @@ class _VideoPositionSliderControlState extends State<_VideoPositionSliderControl
: widget.controller.value.position;

return Row(children: [
Text(_formatDuration(currentPosition),
style: const TextStyle(color: Colors.white)),
VideoDurationLabel(currentPosition,
semanticsLabel: "Current position"),
Expanded(
child: Slider(
value: currentPosition.inMilliseconds.toDouble(),
Expand All @@ -325,20 +330,20 @@ class _VideoPositionSliderControlState extends State<_VideoPositionSliderControl
_sliderValue = Duration(milliseconds: value.toInt());
});
},
onChangeEnd: (value) {
onChangeEnd: (value) async {
final durationValue = Duration(milliseconds: value.toInt());
setState(() {
_sliderValue = durationValue;
});
widget.controller.seekTo(durationValue);

// The toggling back of '_isSliderDragging' is omitted here intentionally,
// see '_handleVideoControllerUpdates'.
await widget.controller.seekTo(durationValue);
Copy link
Member

Choose a reason for hiding this comment

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

Hmm I see! This does feel cleaner than the previous solution.

I guess this is an example that would have been caught by the lint rule unawaited_futures actually I guess its twin, discarded_futures. I've just filed #731 to track us enabling those.

In the commit message:

lightbox: Apply correct fix for slider flickering

This changes makes it so that the async function `seekTo` is awaited
before the `setState` in `Slider.onChangeEnd`, thus first invoking
the `ValueNotifier.value` setter which as a side-effect calls the
`setState` in `_handleVideoControllerUpdate`, and then the `setState`
in `Slider.onChangeEnd`.

Resulting in both setState calls to be coalesced in a single redraw.
Whereas previously the unawaited `seekTo` call would delay the
`ValueNotifier.value` setter call in a microtask.

I don't know what you're referring to when you mention "the ValueNotifier.value setter". Can you expand on that? For example, which object here is the ValueNotifier in question?

Copy link
Member Author

Choose a reason for hiding this comment

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

It refers to seekTo's implementation which calls into platforms bindings, waits for it's Future to complete and then calls _updatePosition which dispatches position update via ValueNotifier.value setter – resulting in our _handleVideoControllerUpdate to be called.

Copy link
Member

Choose a reason for hiding this comment

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

OK, so the ValueNotifier this is referring to is the controller? In that case I think a helpful rewording would be to replace "invoking the ValueNotifier.value setter" with "setting the controller's value".

if (mounted) {
setState(() {
_sliderValue = durationValue;
_isSliderDragging = false;
});
}
},
),
),
Text(_formatDuration(widget.controller.value.duration),
style: const TextStyle(color: Colors.white)),
VideoDurationLabel(widget.controller.value.duration,
semanticsLabel: "Video duration"),
]);
}
}
Expand Down
Loading
Loading