-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Disconnect the channel message handler when releasing the AccessibilityBridge #18657
Conversation
Doh, it's the same as #17646. Thanks for fixing. Can we add a similar test? |
Added a test |
AccessibilityChannel channel = mock(AccessibilityChannel.class); | ||
AccessibilityBridge accessibilityBridge = | ||
setUpBridge(null, channel, null, null, null, null); | ||
verify(channel).setAccessibilityMessageHandler(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is already null, I'd perhaps make this a bit more real by also injecting an AccessibilityManager and let it be isEnabled() true either via mockito or shadowOf(accessibilityManager).setEnabled(true)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the test
AccessibilityManager mockManager = mock(AccessibilityManager.class); | ||
when(mockManager.isEnabled()).thenReturn(true); | ||
AccessibilityBridge accessibilityBridge = | ||
setUpBridge(null, mockChannel, mockManager, null, null, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant if you let it be isEnabled, then you can assert that the message handler is non-null before you release, at which point it's then null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a check for the non-null setAccessibilityMessageHandler
call done during AccessibilityBridge
setup before release
is called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 thanks!
AccessibilityManager mockManager = mock(AccessibilityManager.class); | ||
when(mockManager.isEnabled()).thenReturn(true); | ||
AccessibilityBridge accessibilityBridge = | ||
setUpBridge(null, mockChannel, mockManager, null, null, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 thanks!
Fixes flutter/flutter#57661