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
Remove remaining uses of childToSlot
#64273
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
Adding @goderbauer for context of the change (since he reviewed the earlier changes) Adding @HansMuller and @justinmc for the specific Material changes. |
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
@@ -747,8 +747,7 @@ class _CupertinoTextSelectionToolbarItemsElement extends RenderObjectElement { | |||
) : super(widget); | |||
|
|||
List<Element> _children; | |||
final Map<_CupertinoTextSelectionToolbarItemsSlot, Element> slotToChild = <_CupertinoTextSelectionToolbarItemsSlot, Element>{}; | |||
final Map<Element, _CupertinoTextSelectionToolbarItemsSlot> childToSlot = <Element, _CupertinoTextSelectionToolbarItemsSlot>{}; | |||
final Map<_CupertinoTextSelectionToolbarItemsSlot, Element> slottedChildren = <_CupertinoTextSelectionToolbarItemsSlot, Element>{}; |
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.
This name sounds like a list rather than a map (slotToChild sounds like a map)
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'll update it back.
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 modulo (my humble opinion) renaming the slotToChild properties.
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, just a naming question 👍
@@ -2066,8 +2066,7 @@ enum _ChipSlot { | |||
class _RenderChipElement extends RenderObjectElement { | |||
_RenderChipElement(_ChipRenderWidget chip) : super(chip); | |||
|
|||
final Map<_ChipSlot, Element> slotToChild = <_ChipSlot, Element>{}; | |||
final Map<Element, _ChipSlot> childToSlot = <Element, _ChipSlot>{}; | |||
final Map<_ChipSlot, Element> children = <_ChipSlot, Element>{}; |
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.
Why is this children
here but slottedChildren
in the cupertino file above?
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.
In the Cupertino file above, the element contains both an indexed list of children and a map of what it was referring to as "slotted children", so I made the map name match what it was referred to in the comments. However, it sounds like it's more confusing, so I'll rename all occurrences back to slotToChild
final _ChipSlot slot = childToSlot[child]; | ||
childToSlot.remove(child); | ||
slotToChild.remove(slot); | ||
assert(children.values.contains(child)); |
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.
Nit: I know you're just changing the variable name, but couldn't this be:
assert(children.values.contains(child)); | |
assert(children.containsValue(child)); |
Here and in a couple other places below.
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.
Yep - I'll update them all. I'm not used to having a containsValue()
function on a map 🙂
The remaining uses of this pattern were all due to wanting to have the child's slot when `Element.forgetChild()` was called. However, when that method is called, the child's `slot` value is still valid in the context of the parent, so the uses can just use `child.slot`. This is the final round of cleanup from the fallout of flutter#63269
The remaining uses of this pattern were all due to wanting to have the child's slot when `Element.forgetChild()` was called. However, when that method is called, the child's `slot` value is still valid in the context of the parent, so the uses can just use `child.slot`. This is the final round of cleanup from the fallout of flutter#63269
The remaining uses of this pattern were all due to wanting to have the child's slot when `Element.forgetChild()` was called. However, when that method is called, the child's `slot` value is still valid in the context of the parent, so the uses can just use `child.slot`. This is the final round of cleanup from the fallout of flutter#63269
Description
The remaining uses of this pattern were all due to wanting to have
the child's slot when
Element.forgetChild()
was called. However,when that method is called, the child's
slot
value is still validin the context of the parent, so the uses can just use
child.slot
.Related Issues
This is the final round of cleanup from the fallout of #63269
Tests
Test exemption: this PR just removes code (and renames a few variables) - the existing tests passing validates the change.
Checklist
///
).flutter analyze --flutter-repo
) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read [Handling breaking changes].