Skip to content

Conversation

@polina-c
Copy link
Contributor

@polina-c polina-c commented Sep 3, 2023

No description provided.

@polina-c polina-c requested a review from goderbauer September 3, 2023 18:53
@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Sep 3, 2023
@polina-c polina-c requested a review from chunhtai September 3, 2023 18:53
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM

@polina-c polina-c merged commit c9f70e9 into flutter:master Sep 5, 2023
@polina-c polina-c deleted the _SearchBarState branch September 5, 2023 20:12
polina-c added a commit that referenced this pull request Sep 5, 2023
auto-submit bot pushed a commit that referenced this pull request Sep 5, 2023
@polina-c polina-c restored the _SearchBarState branch September 5, 2023 21:26
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
@mateusfccp
Copy link
Contributor

mateusfccp commented Oct 20, 2023

I think this may not be well solved.

We don't have didUpdateWidget implemented for the state. This means that we have some potential problems:

  1. If the widget starts with a null FocusNode and then is updated to have a non-null FocusNode, when it is disposed the dispose method will check if widget.focusNode == null, which will return false and _focusNode will not be disposed
  2. If the widget starts with a non-null FocusNode and then is updated to have a null FocusNode, when it is disposed the dispose method will check if widget.focusNode == null, which will return true and _focusNode will be disposed. However, as _focusNode was passed by an external widget, it's possible that the external widget uses now a disposed node. Even if it does not uses it, it's possible that it will try to dispose it in its own dispose method, which will cause an exception.
  3. Updating the widget focusNode simply does not update it, which is probably not intended.

I think a more appropriate implementation would be something like this:

  @override
  void initState() {
    super.initState();
    // ...
    _focusNode = widget.focusNode ?? FocusNode();
  }

  @override
  void didUpdateWidget(TradingChart oldWidget) {
    super.didUpdateWidget(oldWidget);

    if (widget.focusNode != oldWidget.focusNode) {
      if (oldWidget.focusNode == null) _focusNode.dispose();
      _focusNode = widget.focusNode ?? FocusNode();
    }
  }

  @override
  void dispose() {
    // ...
    if (widget.focusNode == null) {
      _focusNode.dispose();
    }
    super.dispose();
  }

WDYT?

@chunhtai
Copy link
Contributor

@mateusfccp
You are right, this doesn't take into account that if the widget.focusNode changes. Since you already have the fix, can you file a pr with a test? you can pin me for review

@mateusfccp
Copy link
Contributor

@chunhtai Sure, as soon as I have some time!

@mateusfccp
Copy link
Contributor

@chunhtai It seems that this issue has already been fixed in a later PR.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

3 participants