Improve iOS a11y sync behavior #1170
Conversation
|
I'll do a minor refactor. |
| * A set of node ids that had their bounds invalidated after the last sync. | ||
| */ | ||
| private var invalidatedBoundsNodeIds = mutableSetOf<Int>() | ||
| private val invalidationChannel = Channel<Unit>(1, onBufferOverflow = BufferOverflow.DROP_LATEST) |
There was a problem hiding this comment.
This should be Channel<Unit>(Channel.CONFLATED). It's the same thing, but clearer. I'll change in on the desktop side too.
There was a problem hiding this comment.
I've changed the implementation so this one is not relevant anymore
| val updateIntervalMillis = 50L | ||
| // TODO: this approach was copied from desktop implementation, obviously it has a [updateIntervalMillis] lag | ||
| // between the actual change in the semantics tree and the change in the accessibility tree. | ||
| // should we use some other approach? | ||
| coroutineScope.launch { | ||
| while (isAlive) { |
There was a problem hiding this comment.
I don't think this isAlive is needed. When you cancel the Job, a CancellationException is thrown inside the coroutine.
There was a problem hiding this comment.
It makes sense, will change it
| coroutineScope.launch { | ||
| while (isAlive) { | ||
| var result: NodesSyncResult | ||
| var strategy = SemanticsTreeSyncStrategy.from( |
There was a problem hiding this comment.
It's not necessary to send this data through the channel.
The channel is just used as a synchronization primitive to trigger the coroutine to awake. The information about what to do when awaking can just be put in a regular data structure by onSemanticsChange and onLayoutChange.
There was a problem hiding this comment.
This approach has more straightforward data-flow though instead of scattering the states all around the class.
There was a problem hiding this comment.
It could affect performance, and IMO is not a good idea in general (mixing a synchronization utility with logic). Tomorrow there could be a better/faster way to trigger a coroutine to wake up (like Java's notify) that doesn't allow data to pass through.
But your call.
There was a problem hiding this comment.
So you'd insist to revert e8de6ee?
| while (true) { | ||
| // Consumes all the invalidations to select a proper strategy | ||
| val invalidation = invalidationChannel.tryReceive().getOrNull() ?: break | ||
| strategy = strategy.reduce(invalidation) | ||
| } |
There was a problem hiding this comment.
Extract this into a helper function (channel ext)
There was a problem hiding this comment.
I'd like to keep this code inline so the entire suspend -> consumeRemaining sequence is visible to a reader
There was a problem hiding this comment.
How is "suspend" related here?
There was a problem hiding this comment.
The receive call is two lines above, it's suspending and is a delimiter for a synchronous inner buffer consumption.
My intent was to make this entire logic visible to a reader.
There was a problem hiding this comment.
Reverted to pre-refactor
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/platform/Accessibility.uikit.kt
Show resolved
Hide resolved
elijah-semyonov
changed the title
Proposed Changes
Implement desktop-like
synclogic. Therendercan issue undefined amount of layout and semantics invalidation, that will be coalesced into a singlesyncevery time it happens.Implement different logic based on kind of invalidation to avoid extra when only bounds are changed.
Fix wrong refocusing logical bug.
Testing
Test: N/A