[camera] Add back Optional type for nullable CameraController orientations

[camsim99]

Fixes regressions caused by #6870.

Adds flag to opt into explicitly interpreting nullable device orientations in the CameraValue in the copyWith method to allow for the clearing of those orientations.

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
• I signed the CLA.
• The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• I listed at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[christopherfujino]

Upon reading this, this actually seems like a legitimate use of the Optional type, since there are actually four possible states of the method parameter:

1. The caller wants the field to be true
2. The caller wants the field to be false
3. The caller wants the field to be null, or no lock
4. The caller wants the field to default to the previous state

Do I have this right? And if so, maybe what would be most clear would be to have the .copyWith() method take an enum value, like:

// IDK if these are valid enum value names
enum DesiredLockedCaptureOrientation {
  true,
  false,
  none,
  default,
}

class CameraValue {

  ...
  
  CameraValue copyWith({
    ...
    DesiredLockedCaptureOrientation desiredLockedCaptureOrientation = DesiredLockedCaptureOrientation.default,
    ...
  }) {
    final bool? nextLockedCaptureOrientation;
    switch (desiredLockedCaptureOrientation) {
      case LockedCaptureOrientation.true:
        nextLockedCaptureOrientation = true;
        break;
      case LockedCaptureOrientation.false:
        nextLockedCaptureOrientation = false;
        break;
      case LockedCaptureOrientation.none:
        nextLockedCaptureOrientation = null;
        break;
      case LockedCaptureOrientation.default:
        nextLockedCaptureOrientation = lockedCaptureOrientation;
        break;
    }
    
    return CameraValue(
      ...
      lockedCaptureOrientation: nextLockedCaptureOrientation,
      ...
    );
  }
}

[camsim99]

@christopherfujino I agree that using Optional did make sense upon reflection. I also like the idea of an enum, but I think that using an enum like you did for each nullable orientation could get confusing.

What do you think about creating a different DeviceOrientation-like enum that is essentially the same as DeviceOrientation and maps to it but has an additional value of none that is a valid value for the nullable orientations. For example:

enum DeviceOrientationCode {
  portraitUp,
  ...
  none,
}

extension DeviceOrientationCodeExtension on DeviceOrientation {
  DeviceOrientation? getDeviceOrientation {
    switch (this) {
      case DeviceOrientationCode.portraitUp:
        return DeviceOrientation.portraitUp;
      ...
      case DeviceOrientationCode.none:
        return null;
    }
  }
}

I considered this before, but it did feel a bit redundant to wrap the existing enum only to add one value, but this may be a cleaner solution.

[camsim99]

@christopherfujino Friendly ping -- hoping to get your feedback on my proposal!

[christopherfujino]

Oops, sorry I didn't respond to this.

@christopherfujino I agree that using Optional did make sense upon reflection. I also like the idea of an enum, but I think that using an enum like you did for each nullable orientation could get confusing.

What do you think about creating a different DeviceOrientation-like enum that is essentially the same as DeviceOrientation and maps to it but has an additional value of none that is a valid value for the nullable orientations. For example:

enum DeviceOrientationCode {
  portraitUp,
  ...
  none,
}

extension DeviceOrientationCodeExtension on DeviceOrientation {
  DeviceOrientation? getDeviceOrientation {
    switch (this) {
      case DeviceOrientationCode.portraitUp:
        return DeviceOrientation.portraitUp;
      ...
      case DeviceOrientationCode.none:
        return null;
    }
  }
}

Not sure I fully understand this--how would callers represent when they want whatever was the previous state? Or am I misunderstanding?

[christopherfujino]

Another option is to just copy paste the code for Optional, it's not much: https://github.com/google/quiver-dart/blob/master/lib/src/core/optional.dart

[camsim99]

@christopherfujino I think I may have read your initial proposal incorrectly, and maybe that's where the confusion is coming from?

The fields impacted by this are all DeviceOrientations, so my solution was essentially saying that users would DeviceOrientationExtension instead of DeviceOrientation where DeviceOrientationExtension is the same as DeviceOrientation, with an extra value to specify none/null. Thus, a user would not specify anything in copyWith() to keep the value the same, but would specify DeviceOrientationExtension.none, for instance, to intend for the value of one of the respective field to be cleared, if that makes sense. Also, DeviceOrientationExtension.none would be the default value for all of the fields.

I think that using the Optional code may be a good middle ground, though, since that would avoid the redundant enum and what I think would be a breaking change.

[camsim99]

@hellohuanlin @christopherfujino I added back the Optional type. I put the code within the camera_controller.dart files themselves -- let me know what you think. I was considering putting it in the platform interface because I feel it would make more sense organizationally there, but then I believe this would have to be a breaking change.

[camsim99]

@christopherfujino ping on review for this!

[christopherfujino]

LGTM, sorry for the delay

