Skip to content

fix Applying decoration for a table row widget will cause render exce… #34285

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.

Sign up for GitHub

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

Jump to bottom
Merged
chunhtai merged 1 commit into from
Jun 12, 2019
Merged

fix Applying decoration for a table row widget will cause render exce… #34285

chunhtai merged 1 commit into from
Jun 12, 2019

Conversation

@chunhtai
Copy link
Contributor

@chunhtai chunhtai commented Jun 11, 2019 •
edited
Loading

…ption

Description

Pushing an overlay route will cause the previous route to detach and rebuild. Table widget does not correctly handle the detach and rebuild. This pr fixes it.

Related Issues

#17243

Tests

I added the following tests:

"Table widget can be detached and re-attached"

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@chunhtai chunhtai added the framework flutter/packages/flutter repository. See also f: labels. label Jun 11, 2019
@chunhtai chunhtai requested a review from goderbauer June 11, 2019 21:50
goderbauer
goderbauer reviewed Jun 12, 2019
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

Cirrus is also unhappy.

packages/flutter/lib/src/rendering/table.dart Outdated
Copy link
Member

@goderbauer goderbauer Jun 12, 2019

Choose a reason for hiding this comment

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

Private members don't really belong in a public doc comment. What does the user of RenderTable need to do to ensure that?

Copy link
Member

@goderbauer goderbauer Jun 12, 2019

Choose a reason for hiding this comment

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

Maybe this should just be a comment starting with // instead of ///? And maybe move it one line down?

packages/flutter/test/widgets/table_test.dart Outdated
Copy link
Member

@goderbauer goderbauer Jun 12, 2019

Choose a reason for hiding this comment

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

add a type annotation to the List

packages/flutter/test/widgets/table_test.dart Outdated
Copy link
Member

@goderbauer goderbauer Jun 12, 2019

Choose a reason for hiding this comment

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

Add a trailing comma after this and the next line and then fix indentation (only 2 spaces)

packages/flutter/test/widgets/table_test.dart Outdated
Copy link
Member

@goderbauer goderbauer Jun 12, 2019

Choose a reason for hiding this comment

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

remove whitespace after "Center"

packages/flutter/lib/src/rendering/table.dart Outdated
Copy link
Member

@goderbauer goderbauer Jun 12, 2019

Choose a reason for hiding this comment

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

It seems odd that we change the configuration of the RenderObject. Aren't the rowDecorations user-provided?

@chunhtai
Copy link
Contributor Author

chunhtai commented Jun 12, 2019

addressed all coments

goderbauer
goderbauer approved these changes Jun 12, 2019
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

packages/flutter/lib/src/rendering/table.dart Outdated
Copy link
Member

@goderbauer goderbauer Jun 12, 2019

Choose a reason for hiding this comment

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

In a key place (maybe paint) add an assert for this condition?

@chunhtai chunhtai merged commit ad50772 into flutter:master Jun 12, 2019
tango5614 added a commit to tango5614/flutter that referenced this pull request Jun 13, 2019
@tango5614
Merge branch 'master' into fix28096
7389944 
* master: (84 commits)
  Compatibility pass on flutter/material tests for JavaScript compilation. (9) (flutter#33378)
  fix Applying decoration for a table row widget will cause render exception (flutter#34285)
  Add widget of the week videos (flutter#34356)
  Roll engine ab5c14b..67aadb6 (2 commits) (flutter#34350)
  Re-apply compressionState changes. (flutter#34341)
  Revert "update the Flutter.Frame event to use new engine APIs (flutter#34243)" (flutter#34352)
  Change Xcode project developmentRegion to 'en' and plist CFBundleDevelopmentRegion to DEVELOPMENT_LANGUAGE (flutter#34293)
  update the Flutter.Frame event to use new engine APIs (flutter#34243)
  [flutter_tool] Don't truncate verbose logs from _flutter.listViews (flutter#34255)
  ab5c14b Set Dart version to git hash 3166bbf24b0c929eef33fd5d0f69e0f36a9009f3 (Dart 2.3.3-dev) (flutter/engine#9292) (flutter#34336)
  Prepare for Uint8List SDK breaking changes (flutter#34295)
  Roll engine b14d971..d3c213e (3 commits) (flutter#34331)
  Roll engine 4fc95eb..b14d971 (4 commits) (flutter#34310)
  4fc95eb Fixed memory leaks within FlutterFragment and FlutterView (flutter#34268, flutter#34269, flutter#34270). (flutter/engine#9288) (flutter#34305)
  Roll engine 2d2cfc0..de350c4 (2 commits) (flutter#34302)
  Roll engine 02e6a13..2d2cfc0 (3 commits) (flutter#34292)
  Split gradle_plugin_test.dart (flutter#34282)
  Roll engine 4d68474..02e6a13 (4 commits) (flutter#34281)
  Compatibility pass on flutter/widgets tests for JavaScript compilation. (8) (flutter#33377)
  Instrument usage of include_flutter.groovy and xcode_backend.sh (flutter#34189)
  ...
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@goderbauer goderbauer goderbauer approved these changes

Assignees

No one assigned

Labels

framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@chunhtai @goderbauer @googlebot