Skip to content

Conversation

@goderbauer
Copy link
Member

@goderbauer goderbauer commented Mar 22, 2023

Fixes #123087.

The reason why these now-removed asserts existed was to guide people in the migration from the old deprecated to the new methods. The old deprecated methods were removed in #98616, but we forgot to remove these asserts.

This change makes (insert|move|remove)RenderObjectChild abstract again, like the old ones were before they were deprecated, see https://github.com/flutter/flutter/pull/64254/files.

@flutter-dashboard flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. labels Mar 22, 2023
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From context and from the PR [1] that introduced the allowMoveRenderObjectChild flag it seemed like the assumption is that moveRenderObjectChild is never called when allowMoveRenderObjectChild is false, hence this change here.

[1] 2425814#diff-4ea2907696de4fecf42f2ca56278a7b1d90c28921b1259d18ae28b1a2553d8e8L413

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL!

@goderbauer goderbauer requested a review from Piinks March 22, 2023 20:30
@Piinks Piinks added c: contributor-productivity Team-specific productivity, code health, technical debt. a: error message Error messages from the Flutter framework c: tech-debt Technical debt, code quality, testing, etc. labels Mar 22, 2023
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you for cleaning this up!

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah it looks like render_object_element_test.dart has some expectations around these assertions.

@goderbauer goderbauer added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 22, 2023
@goderbauer goderbauer force-pushed the deprecationFollowUp branch from 94d5409 to 477366e Compare March 22, 2023 22:41
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 23, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Mar 23, 2023

auto label is removed for flutter/flutter, pr: 123276, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@goderbauer goderbauer force-pushed the deprecationFollowUp branch from 477366e to 20a7a0a Compare March 23, 2023 16:14
@goderbauer goderbauer added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 23, 2023
@goderbauer goderbauer force-pushed the deprecationFollowUp branch from 20a7a0a to b292e21 Compare March 23, 2023 18:00
@goderbauer goderbauer merged commit 7f41ab2 into flutter:master Mar 23, 2023
@goderbauer goderbauer deleted the deprecationFollowUp branch March 23, 2023 19:34
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 23, 2023
tarrinneal pushed a commit to flutter/packages that referenced this pull request Mar 24, 2023
* cbdee52 Roll Flutter Engine from 59acb5362098 to 12c822327825 (3 revisions) (flutter/flutter#123330)

* 897e3db Inject the gstatic CanvasKit CDN URL by default in `flutter build web` (flutter/flutter#122772)

* 5a36bdd Stop serving Observatory by default (flutter/flutter#122419)

* b212e7b implement Iterator and Comparable instead of extending them (flutter/flutter#123282)

* af6029c c0fbe5a53 Roll Dart SDK from 9256fffbd5af to e8e045620234 (1 revision) (flutter/engine#40561) (flutter/flutter#123336)

* 9dec4fb FIX: NavigationDrawer hover/focus/pressed does not use indicatorShape (flutter/flutter#123325)

* 4907464 20ab040cd Roll Skia from ce5ff5cc03ce to c42320d53714 (2 revisions) (flutter/engine#40565) (flutter/flutter#123340)

* 28d40a4 Roll Packages from 75491e9 to 0826798 (5 revisions) (flutter/flutter#123342)

* 674ff15 [macOS] Add platform_channel sample/test (flutter/flutter#123141)

* 7b7af9f roll packages (flutter/flutter#123339)

* 3179875 replace some ._() constructors with class modifiers (flutter/flutter#122765)

* 7f41ab2 Fix (insert|move|remove)RenderObjectChild methods in base class (flutter/flutter#123276)

* fccca49 Refactor buildOverscrollIndicator (flutter/flutter#123246)

* 11bbce1 6b0933e74 [web] Add `dart:js_interop` to `_embedder.yaml`. (flutter/engine#40545) (flutter/flutter#123347)

* 716d252 Remove prefer_const_constructors ignores (flutter/flutter#123284)

* 100cf21 Prefer enum over class. (flutter/flutter#123312)

* 5ef9b84 Expose toggle to textfield's opacity animation. (flutter/flutter#122474)

* d79f3aa Roll Flutter Engine from 6b0933e74965 to bdce896fb64f (2 revisions) (flutter/flutter#123351)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
nploi pushed a commit to nploi/packages that referenced this pull request Jul 16, 2023
)

* cbdee52 Roll Flutter Engine from 59acb5362098 to 12c822327825 (3 revisions) (flutter/flutter#123330)

* 897e3db Inject the gstatic CanvasKit CDN URL by default in `flutter build web` (flutter/flutter#122772)

* 5a36bdd Stop serving Observatory by default (flutter/flutter#122419)

* b212e7b implement Iterator and Comparable instead of extending them (flutter/flutter#123282)

* af6029c c0fbe5a53 Roll Dart SDK from 9256fffbd5af to e8e045620234 (1 revision) (flutter/engine#40561) (flutter/flutter#123336)

* 9dec4fb FIX: NavigationDrawer hover/focus/pressed does not use indicatorShape (flutter/flutter#123325)

* 4907464 20ab040cd Roll Skia from ce5ff5cc03ce to c42320d53714 (2 revisions) (flutter/engine#40565) (flutter/flutter#123340)

* 28d40a4 Roll Packages from 75491e9 to 0826798 (5 revisions) (flutter/flutter#123342)

* 674ff15 [macOS] Add platform_channel sample/test (flutter/flutter#123141)

* 7b7af9f roll packages (flutter/flutter#123339)

* 3179875 replace some ._() constructors with class modifiers (flutter/flutter#122765)

* 7f41ab2 Fix (insert|move|remove)RenderObjectChild methods in base class (flutter/flutter#123276)

* fccca49 Refactor buildOverscrollIndicator (flutter/flutter#123246)

* 11bbce1 6b0933e74 [web] Add `dart:js_interop` to `_embedder.yaml`. (flutter/engine#40545) (flutter/flutter#123347)

* 716d252 Remove prefer_const_constructors ignores (flutter/flutter#123284)

* 100cf21 Prefer enum over class. (flutter/flutter#123312)

* 5ef9b84 Expose toggle to textfield's opacity animation. (flutter/flutter#122474)

* d79f3aa Roll Flutter Engine from 6b0933e74965 to bdce896fb64f (2 revisions) (flutter/flutter#123351)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: error message Error messages from the Flutter framework autosubmit Merge PR when tree becomes green via auto submit App c: contributor-productivity Team-specific productivity, code health, technical debt. c: tech-debt Technical debt, code quality, testing, etc. f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RenderObjectElement insertRenderObjectChild seems to throw an error when it shouldn't

2 participants