SemanticsDebugger should use SemanticsNodes directly #6252
Conversation
| /// | ||
| /// Typically obtained from [SemanticsNode.getSemanticsData]. | ||
| class SemanticsData { | ||
| /// Creates a semantics data object. |
There was a problem hiding this comment.
self-evident doc. though i'm not sure what we could add.
There was a problem hiding this comment.
We can say the fields must not be null.
| @required this.actions, | ||
| @required this.label, | ||
| @required this.rect, | ||
| @required this.transform |
There was a problem hiding this comment.
transform is required here but the doc lower down says it has a default
| flags |= node._flags; | ||
| actions |= node._actions; | ||
| if (label.isEmpty) | ||
| label = node._label; |
There was a problem hiding this comment.
shouldn't we merge the strings? I thought I merged the strings separated by newlines or something.
| @@ -497,6 +590,11 @@ class SemanticsOwner { | |||
|
|
|||
| final List<SemanticsListener> _listeners = <SemanticsListener>[]; | |||
|
|
|||
| /// The root node of the semantics tree, if any. | |||
| /// | |||
| /// The semantics tree is empty, returns null. | |||
| @@ -597,12 +695,11 @@ class SemanticsOwner { | |||
| _dirtyNodes.clear(); | |||
| } | |||
|
|
|||
| SemanticsActionHandler _getSemanticsActionHandlerForId(int id, { @required SemanticsAction action }) { | |||
| assert(action != null); | |||
There was a problem hiding this comment.
I removed it because I made the argument positional, but I can add it back. I've moved it to the public API so that if it trips it will be more obvious where the fault lies.
| SemanticsNode result = _nodes[id]; | ||
| if (result != null && result._shouldMergeAllDescendantsIntoThisNode && !result._hasAction(action)) { | ||
| result._visitDescendants((SemanticsNode node) { | ||
| if (node._actionHandler != null && node._hasAction(action)) { |
There was a problem hiding this comment.
why do we want to match if there's no action handler?
There was a problem hiding this comment.
_hasAction does the node._actionHandler internally.
There was a problem hiding this comment.
I've renamed this to _canPerformAction to make that more obvious.
|
LGTM. Since you have the read-only SemanticsData structure already, it seems like it'd be worth going all the way and not exposing the mutable data structures at all. Then you could just expose the SemanticsData objects directly on the RenderObjects. |
I'd like to try getting rid of the SemanticsData objects by moving the merging logic into the semantics compiler. Having the merge operation still pending in SemanticsNode tree is pretty awkward. |
Instead of reading the mojom serialization and re-inflating it, the SemanticsDebugger now shows the SemanticsNode objects directly.
1e85099 to
cfcc103
Compare
Instead of reading the mojom serialization and re-inflating it, the
SemanticsDebugger now shows the SemanticsNode objects directly.