[camera] Make Optional.of constructor const#11247
[camera] Make Optional.of constructor const#11247auto-submit[bot] merged 1 commit intoflutter:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request makes the Optional.of constructor const and removes an obsolete ArgumentError.checkNotNull guard. This change is applied to the Optional class which is duplicated in four files within the camera packages. The version is updated and a changelog entry is added. A test is also updated to use the new const constructor.
| /// Constructs an Optional of the given [value]. | ||
| /// | ||
| /// Throws [ArgumentError] if [value] is null. | ||
| Optional.of(T value) : _value = value { | ||
| // TODO(cbracken): Delete and make this ctor const once mixed-mode | ||
| // execution is no longer around. | ||
| ArgumentError.checkNotNull(value); | ||
| } | ||
| const Optional.of(T value) : _value = value; |
There was a problem hiding this comment.
As you've noted in the PR description, this Optional<T> class is duplicated across multiple packages. This change is a good example of why that's a maintainability issue, as it has to be applied in four separate places. It would be a great improvement to refactor this in a follow-up PR.
Two potential approaches you mentioned are:
- Move
Optional<T>to a shared package (e.g.,camera_platform_interface). - Replace its usage with a private sentinel pattern, similar to what's done in the Flutter framework for
ThemeData.copyWith.
Moving it to a shared package seems like a good path forward. This would also be a good opportunity to audit the implementation for consistency, as there are some subtle differences between the duplicated versions (e.g., in the transform method signature).
stuartmorgan-g
left a comment
There was a problem hiding this comment.
LGTM
Note that the entire
Optional<T>class (and surroundingCameraController) is copy-pasted across 4 files in the camera packages. Would you prefer to moveOptional<T>intocamera_platform_interfaceto share it
We don't currently attempt to de-dup shared example app code between the federated implementation package examples. It's an open area of future work, but much, much bigger in scope than this particular class.
or replace it entirely e.g. with a private sentinel pattern in
copyWith(as the Flutter framework does forThemeData)?
This is public API (unfortunately; I don't think it actually needed to be), so changing it would be a breaking change for the plugin, for very little value.
|
@bparrishMines for secondary review. |
Resolves old TODO from 2023 ( 6e09f5b committed by @camsim99 ) by making the
Optional.ofconstructorconstand removing the obsolete runtimeArgumentError.checkNotNullguard that was only needed during Dart 2's mixed-mode (unsound null safety) era. Mixed-mode execution was removed in Dart 3.0, so this is dead code. Not a breaking change.Note that the entire
Optional<T>class (and surroundingCameraController) is copy-pasted across 4 files in the camera packages. Would you prefer to moveOptional<T>intocamera_platform_interfaceto share it, or replace it entirely e.g. with a private sentinel pattern incopyWith(as the Flutter framework does forThemeData)?Pre-Review Checklist
[shared_preferences]///).