Keep `dirty` manipulations private to `Element` base class by goderbauer · Pull Request #109401 · flutter/flutter

goderbauer · 2022-08-11T17:57:50Z

I tried to create a custom Element that was a direct subclass of Element outside of the framework and it was impossible to implement this right because I couldn't properly set/unset the dirty flag when necessary. Element subclasses in the framework directly manipulate the private Element._dirty field, but subclasses outside of framework.dart don't have this option. This change keeps setting Element._dirty private to the Element class to give subclasses outside of the framework a fair chance. This is archived by:

Resetting _dirty in the default implementation of Element.performRebuild. Subclasses are now expected to call this super implementation to reset the flag.
Adding a force parameter to the Element.rebuild method to force rebuilding even if the Element isn't dirty. This is convenient to call from implementations of update in subclasses.

I don't expect this to be a breaking change. Elements implemented outside the framework, who are not calling super.performRebuild already, are essentially broken because they can't reset the dirty flag properly. Therefore, I am not expecting any implementations out there that override performRebuild without already calling super. Furthermore, the addition of a new parameter to rebuild shouldn't break any subclasses since subclasses should be overriding performRebuild instead and that signature is unchanged. (However, I did find one class in google3 that overrides rebuild directly and that override needs to be updated to include the newly added force parameter.)

Alternatives considered:

Instead of adding force as a parameter to rebuild, add a new forceRebuild method. While this would be non-breaking in google3, it would deepen the call stack if we want to call into forceRebuild from rebuild to re-use code.
Instead of introducing a new force rebuild functionality, have the base implementation of Element.update set the dirty flag to true so subclasses can call the regular rebuild method in their update method (since the element is now dirty, rebuild will actually do the rebuilding work). While this would work for all Element subclasses implemented in the framework, this would force every Element to always rebuild in update to clear the dirty flag. Conceptually, this seems incorrect as there could in theory be updates that don't require a rebuild because the update is essentially a no-op.
Instead of having Element.performRebuild reset the dirty flag, provide a specific API to mark an Element as clean again (i.e. Element.markAsClean()). I am a little hesitant to give Element more API surface and adding that extra API feels easier to miss-use then having performRebuild clear the flag.

goderbauer · 2022-08-11T19:35:10Z

@Hixie Curious to hear what you think about this or whether you have another suggestion for addressing this.

goderbauer · 2022-08-12T18:02:56Z

Global presubmit in google (with that one fix mentioned in the PR description) came back green, g3fix for that fix is posted in cl/467248749.

Hixie

gspencergoog · 2022-08-13T01:21:19Z

I think this broke the build by causing an integration test to time out.

The test that hangs is here:

flutter/packages/flutter_tools/test/integration.shard/break_on_framework_exceptions_test.dart

Line 674 in dce82f7

testWithoutContext('breaks when rebuilding dirty elements throws', () async {

I'm assuming that this is causing the hang because of some change in the rebuild code that the integration test is testing.

I'm going to revert this to green the build. (I'm gardener this week)

…lutter#109401)" This reverts commit a624cb7 because it appears to have broken the build.

…109401)" (#109481) Landing on red to green the build.

…lutter/flutter#109401)

…lutter#109401)" This reverts commit 7887ca5.

…109401)" (#109562)

