_SearchBarState should dispose FocusNode, if it created it.

[polina-c]

No description provided.

[chunhtai]

LGTM

[mateusfccp]

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]

@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]

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

[mateusfccp]

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