@bparrishMines I'm using this PR to do some cleanup I need to do to match the internal plugin and use your updated instance manager. I was wondering where this is used? As the comment suggests, I figured this was used for hot restart, but I didn't see it used in your example.

It's used in the initial creation of BaseObject.globalInstanceManager: https://github.com/bparrishMines/packages/blob/wrapper_example/packages/wrapper_example/lib/src/base_object.dart#L74.

This should translate to be used by your JavaObject class.

I would also note that this also breaks some tests. So I had to mock the host api in the tests:

@GenerateMocks(<Type>[TestInstanceManagerHostApi])
void main() {
  TestWidgetsFlutterBinding.ensureInitialized();

  // Mocks the call to clear the native InstanceManager.
  TestInstanceManagerHostApi.setup(MockTestInstanceManagerHostApi());
...

What do you think about rearranging this code to the following.

CameraSelector.Builder cameraSelectorBuilder = cameraXProxy.createCameraSelectorBuilder();
if (lensFacing != null) { 
    cameraSelectorBuilder = cameraSelectorBuilder.requireLensFacing(lensFacing.intValue());
}
instanceManager.addDartCreatedInstance(cameraSelectorBuilder.build(), identifier);

Did you decide not to do this?

This class looks like it has to check if the manager is closed in every method. That design pattern of requiring work in every method can be dangerous/tedious. It appears that each method has bespoke error conditions so that makes building a helper more difficult but here are some suggestions I would like you to consider.

1. Class method structure:
   For every public method make a private method of the same name. The only logic in public methods is the check for close and error return value.
   As an example

public boolean containsInstance(Object instance) {
    if (isClosed()) {
      Log.w(TAG, CLOSED_WARNING);
      return false;
    }
    return constainsInstanceUnsafe(instance);
  }
// Probably a bad name. 
private boolean constainsInstanceUnsafe(Object instance) {
 return identifiers.containsKey(instance)
}

2. Change the method definitions to only return the expected values and throw a typed error when the instance is called. This requires callers to catch the error but can make this class easier to maintain. This may or may not be combined with the first suggestion. What is nice this this behavior can be contained in a method and documentation added that says that every class in this method must call verifyNotClosed() first.
3. Something else

@bparrishMines what do you think about these alternatives?

1. Class method structure:
   For every public method make a private method of the same name. The only logic in public methods is the check for close and error return value.

I don't see how this is much different than the current structure. It just moves the method logic to a private class. Is it not dangerous/tedious for the same reason?

2. Change the method definitions to only return the expected values and throw a typed error when the instance is called. This requires callers to catch the error but can make this class easier to maintain. This may or may not be combined with the first suggestion. What is nice this this behavior can be contained in a method and documentation added that says that every class in this method must call verifyNotClosed() first.

To verify, this is suggesting throwing a checked exception when closed? This could be a good alternative solution, but it would require adding a try/catch block in a lot of places. InstanceManager is used extensively in this design, so this would require majority of the methods of all the other classes to use a try/catch.

I do like the suggestion of verifyNotClosed. What do you think of adding something like the method below and require it in every method. This would at least reduce the need to log in every method.

private boolean assertNotClosed() {
  if (isClosed()) {
    Log.w(TAG, CLOSED_WARNING);
    return true;
  }
  return false;
}

Although checking the closed state in every method is not easy to maintain, this class is only really changed when fixing bugs. I don't see many reasons to add more than a few methods to it based on the small scope of the class.

@reidbaker Looking to land this so I can make progress elsewhere. Can you file an issue for this or discuss it here, but approve this PR for now? I can make any changes in a follow up PR.

"I don't see how this is much different than the current structure. It just moves the method logic to a private class. Is it not dangerous/tedious for the same reason?"
Yes it is but at least the structure of the code makes it obvious that it is a design pattern. I am not sure the same is obvious with the current reading of the code.

"To verify, this is suggesting throwing a checked exception when closed? This could be a good alternative solution, but it would require adding a try/catch block in a lot of places. " yes I tried to call that out in "This requires callers to catch the error but can make this class easier to maintain."

The difficulties in this class are the result of InstanceManager exporting the need to check if it is closed first. I am not sure if that means it is either an acceptable burden to continue the cost up the tree or if it means that InstanceManager should be refactored to remove the need to check if closed before any interaction.

@camsim99 can you find 30 minutes for you me and @bparrishMines to talk about this. I think the current structure is risky to maintain. I will approve the pr as is since most of this update is unrelated.

Did you decide not to do this?

"I don't see how this is much different than the current structure. It just moves the method logic to a private class. Is it not dangerous/tedious for the same reason?"
Yes it is but at least the structure of the code makes it obvious that it is a design pattern. I am not sure the same is obvious with the current reading of the code.

"To verify, this is suggesting throwing a checked exception when closed? This could be a good alternative solution, but it would require adding a try/catch block in a lot of places. " yes I tried to call that out in "This requires callers to catch the error but can make this class easier to maintain."

The difficulties in this class are the result of InstanceManager exporting the need to check if it is closed first. I am not sure if that means it is either an acceptable burden to continue the cost up the tree or if it means that InstanceManager should be refactored to remove the need to check if closed before any interaction.

@camsim99 can you find 30 minutes for you me and @bparrishMines to talk about this. I think the current structure is risky to maintain. I will approve the pr as is since most of this update is unrelated.

You probably want to update this value to say that it returns INSTANCE_CLOSED not -1.