* 7d1323c Updating expiring cirrus key. (flutter/flutter#109461) * a85902a Roll Flutter Engine from de118a55ade0 to c354e0e04cea (2 revisions) (flutter/flutter#109462) * 28de3d4 Roll Flutter Engine from c354e0e04cea to d1b18b5d6811 (1 revision) (flutter/flutter#109466) * a624cb7 Keep `dirty` manipulations private to `Element` base class (flutter/flutter#109401) * dce82f7 Remove deprecated Ruby File.exists? in helper script (flutter/flutter#109428) * 7887ca5 Revert "Keep `dirty` manipulations private to `Element` base class (#109401)" (flutter/flutter#109481) * 237a298 Roll Flutter Engine from d1b18b5d6811 to a2f3bd58ce73 (1 revision) (flutter/flutter#109472) * fe2fc8d Single tap on the previous selection should toggle the toolbar on iOS… (flutter/flutter#108913) * 696c6fb Roll Flutter Engine from a2f3bd58ce73 to fb19700742c3 (6 revisions) (flutter/flutter#109482) * 17bc0ce Roll Flutter Engine from fb19700742c3 to 2180e62f6c7e (1 revision) (flutter/flutter#109483) * 4265015 Roll Flutter Engine from 2180e62f6c7e to cd1a1cb757f5 (1 revision) (flutter/flutter#109488) * c142dd3 Roll Flutter Engine from cd1a1cb757f5 to 5cc5c5f04f0a (1 revision) (flutter/flutter#109490) * 8a55c36 Roll Flutter Engine from 5cc5c5f04f0a to 18dca3de7496 (2 revisions) (flutter/flutter#109493) * d50c5b1 Roll Flutter Engine from 18dca3de7496 to 83771593cb55 (1 revision) (flutter/flutter#109505) * 97901da Cleaner test.dart output. (flutter/flutter#109206) * fb3c8ea Roll Flutter Engine from 83771593cb55 to f49a617535af (1 revision) (flutter/flutter#109509) * 5fcd9c2 Update README.md (flutter/flutter#109506) * 4dd7065 Add onOpened callback to PopupMenuButton (flutter/flutter#103753) * ed3238e Roll Flutter Engine from f49a617535af to cc9f4c63b7e5 (1 revision) (flutter/flutter#109518) * 8b1ca3f Roll Flutter Engine from cc9f4c63b7e5 to 4656c2f46cad (1 revision) (flutter/flutter#109524) * 20ffb32 Roll Flutter Engine from 4656c2f46cad to 16fb19e72f88 (1 revision) (flutter/flutter#109526) * 087750f Roll Flutter Engine from 16fb19e72f88 to 9c3c233e2639 (1 revision) (flutter/flutter#109532) * 4aba124 Roll Flutter Engine from 9c3c233e2639 to f0c3829e90ce (1 revision) (flutter/flutter#109536) * 2bce108 Roll Plugins from 9fb7654 to 094f1c3 (7 revisions) (flutter/flutter#109543) * 97d9a2f Roll Flutter Engine from f0c3829e90ce to 8c019cdd446f (2 revisions) (flutter/flutter#109548) * 458d618 Roll Flutter Engine from 8c019cdd446f to 198b0051a5a2 (1 revision) (flutter/flutter#109552) * c873c21 Roll Flutter Engine from 198b0051a5a2 to 33fccf564973 (1 revision) (flutter/flutter#109557)

…lutter#109401)" (flutter#109562)

* 7d1323c Updating expiring cirrus key. (flutter/flutter#109461) * a85902a Roll Flutter Engine from de118a55ade0 to c354e0e04cea (2 revisions) (flutter/flutter#109462) * 28de3d4 Roll Flutter Engine from c354e0e04cea to d1b18b5d6811 (1 revision) (flutter/flutter#109466) * a624cb7 Keep `dirty` manipulations private to `Element` base class (flutter/flutter#109401) * dce82f7 Remove deprecated Ruby File.exists? in helper script (flutter/flutter#109428) * 7887ca5 Revert "Keep `dirty` manipulations private to `Element` base class (#109401)" (flutter/flutter#109481) * 237a298 Roll Flutter Engine from d1b18b5d6811 to a2f3bd58ce73 (1 revision) (flutter/flutter#109472) * fe2fc8d Single tap on the previous selection should toggle the toolbar on iOS… (flutter/flutter#108913) * 696c6fb Roll Flutter Engine from a2f3bd58ce73 to fb19700742c3 (6 revisions) (flutter/flutter#109482) * 17bc0ce Roll Flutter Engine from fb19700742c3 to 2180e62f6c7e (1 revision) (flutter/flutter#109483) * 4265015 Roll Flutter Engine from 2180e62f6c7e to cd1a1cb757f5 (1 revision) (flutter/flutter#109488) * c142dd3 Roll Flutter Engine from cd1a1cb757f5 to 5cc5c5f04f0a (1 revision) (flutter/flutter#109490) * 8a55c36 Roll Flutter Engine from 5cc5c5f04f0a to 18dca3de7496 (2 revisions) (flutter/flutter#109493) * d50c5b1 Roll Flutter Engine from 18dca3de7496 to 83771593cb55 (1 revision) (flutter/flutter#109505) * 97901da Cleaner test.dart output. (flutter/flutter#109206) * fb3c8ea Roll Flutter Engine from 83771593cb55 to f49a617535af (1 revision) (flutter/flutter#109509) * 5fcd9c2 Update README.md (flutter/flutter#109506) * 4dd7065 Add onOpened callback to PopupMenuButton (flutter/flutter#103753) * ed3238e Roll Flutter Engine from f49a617535af to cc9f4c63b7e5 (1 revision) (flutter/flutter#109518) * 8b1ca3f Roll Flutter Engine from cc9f4c63b7e5 to 4656c2f46cad (1 revision) (flutter/flutter#109524) * 20ffb32 Roll Flutter Engine from 4656c2f46cad to 16fb19e72f88 (1 revision) (flutter/flutter#109526) * 087750f Roll Flutter Engine from 16fb19e72f88 to 9c3c233e2639 (1 revision) (flutter/flutter#109532) * 4aba124 Roll Flutter Engine from 9c3c233e2639 to f0c3829e90ce (1 revision) (flutter/flutter#109536) * 2bce108 Roll Plugins from 9fb7654 to 094f1c3 (7 revisions) (flutter/flutter#109543) * 97d9a2f Roll Flutter Engine from f0c3829e90ce to 8c019cdd446f (2 revisions) (flutter/flutter#109548) * 458d618 Roll Flutter Engine from 8c019cdd446f to 198b0051a5a2 (1 revision) (flutter/flutter#109552) * c873c21 Roll Flutter Engine from 198b0051a5a2 to 33fccf564973 (1 revision) (flutter/flutter#109557)

flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. labels Aug 11, 2022

goderbauer changed the title ~~Keep dirty setter private to Element~~ Keep dirty manipulations private to Element base class Aug 11, 2022

goderbauer added 2 commits August 11, 2022 11:40

Keep dirty setter private to Element

777d6c9

fix typo

f2658dd

goderbauer force-pushed the keepDirtyPrivate branch from caf3b6c to e5f7399 Compare August 11, 2022 18:44

fix test

7746c21

goderbauer force-pushed the keepDirtyPrivate branch from e5f7399 to 7746c21 Compare August 11, 2022 21:31

Hixie approved these changes Aug 12, 2022

View reviewed changes

goderbauer merged commit a624cb7 into flutter:master Aug 12, 2022

goderbauer deleted the keepDirtyPrivate branch August 12, 2022 19:54

gspencergoog added a commit to gspencergoog/flutter that referenced this pull request Aug 13, 2022

Revert "Keep dirty manipulations private to Element base class (f…

46855ca

…lutter#109401)" This reverts commit a624cb7 because it appears to have broken the build.

gspencergoog added a commit that referenced this pull request Aug 13, 2022

Revert "Keep dirty manipulations private to Element base class (#…

7887ca5

…109401)" (#109481) Landing on red to green the build.

This was referenced Aug 13, 2022

Roll Flutter from c91b481263da to 8a55c366eb2d (50 revisions) flutter/packages#2456

Closed

Roll Flutter from 9ad052a57e0f to 8a55c366eb2d (49 revisions) flutter/plugins#6241

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 13, 2022

a624cb7 Keep dirty manipulations private to Element base class (f…

0e94acd

…lutter/flutter#109401)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 13, 2022

a624cb7 Keep dirty manipulations private to Element base class (f…

188ebab

…lutter/flutter#109401)

engine-flutter-autoroll mentioned this pull request Aug 13, 2022

Roll Flutter from 9ad052a57e0f to 8a55c366eb2d (49 revisions) flutter/plugins#6242

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 13, 2022

a624cb7 Keep dirty manipulations private to Element base class (f…

ac133c2

…lutter/flutter#109401)

engine-flutter-autoroll mentioned this pull request Aug 13, 2022

Roll Flutter from c91b481263da to 8a55c366eb2d (50 revisions) flutter/packages#2457

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 13, 2022

a624cb7 Keep dirty manipulations private to Element base class (f…

fb26f3b

…lutter/flutter#109401)

This was referenced Aug 14, 2022

Roll Flutter from c91b481263da to fb3c8ea2b44a (53 revisions) flutter/packages#2458

Closed

Roll Flutter from 9ad052a57e0f to fb3c8ea2b44a (52 revisions) flutter/plugins#6243

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2022

a624cb7 Keep dirty manipulations private to Element base class (f…

37ec004

…lutter/flutter#109401)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 14, 2022

a624cb7 Keep dirty manipulations private to Element base class (f…

32c6027

…lutter/flutter#109401)

engine-flutter-autoroll mentioned this pull request Aug 14, 2022

Roll Flutter from c91b481263da to 5fcd9c255eb3 (54 revisions) flutter/packages#2459

Closed

engine-flutter-autoroll mentioned this pull request Aug 14, 2022

Roll Flutter from c91b481263da to 5fcd9c255eb3 (54 revisions) flutter/packages#2460

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2022

a624cb7 Keep dirty manipulations private to Element base class (f…

2ac3178

…lutter/flutter#109401)

This was referenced Aug 15, 2022

Roll Flutter from c91b481263da to 4aba1248c6c4 (60 revisions) flutter/packages#2462

Closed

Roll Flutter from 9ad052a57e0f to 4aba1248c6c4 (59 revisions) flutter/plugins#6268

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2022

a624cb7 Keep dirty manipulations private to Element base class (f…

bd2c699

…lutter/flutter#109401)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 15, 2022

a624cb7 Keep dirty manipulations private to Element base class (f…

d6f73dc

…lutter/flutter#109401)

This was referenced Aug 15, 2022

Roll Flutter from c91b481263da to 2bce10866592 (61 revisions) flutter/packages#2463

Closed

Roll Flutter from 9ad052a57e0f to 2bce10866592 (60 revisions) flutter/plugins#6269

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2022

a624cb7 Keep dirty manipulations private to Element base class (f…

dbc6cb7

…lutter/flutter#109401)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 15, 2022

a624cb7 Keep dirty manipulations private to Element base class (f…

de68a20

…lutter/flutter#109401)

This was referenced Aug 15, 2022

Roll Flutter from c91b481263da to 97d9a2f0d601 (62 revisions) flutter/packages#2464

Closed

Roll Flutter from 9ad052a57e0f to 97d9a2f0d601 (61 revisions) flutter/plugins#6270

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2022

a624cb7 Keep dirty manipulations private to Element base class (f…

a02df8e

…lutter/flutter#109401)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 15, 2022

a624cb7 Keep dirty manipulations private to Element base class (f…

0eedeb6

…lutter/flutter#109401)

This was referenced Aug 15, 2022

Roll Flutter from c91b481263da to 458d618ad693 (63 revisions) flutter/packages#2465

Closed

Roll Flutter from 9ad052a57e0f to 458d618ad693 (62 revisions) flutter/plugins#6273

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2022

a624cb7 Keep dirty manipulations private to Element base class (f…

c138a3d

…lutter/flutter#109401)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 15, 2022

a624cb7 Keep dirty manipulations private to Element base class (f…

675db76

…lutter/flutter#109401)

This was referenced Aug 15, 2022

Roll Flutter from a0f1c670ed90 to c873c2100ad7 (27 revisions) flutter/packages#2466

Merged

Roll Flutter from a0f1c670ed90 to c873c2100ad7 (27 revisions) flutter/plugins#6275

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2022

a624cb7 Keep dirty manipulations private to Element base class (f…

08bd8bc

…lutter/flutter#109401)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 15, 2022

a624cb7 Keep dirty manipulations private to Element base class (f…

bf77c4e

…lutter/flutter#109401)

goderbauer added a commit to goderbauer/flutter that referenced this pull request Aug 15, 2022

Reland "Keep dirty manipulations private to Element base class (f…

0527051

…lutter#109401)" This reverts commit 7887ca5.

auto-submit bot pushed a commit that referenced this pull request Aug 15, 2022

Reland "Keep dirty manipulations private to Element base class (#…

2b9beda

…109401)" (#109562)

goderbauer added a commit to goderbauer/flutter that referenced this pull request Aug 17, 2022

Reland "Keep dirty manipulations private to Element base class (f…

ab923dd

…lutter#109401)" (flutter#109562)

goderbauer mentioned this pull request Sep 13, 2022

Element subclasses can't clear dirty flag #99172

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep `dirty` manipulations private to `Element` base class#109401

Keep `dirty` manipulations private to `Element` base class#109401
goderbauer merged 3 commits intoflutter:masterfrom
goderbauer:keepDirtyPrivate

goderbauer commented Aug 11, 2022 •

edited

Loading

Uh oh!

goderbauer commented Aug 11, 2022

Uh oh!

goderbauer commented Aug 12, 2022

Uh oh!

Hixie left a comment

Uh oh!

gspencergoog commented Aug 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

goderbauer commented Aug 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goderbauer commented Aug 11, 2022

Uh oh!

goderbauer commented Aug 12, 2022

Uh oh!

Hixie left a comment

Choose a reason for hiding this comment

Uh oh!

gspencergoog commented Aug 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

goderbauer commented Aug 11, 2022 •

edited

Loading