Can you describe why we need this? It looks like it's enabling switching out an instance without letting its client know that the value switched. But I'm not sure why that's a good thing to do instead of just offering a mutator on the client and avoiding this class entirely.

In this case the client doesn't need to know that a change happened, it just needs to know the current value when it needs to use it. Note that this is not public API but a package private mechanism.

I'm assuming that by "offering a mutator" you mean exposing a setAccessibilityBridge call(?). I tend to prefer this solution where there's a single source of truth for the current a11y bridge vs. keeping N references in N clients, and propagating N set calls multiple levels down every time the value is changed, whether that new value is needed or not (in most common cases where a11y is off the value will never be used).

I agree that it's ugly to have a dedicated class just for this but I like the alternatives we've considered less (propagating set calls, or making this class expose update callbacks).

Happy to be convinced otherwise if you have good reasons to prefer a different approach.

In terms of this chosen approach, I think the only recommendation I would have is that you consider calling this class AccessibilityBridgeHolder because the concept of something being "current" is actually a runtime statement, and ending with AccessibilityBridge makes this class sound like a subclass of AccessibilityBridge.

With regard to this approach vs others, you suggested that there might be many setter methods required to replace this class, and that many would never be used. I'm curious why we would add setters, or even expose this class, to things that won't need it? I'm also curious how this approach reduces exposure across the API surface. Considering all of those classes that would have setters added to them, aren't they all being given a reference to this class, anyway?

If your analysis says that this is the most readable and maintainable option of those that exist, then maybe I'm just missing something. But I would definitely recommend considering if this approach isn't simply re-inventing what we would normally do with a variable and a setter. If it is, then it will probably be less readable and less maintainable over time for developers who are used to simply setting references on things. But it's up to you.

@amirh (optional) I thought of another potential alternative besides this, having to propagate all the set calls, or having to register callbacks. You could get rid of the getCurrent method on this holder class and instead wrap and expose all the AccessibilityBridge methods that you're expecting clients to call, so none of the clients are ever dealing with the Bridge directly and able to cache it. It would mean extra boilerplate and a maintenance burden though.

@matthew-carroll I think the root issue here is that there's a convoluted and long chain of instantiations, some of which require dependencies from upper levels. The same kind of issue that dependency injection practices and frameworks try to prevent. We need AccessiblityBridge constructed and maintained in PlatformViewsController because of onChange callbacks, but then it's used way over in AccessibilityDelegatingFrameLayout, which is constructed through an internal chain of new calls. So if setters were used PlatformViewsController would need to call VirtualDisplayController#setBridge which would need to call SingleViewPresentation#setBridge which would need to call AccessibilityDelegatingFrameLayout#setBridge which would then be the final thing to actually use the bridge. Pub/sub systems (like EventBus also address this type of problem, but if we want to bring in something like either that or use DI here it would be its own discussion/broader refactor.

Went with @mklim's suggestion, made this be an AccessibilityEventsDelegate.

We could potentially take this a step further in making sure no-one but PlatformViewsController sets the a11y bridge by splitting this into an interface and an implementation. But I think that this extra complexity isn't justified, as this is an internal class.

Can we just make this an Integer and use null for non-existence rather than a magic number?

done

Can you add appropriate javadocs to this method?

done

Can you include the meaning of the return value in the javadoc?

done

Same here with the return value.

done

Can you add a javadoc to this method?

done

I think this could use some huge warning labels on it for how long "current" is valid for.

obsolete now

I'm not sure the right way tor restrict this, but there's a potential bug here since clients could call this and override the actual current bridge with whatever they've cached from getCurrent. In general exposing this just makes it kind of unclear for how this state should be managed. I'd add least add code comments saying that this should only ever be called by the class controlling the AccessibilityBridge.

Would it be potentially worth taking lambda or something similar in the constructor and updating the bridge internally when it's triggered? That way the only class that can mess with the state is the person that constructed this originally. Not sure if that code complexity would be worth the guarantee of really guarding this setter though. I think it would end up being extremely verbose. Since we're just using this internally I think this is fine as long as it has a comment on it saying when this should be called.

So this suggestion is to do something like:

class AccessibilityEventsDelegate {
  private AccessibilityBridge accessibilityBridge;
  AccessibilityEventsDelegate(OnAccessibilityBridgeUpdated onAccessibilityBridgeUpdated) {
   onAccessibilityBridgeUpdated.setListener((accessibilityBridge) -> this.accessibilityBridge = accessibilityBridge );
  }
}

And then have PlatformViewsController to pass in a callback that's updated on attach/detach.

As noted above I don't believe the use circumstances here justifies this kind of code complexity.

@amirh (optional) I thought of another potential alternative besides this, having to propagate all the set calls, or having to register callbacks. You could get rid of the getCurrent method on this holder class and instead wrap and expose all the AccessibilityBridge methods that you're expecting clients to call, so none of the clients are ever dealing with the Bridge directly and able to cache it. It would mean extra boilerplate and a maintenance burden though.

@matthew-carroll I think the root issue here is that there's a convoluted and long chain of instantiations, some of which require dependencies from upper levels. The same kind of issue that dependency injection practices and frameworks try to prevent. We need AccessiblityBridge constructed and maintained in PlatformViewsController because of onChange callbacks, but then it's used way over in AccessibilityDelegatingFrameLayout, which is constructed through an internal chain of new calls. So if setters were used PlatformViewsController would need to call VirtualDisplayController#setBridge which would need to call SingleViewPresentation#setBridge which would need to call AccessibilityDelegatingFrameLayout#setBridge which would then be the final thing to actually use the bridge. Pub/sub systems (like EventBus also address this type of problem, but if we want to bring in something like either that or use DI here it would be its own discussion/broader refactor.

