Skip to content

Conversation

shihaohong
Copy link
Contributor

@shihaohong shihaohong commented Apr 17, 2019

Description

The current ExpansionPanelList implementation does not work correctly when wrapped with LayoutBuilder and possibly StreamBuilder because setState is being called when panel is pressed, triggering a rebuild of the ExpansionPanelList while LayoutBuilder is also trying to rebuild, causing duplicate global keys to be generated and throwing an exception. This seems to have been introduced when trying to manage expanded states for ExpansionPanelList.radio here.

This PR does the following:

  1. Moves setState to only be invoked when guarded by widget._allowMultiplePanelsOpen, fixing the case for ExpansionPanelList.
  2. Remove setting _currentOpenPanel to initialOpenPanelValue in didUpdateWidget, since this should only occur on initState and not every time the widget is updated. This fixes the problem for ExpansionPanelList.radio.

However, there are some design choices I want to consider before moving forward:

  1. ExpansionPanelList.radio, in this implementation, will call expansionCallback twice in the event that one panel is open prior to opening a new one. This is fine, but some documentation would have to be added to explain this behavior if moving forward with this implementation.
    UPDATE: Documentation and tests were added to verify that expansionCallback may be invoked twice in the event that a previously opened panel has to be closed.
  2. As far as I can tell, it seems like the remaining code in didUpdateWidget "guards" for transitions between ExpansionPanelList to ExpansionPanelList.radio and vice versa, and while I can't imagine that users will do this much, it's still a possibility and want to ensure that this logic is valid.
    UPDATE: Added some logic to ensure that didUpdateWidget initialized the open panel correctly if changed from an ExpansionPanelList to ExpansionPanelList.radio.
  3. I've also considered refactoring ExpansionPanelList.radio into a widget of its own that has its own ExpansionPanelList widget, since it feels like a lot of the logic diverges into two paths in the current implementation, but that would be a breaking change.

Related Issues

Fixes #13780

Tests

  • A regression test was added
  • Two tests to verify expansionCallback's behavior for ExpansionPanelList.radio.
  • A test to verify that transitions between ExpansionPanelList and ExpansionPanelList.radio occur properly

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.

@shihaohong shihaohong added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Apr 17, 2019
@shihaohong shihaohong requested a review from HansMuller April 17, 2019 22:38
Updated documentation with ExpansionPanelList.radio implementation.
Also reverted the call order for the radio implementation to call
the tapped panel first, and then the closed panel. Also reverted
the closed panel call to false instead of true.
@shihaohong shihaohong changed the title (WIP) Expansion panel global key exception Fix ExpansionPanelList Duplicate Global Keys Exception Apr 18, 2019
@shihaohong
Copy link
Contributor Author

shihaohong commented Apr 18, 2019

Tests were also added to verify expansionCallback behavior for ExpansionPanelList.radio, along with API documentation to clarify its current behavior.

@shihaohong shihaohong added the c: crash Stack traces logged to the console label Apr 22, 2019
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

This looks good; just had one question about semantics.

if (widget._allowOnlyOnePanelOpen) {
final ExpansionPanelRadio pressedChild = widget.children[index];

// Determine if an ExpansionPanelRadio was opened prior, invoking
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested rewording: If another ExpansionPanelRadio was already open, apply its expansionCallback (if any) to false, because it's closing.

However, it's possible that all of the expansion panels were open because previously this was an ordinary expansion panel list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand the question correctly, I think you're asking about how this logic works with the case where ExpansionPanelList --> press --> ExpansionPanelList.radio

I believe _handlePressed only opens/closes the radio panel in question, and additionally closing a previous panel if it was an ExpansionPanelList.radio. Lets say tapping a panel of ExpansionPanelList with all panels open sets state that updates the widget to behave like ExpansionPanelList.radio:

  1. First, _handlePressed will be called and trigger the callback once.
  2. Then, didUpdateWidget would update the widget and set the open radio panel, if any

Thus, expansionCallback will not be called for any of the prior open ExpansionPanelList panels. Should this be a case we should consider?

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense; in this (corner) case there's no need to call the expansionCallback on the formerly open panels, since they were part of an ordinary ExpansionPanelList and its callback wouldn't be called in this case.

Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

LGTM

@shihaohong shihaohong merged commit 3971285 into flutter:master Apr 29, 2019
@shihaohong shihaohong deleted the expansion-panel-global-key branch April 29, 2019 23:55
@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.
Labels
c: crash Stack traces logged to the console f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exception on ExpansionPanel toggle due to Duplicate Global Keys
3 participants