This repository was archived by the owner on Aug 14, 2023. It is now read-only.
feat: add camera selection dropdown to the animoji intro page #435
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
omartinma
reviewed
Feb 1, 2023
|
Thanks a lot for opening this PR @sergsavchuk, i will be reviewing this soon! |
omartinma
reviewed
Feb 2, 2023
omartinma
reviewed
Feb 2, 2023
omartinma
reviewed
Feb 2, 2023
omartinma
reviewed
Feb 2, 2023
omartinma
reviewed
Feb 2, 2023
omartinma
reviewed
Feb 2, 2023
omartinma
reviewed
Feb 2, 2023
omartinma
reviewed
Feb 2, 2023
omartinma
reviewed
Feb 2, 2023
omartinma
reviewed
Feb 2, 2023
omartinma
reviewed
Feb 2, 2023
omartinma
reviewed
Feb 2, 2023
omartinma
reviewed
Feb 2, 2023
omartinma
reviewed
Feb 2, 2023
omartinma
reviewed
Feb 2, 2023
omartinma
reviewed
Feb 2, 2023
omartinma
reviewed
Feb 2, 2023
omartinma
reviewed
Feb 2, 2023
omartinma
reviewed
Feb 2, 2023
omartinma
reviewed
Feb 2, 2023
omartinma
reviewed
Feb 2, 2023
|
I left some comments in the PR @sergsavchuk. Happy to chat if you need some help there. Thanks again for the contribution! |
|
@omartinma thanks for the review! 😃 |
Thanks for all the changes @sergsavchuk This is a consequence of making the Bloc global. Also, i dont think this init needs to be here. If you call this event here then the camera permission popup will be triggered, and we wanted to be on the animoji screen. So in order to not change this flow the only solution i see is to move the init call to that page and test it there, so wondering if we need still the bloc to be global, and simply pass the argument of CameraDescription from Animoji screen to Photobooth. |
|
@omartinma by default Code examplediff --git a/lib/animoji_intro/widgets/animoji_intro_body.dart b/lib/animoji_intro/widgets/animoji_intro_body.dart
index 02a895c..9674427 100644
--- a/lib/animoji_intro/widgets/animoji_intro_body.dart
+++ b/lib/animoji_intro/widgets/animoji_intro_body.dart
@@ -1,4 +1,5 @@
import 'package:flutter/material.dart';
+import 'package:flutter_bloc/flutter_bloc.dart';
import 'package:holobooth/animoji_intro/animoji_intro.dart';
import 'package:holobooth/camera/camera.dart';
import 'package:holobooth/l10n/l10n.dart';
@@ -117,7 +118,7 @@ class _BottomContent extends StatelessWidget {
textAlign: smallScreen ? TextAlign.center : TextAlign.left,
),
),
- const CameraSelectionDropdown(),
+ CameraSelectionDropdown(cameraBloc: context.read<CameraBloc>()),
const Flexible(child: AnimojiNextButton()),
],
),
diff --git a/lib/app/app.dart b/lib/app/app.dart
index 31d82ec..1388c11 100644
--- a/lib/app/app.dart
+++ b/lib/app/app.dart
@@ -45,9 +45,7 @@ class App extends StatelessWidget {
child: MultiBlocProvider(
providers: [
BlocProvider(create: (context) => MuteSoundBloc()),
- BlocProvider(
- create: (context) => CameraBloc()..add(CameraStarted()),
- ),
+ BlocProvider(create: (context) => CameraBloc()),
],
child: AnimatedFadeIn(
child: ResponsiveLayoutBuilder(
diff --git a/lib/camera/widgets/camera_selection_dropdown.dart b/lib/camera/widgets/camera_selection_dropdown.dart
index c6ea713..ba9b54c 100644
--- a/lib/camera/widgets/camera_selection_dropdown.dart
+++ b/lib/camera/widgets/camera_selection_dropdown.dart
@@ -5,12 +5,27 @@ import 'package:holobooth/camera/camera.dart';
import 'package:holobooth/l10n/l10n.dart';
import 'package:holobooth_ui/holobooth_ui.dart';
-class CameraSelectionDropdown extends StatelessWidget {
- const CameraSelectionDropdown({super.key});
+class CameraSelectionDropdown extends StatefulWidget {
+ const CameraSelectionDropdown({super.key, required this.cameraBloc});
+
+ final CameraBloc cameraBloc;
@visibleForTesting
static const cameraErrorViewKey = Key('camera_error_view');
+ @override
+ State<CameraSelectionDropdown> createState() =>
+ _CameraSelectionDropdownState();
+}
+
+class _CameraSelectionDropdownState extends State<CameraSelectionDropdown> {
+ @override
+ void initState() {
+ super.initState();
+
+ widget.cameraBloc.add(CameraStarted());
+ }
+
@override
Widget build(BuildContext context) {
return BlocBuilder<CameraBloc, CameraState>(
@@ -20,7 +35,7 @@ class CameraSelectionDropdown extends StatelessWidget {
builder: (_, state) {
if (state.cameraError != null) {
return _CameraErrorView(
- key: cameraErrorViewKey,
+ key: CameraSelectionDropdown.cameraErrorViewKey,
error: state.cameraError!,
);
}
@@ -37,7 +52,7 @@ class CameraSelectionDropdown extends StatelessWidget {
final cameras = state.availableCameras;
if (cameras == null || cameras.isEmpty) {
return _CameraErrorView(
- key: cameraErrorViewKey,
+ key: CameraSelectionDropdown.cameraErrorViewKey,
error: CameraException('cameraNotFound', 'Camera not found'),
);
} |
@omartinma it seems strange to pass the camera all around the screens just to pass it back to the photobooth page and in fact we achieve the same result as before: we have reference to the camera on all the pages 🤔 Code examplediff --git a/lib/animoji_intro/widgets/animoji_intro_body.dart b/lib/animoji_intro/widgets/animoji_intro_body.dart
index 02a895c..33f0f1b 100644
--- a/lib/animoji_intro/widgets/animoji_intro_body.dart
+++ b/lib/animoji_intro/widgets/animoji_intro_body.dart
@@ -1,4 +1,5 @@
import 'package:flutter/material.dart';
+import 'package:flutter_bloc/flutter_bloc.dart';
import 'package:holobooth/animoji_intro/animoji_intro.dart';
import 'package:holobooth/camera/camera.dart';
import 'package:holobooth/l10n/l10n.dart';
@@ -86,40 +87,43 @@ class _BottomContent extends StatelessWidget {
final l10n = context.l10n;
final textTheme = Theme.of(context).textTheme;
- return Container(
- color: HoloBoothColors.modalSurface,
- padding: const EdgeInsets.all(20),
- child: Flex(
- direction: smallScreen ? Axis.vertical : Axis.horizontal,
- mainAxisAlignment: MainAxisAlignment.spaceEvenly,
- children: [
- Flexible(
- child: ShaderMask(
- shaderCallback: (bounds) {
- return HoloBoothGradients.secondaryFive
- .createShader(Offset.zero & bounds.size);
- },
- child: const Icon(
- Icons.videocam_rounded,
- size: 40,
- color: HoloBoothColors.white,
+ return BlocProvider(
+ create: (context) => CameraBloc()..add(CameraStarted()),
+ child: Container(
+ color: HoloBoothColors.modalSurface,
+ padding: const EdgeInsets.all(20),
+ child: Flex(
+ direction: smallScreen ? Axis.vertical : Axis.horizontal,
+ mainAxisAlignment: MainAxisAlignment.spaceEvenly,
+ children: [
+ Flexible(
+ child: ShaderMask(
+ shaderCallback: (bounds) {
+ return HoloBoothGradients.secondaryFive
+ .createShader(Offset.zero & bounds.size);
+ },
+ child: const Icon(
+ Icons.videocam_rounded,
+ size: 40,
+ color: HoloBoothColors.white,
+ ),
),
),
- ),
- Flexible(
- flex: smallScreen ? 0 : 3,
- child: SelectableText(
- l10n.animojiIntroPageSubheading,
- key: const Key('animojiIntro_subheading_text'),
- style: textTheme.bodyLarge?.copyWith(
- color: HoloBoothColors.white,
+ Flexible(
+ flex: smallScreen ? 0 : 3,
+ child: SelectableText(
+ l10n.animojiIntroPageSubheading,
+ key: const Key('animojiIntro_subheading_text'),
+ style: textTheme.bodyLarge?.copyWith(
+ color: HoloBoothColors.white,
+ ),
+ textAlign: smallScreen ? TextAlign.center : TextAlign.left,
),
- textAlign: smallScreen ? TextAlign.center : TextAlign.left,
),
- ),
- const CameraSelectionDropdown(),
- const Flexible(child: AnimojiNextButton()),
- ],
+ const CameraSelectionDropdown(),
+ const Flexible(child: AnimojiNextButton()),
+ ],
+ ),
),
);
}
diff --git a/lib/animoji_intro/widgets/animoji_next_button.dart b/lib/animoji_intro/widgets/animoji_next_button.dart
index 314d562..a38f208 100644
--- a/lib/animoji_intro/widgets/animoji_next_button.dart
+++ b/lib/animoji_intro/widgets/animoji_next_button.dart
@@ -3,6 +3,7 @@ import 'package:flutter/material.dart';
import 'package:flutter_bloc/flutter_bloc.dart';
import 'package:holobooth/assets/assets.dart';
import 'package:holobooth/audio_player/audio_player.dart';
+import 'package:holobooth/camera/camera.dart';
import 'package:holobooth/l10n/l10n.dart';
import 'package:holobooth/photo_booth/photo_booth.dart';
import 'package:holobooth_ui/holobooth_ui.dart';
@@ -32,7 +33,9 @@ class _AnimojiNextButtonState extends State<AnimojiNextButton> {
label: 'start-holobooth',
),
);
- Navigator.of(context).push(PhotoBoothPage.route());
+ Navigator.of(context).push(
+ PhotoBoothPage.route(context.read<CameraBloc>()),
+ );
},
child: Text(l10n.nextButtonText),
),
diff --git a/lib/app/app.dart b/lib/app/app.dart
index 31d82ec..82b6aeb 100644
--- a/lib/app/app.dart
+++ b/lib/app/app.dart
@@ -6,7 +6,6 @@ import 'package:download_repository/download_repository.dart';
import 'package:flutter/material.dart';
import 'package:flutter_bloc/flutter_bloc.dart';
import 'package:holobooth/audio_player/audio_player.dart';
-import 'package:holobooth/camera/camera.dart';
import 'package:holobooth/l10n/l10n.dart';
import 'package:holobooth/landing/landing.dart';
import 'package:holobooth/rive/rive.dart';
@@ -45,9 +44,6 @@ class App extends StatelessWidget {
child: MultiBlocProvider(
providers: [
BlocProvider(create: (context) => MuteSoundBloc()),
- BlocProvider(
- create: (context) => CameraBloc()..add(CameraStarted()),
- ),
],
child: AnimatedFadeIn(
child: ResponsiveLayoutBuilder(
diff --git a/lib/convert/view/convert_page.dart b/lib/convert/view/convert_page.dart
index 8abf483..f8b4796 100644
--- a/lib/convert/view/convert_page.dart
+++ b/lib/convert/view/convert_page.dart
@@ -2,6 +2,7 @@ import 'package:convert_repository/convert_repository.dart';
import 'package:flutter/material.dart';
import 'package:flutter_bloc/flutter_bloc.dart';
import 'package:holobooth/assets/assets.dart';
+import 'package:holobooth/camera/bloc/camera_bloc.dart';
import 'package:holobooth/convert/convert.dart';
import 'package:holobooth/footer/footer.dart';
import 'package:holobooth/l10n/l10n.dart';
@@ -23,10 +24,13 @@ class ConvertPage extends StatelessWidget {
final List<Frame> frames;
- static Route<void> route(
- List<Frame> frames,
- ) {
- return AppPageRoute(builder: (_) => ConvertPage(frames: frames));
+ static Route<void> route(List<Frame> frames, CameraBloc cameraBloc) {
+ return AppPageRoute(
+ builder: (_) => BlocProvider.value(
+ value: cameraBloc,
+ child: ConvertPage(frames: frames),
+ ),
+ );
}
@override
@@ -78,7 +82,12 @@ class _ConvertViewState extends State<ConvertView> {
} else if (state.status == ConvertStatus.loadedFrames) {
final convertBloc = context.read<ConvertBloc>()
..add(const GenerateVideoRequested());
- Navigator.of(context).push(SharePage.route(convertBloc: convertBloc));
+ Navigator.of(context).push(
+ SharePage.route(
+ convertBloc: convertBloc,
+ cameraBloc: context.read<CameraBloc>(),
+ ),
+ );
}
},
child: Scaffold(
diff --git a/lib/convert/widgets/convert_error_view.dart b/lib/convert/widgets/convert_error_view.dart
index 7a6c393..d00f79d 100644
--- a/lib/convert/widgets/convert_error_view.dart
+++ b/lib/convert/widgets/convert_error_view.dart
@@ -1,5 +1,6 @@
import 'package:flutter/material.dart';
import 'package:flutter_bloc/flutter_bloc.dart';
+import 'package:holobooth/camera/bloc/camera_bloc.dart';
import 'package:holobooth/convert/convert.dart';
import 'package:holobooth/l10n/l10n.dart';
import 'package:holobooth/photo_booth/photo_booth.dart';
@@ -91,7 +92,9 @@ class RetakeExperienceButton extends StatelessWidget {
final l10n = context.l10n;
return OutlinedButton(
onPressed: () {
- Navigator.of(context).pushReplacement(PhotoBoothPage.route());
+ Navigator.of(context).pushReplacement(
+ PhotoBoothPage.route(context.read<CameraBloc>()),
+ );
},
child: Text(l10n.retakeVideoGeneration),
);
diff --git a/lib/photo_booth/view/photo_booth_page.dart b/lib/photo_booth/view/photo_booth_page.dart
index da4ce36..b4ecb65 100644
--- a/lib/photo_booth/view/photo_booth_page.dart
+++ b/lib/photo_booth/view/photo_booth_page.dart
@@ -4,6 +4,7 @@ import 'package:flutter_bloc/flutter_bloc.dart';
import 'package:holobooth/assets/assets.dart';
import 'package:holobooth/audio_player/audio_player.dart';
import 'package:holobooth/avatar_detector/avatar_detector.dart';
+import 'package:holobooth/camera/camera.dart';
import 'package:holobooth/convert/convert.dart';
import 'package:holobooth/in_experience_selection/in_experience_selection.dart';
import 'package:holobooth/photo_booth/photo_booth.dart';
@@ -16,10 +17,13 @@ typedef PrecacheImageFn = Future<void> Function(
);
class PhotoBoothPage extends StatelessWidget {
- const PhotoBoothPage({super.key});
+ const PhotoBoothPage({super.key, required this.cameraBloc});
- static Route<void> route() =>
- AppPageRoute<void>(builder: (_) => const PhotoBoothPage());
+ final CameraBloc cameraBloc;
+
+ static Route<void> route(CameraBloc cameraBloc) => AppPageRoute<void>(
+ builder: (_) => PhotoBoothPage(cameraBloc: cameraBloc),
+ );
@override
Widget build(BuildContext context) {
@@ -32,6 +36,7 @@ class PhotoBoothPage extends StatelessWidget {
context.read<AvatarDetectorRepository>(),
)..add(const AvatarDetectorInitialized()),
),
+ BlocProvider.value(value: cameraBloc),
],
child: const PhotoBoothView(),
);
@@ -72,7 +77,7 @@ class _PhotoBoothViewState extends State<PhotoBoothView> {
_audioPlayerController.stopAudio();
Navigator.of(context).pushReplacement(
- ConvertPage.route(state.frames),
+ ConvertPage.route(state.frames, context.read<CameraBloc>()),
);
}
},
diff --git a/lib/share/view/share_page.dart b/lib/share/view/share_page.dart
index 26569f6..6142e9b 100644
--- a/lib/share/view/share_page.dart
+++ b/lib/share/view/share_page.dart
@@ -1,6 +1,7 @@
import 'package:download_repository/download_repository.dart';
import 'package:flutter/material.dart';
import 'package:flutter_bloc/flutter_bloc.dart';
+import 'package:holobooth/camera/bloc/camera_bloc.dart';
import 'package:holobooth/convert/convert.dart';
import 'package:holobooth/footer/footer.dart';
import 'package:holobooth/share/share.dart';
@@ -10,14 +11,22 @@ class SharePage extends StatelessWidget {
const SharePage({
super.key,
required this.convertBloc,
+ required this.cameraBloc,
});
final ConvertBloc convertBloc;
+ final CameraBloc cameraBloc;
static Route<void> route({
required ConvertBloc convertBloc,
+ required CameraBloc cameraBloc,
}) =>
- AppPageRoute(builder: (_) => SharePage(convertBloc: convertBloc));
+ AppPageRoute(
+ builder: (_) => SharePage(
+ convertBloc: convertBloc,
+ cameraBloc: cameraBloc,
+ ),
+ );
@override
Widget build(BuildContext context) {
diff --git a/lib/share/widgets/retake_button.dart b/lib/share/widgets/retake_button.dart
index 2ef34c8..543c802 100644
--- a/lib/share/widgets/retake_button.dart
+++ b/lib/share/widgets/retake_button.dart
@@ -1,4 +1,6 @@
import 'package:flutter/material.dart';
+import 'package:flutter_bloc/flutter_bloc.dart';
+import 'package:holobooth/camera/bloc/camera_bloc.dart';
import 'package:holobooth/l10n/l10n.dart';
import 'package:holobooth/photo_booth/photo_booth.dart';
import 'package:holobooth_ui/holobooth_ui.dart';
@@ -11,7 +13,8 @@ class RetakeButton extends StatelessWidget {
final l10n = context.l10n;
return GradientOutlinedButton(
onPressed: () {
- Navigator.of(context).pushReplacement(PhotoBoothPage.route());
+ Navigator.of(context)
+ .pushReplacement(PhotoBoothPage.route(context.read<CameraBloc>()));
},
icon: const Icon(
Icons.videocam_rounded,
|
@sergsavchuk IMO it seems also strange to have a bloc being global when actually is not needed, and if you pass the instance of the bloc from one page to the other, by calling pushReplacement the onClose method will be called so we might have trouble there as well |
|
@omartinma would it be appropriate to use Like that:diff --git a/lib/photo_booth/view/photo_booth_page.dart b/lib/photo_booth/view/photo_booth_page.dart
index da4ce36..fba0c52 100644
--- a/lib/photo_booth/view/photo_booth_page.dart
+++ b/lib/photo_booth/view/photo_booth_page.dart
@@ -1,4 +1,5 @@
import 'package:avatar_detector_repository/avatar_detector_repository.dart';
+import 'package:camera/camera.dart';
import 'package:flutter/material.dart';
import 'package:flutter_bloc/flutter_bloc.dart';
import 'package:holobooth/assets/assets.dart';
@@ -16,24 +17,29 @@ typedef PrecacheImageFn = Future<void> Function(
);
class PhotoBoothPage extends StatelessWidget {
- const PhotoBoothPage({super.key});
+ const PhotoBoothPage({super.key, this.camera});
- static Route<void> route() =>
- AppPageRoute<void>(builder: (_) => const PhotoBoothPage());
+ static Route<void> route({CameraDescription? camera}) =>
+ AppPageRoute<void>(builder: (_) => PhotoBoothPage(camera: camera));
+
+ final CameraDescription? camera;
@override
Widget build(BuildContext context) {
- return MultiBlocProvider(
- providers: [
- BlocProvider(create: (_) => PhotoBoothBloc()),
- BlocProvider(create: (_) => InExperienceSelectionBloc()),
- BlocProvider(
- create: (_) => AvatarDetectorBloc(
- context.read<AvatarDetectorRepository>(),
- )..add(const AvatarDetectorInitialized()),
- ),
- ],
- child: const PhotoBoothView(),
+ return RepositoryProvider<CameraDescription?>(
+ create: (_) => camera,
+ child: MultiBlocProvider(
+ providers: [
+ BlocProvider(create: (_) => PhotoBoothBloc()),
+ BlocProvider(create: (_) => InExperienceSelectionBloc()),
+ BlocProvider(
+ create: (_) => AvatarDetectorBloc(
+ context.read<AvatarDetectorRepository>(),
+ )..add(const AvatarDetectorInitialized()),
+ ),
+ ],
+ child: const PhotoBoothView(),
+ ),
);
}
}
diff --git a/lib/photo_booth/widgets/photobooth_body.dart b/lib/photo_booth/widgets/photobooth_body.dart
index 8a948bf..53e4d1d 100644
--- a/lib/photo_booth/widgets/photobooth_body.dart
+++ b/lib/photo_booth/widgets/photobooth_body.dart
@@ -74,11 +74,9 @@ class _PhotoboothBodyState extends State<PhotoboothBody> {
return Stack(
children: [
Align(
- child: BlocBuilder<CameraBloc, CameraState>(
- builder: (context, state) => CameraView(
- camera: state.camera,
- onCameraReady: _onCameraReady,
- ),
+ child: CameraView(
+ camera: context.read<CameraDescription?>(),
+ onCameraReady: _onCameraReady,
),
),
LayoutBuilder(
|
That is not the purpose of RepositoryProvider, it is this case is over-engineer when you just need to send one object from AnimojiPage to PhotoboothPage. What is the issue with passing this as a parameter? |
|
@omartinma I've removed the global |
|
@sergsavchuk just merged. I will be deploying to PROD this week ;) Thanks again for this awesome contribution! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Hi there!
I was quite upset when i couldn't try out Holobooth because it uses the first available camera and there is no way to change it. So I decided to fix it myself. I added a dropdown button with available cameras to the animoji intro page and passed the selected camera to
CameraViewonPhotoboothBody.There are few things I'm not sure about 🤔:
Coverage is still 100% 😋:
Desktop:
Mobile:
Type of Change