nit: Please rename to just currentAccessibilityBridge for this and the related variables.

https://google.github.io/styleguide/javaguide.html#s5.1-identifier-names

Our codebase right now is in this weird state were part of the newer files are following the Google Style guide and files from up to around 8 months ago are following something similar to the Android style guide.

We should migrate to a single style, but I think it should be done in dedicated PRs, and I wouldn't want to introduce mixed style to this file in this PR. I think we should follow up with a separate PR to rename all the fields here.

SGTM.

(subjective, not in scope for this change, doesn't need to be resolved): I think relying on null as a silent error condition like here is dangerous. It's hard to know when to look for it. Especially on a type like Integer that tends to get equated with the non nullable int. I'd rather there was a constant for INVALID that callers were expected to check for, and the method returned an int instead.

FYI, I actually asked @amirh to switch from a magic number to null. Can you elaborate on why null would be bad? Null is often used to express non-existence, whereas a value definitely exists but has some special meaning that is different than all other numbers. To me, the latter seemed more likely to confuse people, especially if you ever do a debug dump and see just the magic number instead of null. Are there likely situations where null would create issues?

Changed to check containsKey's return value.

Sure thing. I do think this is subjective so the patch as it is is also fine. And I agree magic numbers are unclear, so that's why I was suggesting a named constant for the error case specifically.

In general I think all deliberate uses of null are somewhat dangerous and prefer that non-nullable be default and enforced with static analysis if possible, so that's mostly what's prompting my suggestion here. "Null References: The Billion Dollar Mistake" is a really good talk going into deeper issues with it. Items 54-55 in Effective Java also touch on problems with null.

My main dislike of deliberate null returns comes down to a combination of two things.

• Nullable values are invisible. It's up to the programmer calling the method to proactively figure out that the return can be null and handle it appropriately, and usually the only tools at their disposal are untrustworthy manually added markers like annotations, comments, etc which can all be out of date, missing, or hard to notice in the first place. The only way to really know is to read the implementation details of every method that returns a potentially nullable type.
• Missing null means getting an NPE and a program crash, so checking for it is critically important at the same time as being really hard to know when to guard against. NPEs can crop up far removed from the original nullable getter and be generally hard to diagnose.

It's basically breaking down the type system. If any type can be either itself or null, then all code using nullable values needs to first basically do type checking to make sure the value is actually the thing that they're expecting before relying on it like it's an instance of its type. With this Integer case specifically I'd be especially worried about somebody not realizing that this method is nullable and just trying to cast it to int or even comparing it with another int with ==, which would cause an NPE.

These concerns outweigh the semantic harmony of literally returning "nothing" to match that we found "nothing" for me. I'd rather we return a status code labeled "NOT_FOUND" or similar for clients to handle.

Optional<int> is probably the better way to go in a lot of these kinds of cases. It points out that it's possible to get "nothing" back from a method in a deliberate way that's impossible to miss while coding. But I don't think it's available in Android APIs before 24.

I'd either tag this method with @Nullable and document that it returns null when one can't be found or convert it to use int with an INVALID constant for not found cases. It's subjective but I prefer the INVAILD constant option instead since it's more clear and can't potentially end up with NPEs.

I feel like returning a valid int value here is dangerous as misuse might result in an error that's much harder to spot (setting focus on a wrong a11y node, or setting a wrong child node) vs. an NPE.

I see the reasoning. I think adding the @Nullable annotations helps with the discoverability here at any rate, so at least now callers of the method have it declared for them.

Went with @mklim's suggestion, made this be an AccessibilityEventsDelegate.

We could potentially take this a step further in making sure no-one but PlatformViewsController sets the a11y bridge by splitting this into an interface and an implementation. But I think that this extra complexity isn't justified, as this is an internal class.

obsolete now

So this suggestion is to do something like:

class AccessibilityEventsDelegate {
  private AccessibilityBridge accessibilityBridge;
  AccessibilityEventsDelegate(OnAccessibilityBridgeUpdated onAccessibilityBridgeUpdated) {
   onAccessibilityBridgeUpdated.setListener((accessibilityBridge) -> this.accessibilityBridge = accessibilityBridge );
  }
}

And then have PlatformViewsController to pass in a callback that's updated on attach/detach.

As noted above I don't believe the use circumstances here justifies this kind of code complexity.

Our codebase right now is in this weird state were part of the newer files are following the Google Style guide and files from up to around 8 months ago are following something similar to the Android style guide.

We should migrate to a single style, but I think it should be done in dedicated PRs, and I wouldn't want to introduce mixed style to this file in this PR. I think we should follow up with a separate PR to rename all the fields here.

Changed to check containsKey's return value.

I feel like returning a valid int value here is dangerous as misuse might result in an error that's much harder to spot (setting focus on a wrong a11y node, or setting a wrong child node) vs. an NPE.

Is this line actually required? Wouldn't the event to null this out always be handled by the if you added above?

Discussed offline - we're leaving it here as a defensive measure in case the ACCESSIBLITY_FOCUS_CLEARED event is not sent from the currently accessibility focused node

What about INPUT_FOCUS for embedded views? Finding the node that has input focus is important for text input via a11y.

added tracking for input focus as well.

Do you enforce in the framework that we wrap the id generation at MIN_ENGINE_GENERATED_NODE_ID? If not, please do.

Sent flutter/flutter#29928