Fix contradictory advice in "detach" docs; cut redundancy in "attach" #130688
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes flutter#115525. On [AbstractNode.detach] and its two progeny [RenderNode.detach] and [Layer.detach], the docs said both to call the inherited method before detaching children, and to end by doing so. The former advice is what's enforced by an assertion in the base implementation, so cut out the other. The corresponding [attach] methods redundantly said twice to call the inherited method first, so cut the redundancy. Leave in place the version more recently added (in flutter#76021), because that PR shows the old version must have been easy to overlook.
github-actions
bot
added
the
framework
8 tasks
Contributor
|
Would it make sense to phrase it something like "Implementations in subclasses should attach their children after calling the superclass method" (as opposed to "must start with calling the superclass method")? (Maybe with better phrasing, that's not my best work!) The point being to avoid implying that doing unrelated work before calling the superclass is a problem. |
Member
Author
|
Yeah. Perhaps like this? /// Subclasses with children should override this method to
/// [attach] all their children to the same [owner]
/// after calling the inherited method, as in `super.attach(owner)`. /// Subclasses with children should override this method to
/// [detach] all their children after calling the inherited
/// method, as in `super.detach()`.That is:
This version (unlike what's in main) doesn't mention that implementations even in subclasses without children should call the superclass method. But |
Contributor
|
SGTM. |
Member
Author
|
Cool. Pushed a revision with that text. |
gnprice
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
Contributor
|
auto label is removed for flutter/flutter, pr: 130688, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
goderbauer
added
the
autosubmit
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 20, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 20, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 20, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 21, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 21, 2023
LouiseHsu
pushed a commit
to LouiseHsu/flutter
that referenced
this pull request
Jul 31, 2023
…flutter#130688) Fixes flutter#115525. On [AbstractNode.detach] and its two progeny [RenderNode.detach] and [Layer.detach], the docs said both to call the inherited method before detaching children, and to end by doing so. The former advice is what's enforced by an assertion in the base implementation, so cut out the other. The corresponding [attach] methods redundantly said twice to call the inherited method first, so cut the redundancy. Leave in place the version more recently added (in flutter#76021), because that PR shows the old version must have been easy to overlook.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #115525.
On [AbstractNode.detach] and its two progeny [RenderNode.detach]
and [Layer.detach], the docs said both to call the inherited method
before detaching children, and to end by doing so. The former
advice is what's enforced by an assertion in the base implementation,
so cut out the other.
The corresponding [attach] methods redundantly said twice to
call the inherited method first, so cut the redundancy.
Leave in place the version more recently added (in #76021), because
that PR shows the old version must have been easy to overlook.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.