[hellohuanlin]

i'm a bit confused about this part - is Optional the old "nullable" before null safety in dart?

[hellohuanlin]

The combination of optional and nullable feels a bit dangerous to me. Is it just a short-term fix? if so, what's the long term plan?

[stuartmorgan]

Fundamentally, to support the copyWith pattern with nullable values, you need to be able to express a nested nullable thing. It's unusual, but there isn't really a better pattern for it. The states we need to be able to represent are:

• I want to leave the old value unchanged
• I want to override the old value with a specific, non-null value
• I want to override the old value with null

[hellohuanlin]

Just reading the code, it's unclear to me which one represent "leave the old value unchanged" and which one represent "override the old value with null". IMO using a more descriptive enum is clearer, despite of slightly more code.

[christopherfujino]

Please read other code review comments, as there has already been quite a bit of discussion on different approaches.

[hellohuanlin]

@christopherfujino I read through the comments, but i'm not convinced that Optional<X>? is the best choice among alternatives. I think the benefit of clearer API outweights the drawback of slightly more coding (if it's more coding at all, since you are also copying the entire Optional class here).

As for API change, i think the old API is justified to be gone - in fact, having compatible API may be potentially dangerous, since it can get overlooked and misused.

[hellohuanlin]

If the underlying value didn't happen to be an enum, how would you express this?

Since dart does not support enum with associated value, we have to use a separate boolean flag (like shouldLeaveOldValue example you gave).

This seems fixable by renaming. E.g., OptionalReplacement with shouldLeaveOldValue (a bool) and newValue (a T) accessors.

Yes i think this approach is better than Optional<T>?. And since the underlying type is an enum, we can also combine the 2 parameters into 1 by defining a new enum. I think either is better than Optional<T>?.

[stuartmorgan]

Yes i think this approach is better than Optional<T>?.

To clarify, I'm suggesting something equivalent to (a stripped down) Optional<T> but called OptionalReplacement<T>, and with different accessors.

And since the underlying type is an enum, we can also combine the 2 parameters into 1 by defining a new enum.

Again, that would only work for this one specific data type. We should not take a general problem that we are likely to need to solve in the future for another datatype, and intentionally solve it in a way that can't scale to those other types.

[hellohuanlin]

Again, that would only work for this one specific data type. We should not take a general problem that we are likely to need to solve in the future for another datatype, and intentionally solve it in a way that can't scale to those other types.

For a general solution, I think an OptionalReplacement with a shouldLeaveOldValue property makes sense - it combines the 2 parameters I talked about into 1 single type, and it works for all types (enum vs non-enum).

To make sure we are talking about the same thing, something like this right?

class OptionalReplacement<T> {
  bool shouldLeaveOldValue
  T? replacement
}

Though I view it as "powered-up" (rather than "stripped-down") Optional since it adds a new capability to represent "leave old value".

[stuartmorgan]

class OptionalReplacement<T> {
  bool shouldLeaveOldValue
  T? replacement
}

T replacement, because there's no reason it couldn't be used for both nullable and non-nullable types. (Arguably the API would be better if all the parameters used it.)

Though I view it as "powered-up" (rather than "stripped-down") Optional since it adds a new capability to represent "leave old value".

Sorry, I forgot that this inverted the wrapping relative to the old semantics where it was the nullability of the wrapper that decided whether it was replacing or not. Despite the other issues, one advantage the old version had was that it was consistent about what a null parameter meant, which is an argument for keeping that version.

However, looking at the history a bit more here: the signature of the public copyWith method of a public type was changed without changing the major version of the plugin? That's bad (unless I'm misreading); changing the type of existing parameters is absolutely a breaking change. If that's indeed what happened, we should minimize the damage by replacing it with the copy of Optional with no changes because it minimize the chances that this break will break someone who hasn't yet updated to get the breaking change and been broken by it. If we want to actually redesign this API, it should be in a follow-up breaking change (with proper versioning) after the damage control fix goes out.

[christopherfujino]

However, looking at the history a bit more here: the signature of the public copyWith method of a public type was changed without changing the major version of the plugin? That's bad (unless I'm misreading); changing the type of existing parameters is absolutely a breaking change. If that's indeed what happened, we should minimize the damage by replacing it with the copy of Optional with no changes because it minimize the chances that this break will break someone who hasn't yet updated to get the breaking change and been broken by it. If we want to actually redesign this API, it should be in a follow-up breaking change (with proper versioning) after the damage control fix goes out.

Agreed. I believe landing this PR as is would satisfy this (allowing users who were broken by the upgrade from 0.10.1 -> 0.10.2 to get essentially an API revert back to 0.10.1).

[auto-submit]

auto label is removed for flutter/plugins, pr: 6911, due to - This commit is not mergeable and has conflicts. Please rebase your PR and fix all the conflicts.