Skip to content
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.

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

feat: added autocomplete options to url input #128

Merged
merged 12 commits into from
Nov 2, 2024

Conversation

RA341
Copy link
Contributor

@RA341 RA341 commented Sep 8, 2024

Implemented #127, had to make a custom autocomplete because the default one does not accept a controller and defaults to maxwidth.

If no addresses are found:

image

If previous address are present:

image

@@ -266,3 +259,68 @@ class LoginScreen extends HookConsumerWidget {
.trim();
}
}

class UrlFieldInput extends ConsumerWidget {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please extract the widget to another file.

@sevenrats
Copy link
Collaborator

Im abroad and can’t test this, but I’m curious if tab-to-complete works with this. Does anybody know?

@RA341
Copy link
Contributor Author

RA341 commented Sep 12, 2024

I tried unsuccessfully to add the shortcut; my best guess is that the textfield is capturing the tab event preventing the shortcut to execute.

@RA341
Copy link
Contributor Author

RA341 commented Sep 12, 2024

i am not sure how to disable that behaviour

@sevenrats
Copy link
Collaborator

Can we extend TextField?

@RA341
Copy link
Contributor Author

RA341 commented Sep 12, 2024

Probably, but I don't think textfield itself is the issue its something related to focus and focus node

@RA341
Copy link
Contributor Author

RA341 commented Sep 12, 2024

tab.mp4

The flickering is me pressing tab

@sevenrats
Copy link
Collaborator

flutter/flutter#75032

@sevenrats
Copy link
Collaborator

Just glancing but maybe DefaultTextEditingAction

@RA341
Copy link
Contributor Author

RA341 commented Sep 12, 2024

I tried it still the same behavior,

the autocomplete is made up of 2 parts a textfield and a listview that displays the options, now I am not sure if focus is passed to the listview or remains with textfield.

which I believe is the issue, If the focus remains with textfield (which I suspect it is) when we press tab the textfield attempts to go to the next field which resets autocomplete causing the behavior in the video.

but I could be wrong and its not any of the above

@RA341
Copy link
Contributor Author

RA341 commented Sep 12, 2024

yeah i changed the shortcut to ctrl_space it still wont work I am pretty sure it's a focus issue

@sevenrats
Copy link
Collaborator

hmm. this problem is actually super sticky because the intuitive behavior of the tab key is extremely complicated.
implementing tab completion is possible, but its not clear that its desireable because it breaks tabbing between form fields, which is important. maybe we use enter instead? somebody needs to have a good idea before we can get around that problem, but, here is how to grab the keypress:

import 'package:flutter/material.dart';
import 'package:flutter/services.dart';
import 'package:hooks_riverpod/hooks_riverpod.dart';
import 'package:jellyflix/components/custom_autocomplete_options.dart';
import 'package:jellyflix/models/user.dart';
import 'package:jellyflix/providers/auth_provider.dart';
import 'package:flutter_gen/gen_l10n/app_localizations.dart';

class UrlFieldInput extends ConsumerWidget {
  const UrlFieldInput({super.key, required this.serverAddress});

  final TextEditingController serverAddress;

  @override
  Widget build(BuildContext context, WidgetRef ref) {
    final savedAddress = ref.watch(allProfilesProvider);

    // Needed to get width of the URL text field
    // so we can assign that to the autocomplete width
    final urlTextFieldKey = GlobalKey();

    // Create a custom FocusNode to listen for key events
    final focusNode = FocusNode();

    // Attach the key listener for Tab key press
    focusNode.onKeyEvent = (FocusNode node, KeyEvent event) {
      if (event is KeyDownEvent &&
          event.logicalKey == LogicalKeyboardKey.enter) {
        _selectAutocompleteOption(focusNode, serverAddress, savedAddress);
        return KeyEventResult.handled;
      }
      return KeyEventResult.ignored;
    };

    return RawAutocomplete<String>(
      focusNode: focusNode,
      textEditingController: serverAddress,
      optionsBuilder: (TextEditingValue textEditingValue) {
        final result = savedAddress.valueOrNull
            ?.where((element) => element.serverAdress!
                .toLowerCase()
                .contains(textEditingValue.text.toLowerCase()))
            .map((e) => e.serverAdress!);
        return result == null || result.isEmpty
            ? ['http://', 'https://']
                .where((e) => e.contains(textEditingValue.text.toLowerCase()))
            : result;
      },
      onSelected: (option) => serverAddress.text = option,
      optionsViewOpenDirection: OptionsViewOpenDirection.down,
      fieldViewBuilder: (context, controller, focusNode, _) {
        return TextField(
          key: urlTextFieldKey,
          focusNode: focusNode,
          controller: controller,
          decoration: InputDecoration(
            border: const OutlineInputBorder(),
            labelText: AppLocalizations.of(context)!.serverAddress,
            hintText: 'http://',
          ),
        );
      },
      optionsViewBuilder: (
        BuildContext context,
        void Function(String) onSelected,
        Iterable<String> options,
      ) {
        RenderBox? renderBox;
        if (urlTextFieldKey.currentContext?.findRenderObject() != null) {
          renderBox =
              urlTextFieldKey.currentContext!.findRenderObject() as RenderBox;
        }

        return CustomAutocompleteOptions(
          displayStringForOption: RawAutocomplete.defaultStringForOption,
          onSelected: onSelected,
          options: options,
          openDirection: OptionsViewOpenDirection.down,
          maxOptionsHeight: 100,
          maxOptionsWidth: renderBox?.size.width ?? 300,
        );
      },
    );
  }

  void _selectAutocompleteOption(FocusNode focusNode,
      TextEditingController controller, AsyncValue<List<User>> savedAddress) {
    // Get the current text in the controller
    final currentText = controller.text;

    // Find the first matching option
    final matchedOption = savedAddress.valueOrNull?.firstWhere(
        (element) => element.serverAdress!.startsWith(currentText),
        orElse: () => User());

    // If there's a match, set it in the controller
    if (matchedOption != null) {
      controller.text = matchedOption.serverAdress!;
      // Move the cursor to the end of the text
      controller.selection = TextSelection.fromPosition(
        TextPosition(offset: controller.text.length),
      );
    }
  }
}

Note that this does not work because of the broken null check operators and whatnot but I think this should be enough to get past the blockage.

@RA341
Copy link
Contributor Author

RA341 commented Sep 22, 2024

I changed how options are autocompleted, inspired by @sevenrats' solution. The gist is that I used Riverpod statenotifiers to sync the state with autocomplete and the textbox. Let me know if there is some weird behavior.

Please build it locally and test it out.

@RA341
Copy link
Contributor Author

RA341 commented Sep 22, 2024

also added this neat little hint enter to fill
This is automatically hidden on mobile

image

@sevenrats
Copy link
Collaborator

the shortcut is firing for me but new servers don't get stored and the shortcut doesn't actually cause selection to happen, though clicking does.

@RA341
Copy link
Contributor Author

RA341 commented Sep 22, 2024

wdym its working for me

Recording.2024-09-22.130452.mp4

@sevenrats
Copy link
Collaborator

Are you sure you pushed all changes? What is you dev platform?

@RA341
Copy link
Contributor Author

RA341 commented Sep 22, 2024

Windows, everything should be pushed; whats the issue you are running into,

you have to use arrow keys to select the urls

@sevenrats
Copy link
Collaborator

one, my list only ever contains "http" and "https" it doesnt contain any servers. so maybe thats the issue.

@sevenrats
Copy link
Collaborator

ok i solved the first issue. "log out" destroys the session obviously so thats just me being dumb. enter still doesnt work though even after I add several profiles. plus, the http and https strings need to go unless we can make those work as expected.

@sevenrats
Copy link
Collaborator

ah, and now I am getting your catch string. flutter: Option list accessed a element out of bounds
flutter: RangeError (index): Invalid value: Valid value range is empty: 0

@RA341
Copy link
Contributor Author

RA341 commented Sep 22, 2024

no i made a mistake, i didnt take care of HTTP, https I match only with previous addresses, it will not fill if its http or https only

fixing it

@RA341
Copy link
Contributor Author

RA341 commented Sep 22, 2024

i added some new debug prints what is the optionslist and selectedindex

@RA341
Copy link
Contributor Author

RA341 commented Sep 22, 2024

there is a reason for the empty options, though; I think there is some sort of race condition, or the state is not being updated, so the options provider is not filled even after the list is built.

But I cant seem to recreate it, it does it randomly

@sevenrats
Copy link
Collaborator

sorry this is probably my fault. i think i was overcasting the pull by having a local change in the tree. stand by.

@sevenrats
Copy link
Collaborator

why is video state pulled in?

@RA341
Copy link
Contributor Author

RA341 commented Sep 22, 2024

wdym

@sevenrats
Copy link
Collaborator

image

@sevenrats
Copy link
Collaborator

flutter: Option list accessed a element out of bounds
flutter: The following are the available options
flutter: []
flutter: Index that was accessed
flutter: 0
flutter: RangeError (index): Invalid value: Valid value range is empty: 0

something is definitely bugged out with the state.

@RA341
Copy link
Contributor Author

RA341 commented Sep 22, 2024

image

My IDE must have pulled it in, removed it

@RA341
Copy link
Contributor Author

RA341 commented Sep 22, 2024

@jdk-21 when you have the time could you verify you are running into the same issue as @sevenrats with the out of bounds exception

should be fixed

@sevenrats
Copy link
Collaborator

k the race condition is in the use of "read" instead of "watch" I think. this fixes it almost, but there is one last little focus bug where after "enter" the focus node doesn't let go and I can't tab out. we are very close tho.
image

@RA341
Copy link
Contributor Author

RA341 commented Sep 22, 2024

got it ill switch to watch,

the tabbing thing will not work with autocomplete; you have to press ESC first if the autocomplete options are being shown then you can tab out

@sevenrats
Copy link
Collaborator

ah. lets make the select function close the autocomplete then, and reopen it on subsequent keypresses, yeah?

@RA341
Copy link
Contributor Author

RA341 commented Sep 22, 2024

I made it so that options are automatically hidden if we have the exact value as the options,

check it out, works pretty nice now

@sevenrats
Copy link
Collaborator

yeah that works great. awesome! I will do a syntactical review after work.
good job this is very cool!

@sevenrats
Copy link
Collaborator

ok I hate to say it but I think this needs a little more time in the oven. I've done some experiments that make it clear that we can actually have everything we want (tab and enter to select with properly preserved focus hierarchy) we just need to restructure behind a FocusGroup of some kind. If you want to keep running this ball I can give you info on the experiements I've done or I will just try to work up a pr into your branch some time this week.

@RA341
Copy link
Contributor Author

RA341 commented Sep 23, 2024

What else needs to be added ?

@RA341
Copy link
Contributor Author

RA341 commented Sep 23, 2024

removed some redundant code, and added comments, shouldn't break anything

@jdk-21
Copy link
Collaborator

jdk-21 commented Sep 24, 2024

Are the changes compatible with d-Pad navigation for AndroidTV?

@RA341
Copy link
Contributor Author

RA341 commented Sep 24, 2024

I have no way to test that, but I didn't change any default nav behaviour just how the options are filled (removed a unnecessary provider)

@RA341
Copy link
Contributor Author

RA341 commented Sep 24, 2024

If flutter supports dpad similar to arrow keys it should work

@jdk-21
Copy link
Collaborator

jdk-21 commented Sep 25, 2024

The Android emulator can also emulate TVs. Yes, dpad navigation are just arrow keys.

@RA341
Copy link
Contributor Author

RA341 commented Sep 25, 2024

It'll work then, also the form is already wrapped in a focus group, because of password auto fill stuff.

@jdk-21 jdk-21 merged commit 18184d2 into jellyflix-app:main Nov 2, 2024
1 check failed
@sevenrats
Copy link
Collaborator

@jdk-21 I strongly recommend that we revert this. It's not feature complete and contains unnecessary technical debt.

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.

3 participants