Skip to content

Add missing TextField attributes #87

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

Closed
saif-ellafi wants to merge 1 commit into from
Closed

Add missing TextField attributes #87

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion lib/src/presentation/language_tool_text_field.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ class LanguageToolTextField extends StatefulWidget {
final ValueChanged<String>? onTextSubmitted;
final VoidCallback? onTap;
final TapRegionCallback? onTapOutside;
final TextCapitalization? textCapitalization;
final int? maxLength;
final Object? groupId;
final TextInputAction? textInputAction;
final TextInputType? keyboardType;
final Color? cursorColor;
Expand All @@ -53,6 +56,9 @@ class LanguageToolTextField extends StatefulWidget {
final bool readOnly;
final MouseCursor? mouseCursor;
final bool alignCenter;
final ScrollController? scrollController;
final bool? enabled;
final Widget? Function(BuildContext, { required int currentLength, required bool isFocused, required int? maxLength})? buildCounter;

/// Creates a widget that checks grammar errors.
const LanguageToolTextField({
Expand All @@ -63,6 +69,7 @@ class LanguageToolTextField extends StatefulWidget {
this.mistakePopup,
this.maxLines = 1,
this.minLines,
this.maxLength,
this.expands = false,
this.textAlign = TextAlign.start,
this.textDirection,
Expand All @@ -71,6 +78,11 @@ class LanguageToolTextField extends StatefulWidget {
this.autoFocus = false,
this.readOnly = false,
this.textInputAction,
this.groupId,
this.enabled,
this.buildCounter,
this.scrollController,
this.textCapitalization,
this.keyboardType,
this.focusNode,
this.keyboardAppearance,
Expand All @@ -89,7 +101,7 @@ class LanguageToolTextField extends StatefulWidget {

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

@gemini-code-assist gemini-code-assist Bot May 27, 2025

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


@override
void initState() {
Expand Down Expand Up @@ -148,6 +160,11 @@ class _LanguageToolTextFieldState extends State<LanguageToolTextField> {
onSubmitted: widget.onTextSubmitted,
onTap: widget.onTap,
onTapOutside: widget.onTapOutside,
groupId: widget.groupId ?? EditableText,

@gemini-code-assist gemini-code-assist Bot May 27, 2025

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,

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

@gemini-code-assist gemini-code-assist Bot May 27, 2025

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,

buildCounter: widget.buildCounter,
);

if (widget.alignCenter) {
Expand Down