Skip to content

Add missing TextField attributes - #87

Closed
saif-ellafi wants to merge 1 commit into
solid-software:mainfrom
saif-ellafi:add-missing-textfield-params
Closed

Add missing TextField attributes#87
saif-ellafi wants to merge 1 commit into
solid-software:mainfrom
saif-ellafi:add-missing-textfield-params

Conversation

@saif-ellafi

Copy link
Copy Markdown

Adds missing attributes of TextField to allow developer to make use of them

@illia-romanenko

Copy link
Copy Markdown
Contributor

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request is a good initiative to enhance LanguageToolTextField by adding more standard TextField attributes, increasing its flexibility for developers. The added properties are relevant and cover common use cases.

However, there are a couple of critical issues related to the implementation of the scrollController and groupId properties that need to be addressed. There's also a point regarding the default value for textCapitalization worth considering for consistency.

Summary of Findings

  • Incorrect ScrollController Handling: The widget.scrollController property is added but not utilized. The LanguageToolTextField always uses an internally created ScrollController, ignoring any provided by the developer. This requires changes to the state's lifecycle management for the scroll controller.
  • Incorrect groupId Default: The groupId property in the TextField is incorrectly defaulted to the type EditableText instead of null or widget.groupId.
  • Divergent textCapitalization Default: LanguageToolTextField defaults textCapitalization to .sentences, while the underlying TextField defaults to .none. This difference should be intentional and clear, or aligned.
  • Missing Listener Removal: The _textControllerListener added in initState is not removed in dispose, which can lead to memory leaks. This was noted as part of the scrollController feedback.

Merge Readiness

The pull request aims to add valuable features by exposing more TextField attributes. However, there are critical issues, particularly with how the scrollController is handled and the groupId default, that must be addressed before this PR can be merged. The textCapitalization default is a medium-severity point to consider for consistency.

I recommend addressing these points to ensure the new attributes function correctly and as expected. I am unable to approve the pull request in its current state; please make the necessary changes, and then it can be re-reviewed for merging.

class _LanguageToolTextFieldState extends State<LanguageToolTextField> {
FocusNode? _focusNode;
final _scrollController = ScrollController();
final ScrollController _scrollController = ScrollController();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

The current initialization of _scrollController here (and its subsequent exclusive use) doesn't correctly incorporate the new widget.scrollController property. This means LanguageToolTextField will always create and use its own internal ScrollController, effectively ignoring any ScrollController instance passed by the developer via the widget.scrollController property.

To correctly implement this, the state needs to manage the ScrollController lifecycle based on whether widget.scrollController is provided:

  1. The state's scroll controller field (e.g., _internalScrollController as suggested below) should be nullable. It should only be initialized with a new ScrollController() in initState if widget.scrollController is null.
  2. An 'effective' ScrollController (which is widget.scrollController if provided, otherwise the internally created one) should be determined and used throughout the state's lifecycle.
  3. This effective ScrollController must be passed to the TextField(scrollController: ...) in the build method.
  4. The _textControllerListener must read its offset from this effective controller (e.g., widget.controller.scrollOffset = _effectiveScrollController.offset). It's also good practice to check _effectiveScrollController.hasClients before accessing .offset.
  5. In the dispose method, the internally created scroll controller (if any) should be disposed.
  6. Additionally, a crucial point related to lifecycle management: the listener _textControllerListener added to widget.controller in initState is not currently removed in dispose. This can lead to memory leaks or errors. Please add widget.controller.removeListener(_textControllerListener); in the dispose method.
  ScrollController? _internalScrollController; // This will be initialized in initState if widget.scrollController is null

onSubmitted: widget.onTextSubmitted,
onTap: widget.onTap,
onTapOutside: widget.onTapOutside,
groupId: widget.groupId ?? EditableText,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

The use of EditableText as a fallback for widget.groupId (i.e., widget.groupId ?? EditableText) is incorrect. EditableText is a class (a type), not an Object instance that can be used as a groupId. The groupId property of TextField expects an Object?.

If widget.groupId is null, you should pass null to the TextField, which aligns with TextField's own default behavior for groupId. Could you please adjust this to pass widget.groupId directly?

          groupId: widget.groupId,

groupId: widget.groupId ?? EditableText,
maxLength: widget.maxLength,
enabled: widget.enabled,
textCapitalization: widget.textCapitalization ?? TextCapitalization.sentences,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The textCapitalization property is defaulted to TextCapitalization.sentences if widget.textCapitalization is null. While TextCapitalization.sentences can be a sensible default for many text fields, the underlying Flutter TextField widget defaults its textCapitalization property to TextCapitalization.none.

Is this deviation intentional to provide a different default for LanguageToolTextField? If the goal is to closely mirror TextField's behavior, you might consider passing widget.textCapitalization directly, allowing TextField to apply its own default of .none when widget.textCapitalization is null. If .sentences is a deliberate choice for this wrapper, it might be helpful to document this specific default behavior for users of LanguageToolTextField.

          textCapitalization: widget.textCapitalization,

@illia-romanenko

Copy link
Copy Markdown
Contributor

fixed in #90

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants