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

Gesture recognizers hooks #190

Open
14 tasks
Albert221 opened this issue Oct 3, 2020 · 7 comments
Open
14 tasks

Gesture recognizers hooks #190

Albert221 opened this issue Oct 3, 2020 · 7 comments
Assignees
Labels
enhancement New feature or request

Comments

@Albert221
Copy link
Contributor

Albert221 commented Oct 3, 2020

Gesture recognizers need to be disposed. Hooks for them would make using them correctly really simple.

I'm creating this issue to track which gesture recognizers still need a hook and which don't. I'm gonna start working on the Pull Requests.

BTW I see GitHub likes to flatten my nested list. Here's a screenshot of the hierarchy:

image

@rrousselGit
Copy link
Owner

I'm honestly not sure. In which situation would you need that?

@Albert221
Copy link
Contributor Author

For example when you have a RichText containing some ordinary text and a link in it:

final tapRecognizer = useTapGestureRecognizer()
    ..onTap = () => print('Hello!');

return Text.rich(TextSpan(
  text: 'Hello there! ',
  children: [
    TextSpan(
      text: 'Tap me.',
      recognizer: tapRecognizer,
    ),
  ],
))

@rrousselGit
Copy link
Owner

What about:

final recognizer = useGestureRecognizer(
  onTap: () => print('hello'),
  onLongPress: () => print('world'),
);

I don't think that having one hook per class is a good API
Although I'm still skeptical on using hooks for this. Hooks are for state composition. Clicks are neither state nor need to be composed

And we can technically use WidgetSpan to add a GestureDetector:

Text.rich(
  TextSpan(
    text: 'Hello there! ',
    children: [
      WidgetSpan(
        child: GestureDetector(
          onTap: () => print('hello'),
          child: Text('Tap me')
        )
      ),
    ],
  ),
),

@Albert221
Copy link
Contributor Author

Hooks are for state composition.

Hmm, I just thought as it would be nice to have the family of hooks more complete with gesture recognizers, as they need proper disposing, and also hooks are more than appropriate for this as the description says:

Hooks are a new kind of object that manages a Widget life-cycles. They exist for one reason: increase the code-sharing between widgets by removing duplicates.

What do you think then? Should we (you) limit hooks provided OOTB (in this repository) only to those that manage some state or just to any that would "help manage a Widget life-cycle" with Flutter framework provided classes?

@rrousselGit
Copy link
Owner

I'm not against adding hooks to the core. But hooks aren't always the logical choice.

In this case, I don't see a use-case for such a hook other thanTextSpan. Any other use-case would use GestureDetector.
And as I mentioned previously, WdgetSpan can cover the lack of flexibility of TextSpan. It's also possible to subclass TextSpan/WidgetSpan to reduce the boilerplate, or maybe make a custom Text.rich

Similarly, to its core this family of hooks can be attributed as a useDisposable(() => MyGestureRecognizer()).
If not for the lack of a Disposable interface, that hook would probably cover all the needs.

We can also implement this useDisposable today without the interface, by "cheating" with dynamic:

T useDisposable<T>(T Function() cb, {List<Object> keys}) {
  final obj = useMemoized(cb, keys);

  assert((obj as dynamic).dispose is void Function(), '$T does not have a `void dispose()` method');

  useEffect(() {
    dynamic disposable = obj;
    return disposable.dispose;
  }, keys);
}

If not for the fact that this hook isn't "sound" due to the lack of a Disposable interface, it'd be in the core.

@rivasdiaz
Copy link

Although I agree with @rrousselGit comments, I found that a WidgetSpan with a GestureDetector and Text as child is not a 100% substitution for a TextSpan with a GestureRecognizer, as the former is rendered as a block inside the rest of the span, while using a TextSpan wraps as needed.

I implemented a simple hook expecting a function that builds the gesture recognizer and just handles the recognizer disposal. I'm sharing to anyone looking for a solution. This is my first look at implementing a hook. It works for my case, hopefully it may work for someone else.

This is how to use it:

// define your gestureRecognizer
final gestureRecognizer = useGestureRecognizer(
  () => TapGestureRecognizer()
    ..onTap = () {
      // do something when a tap is recognized
      ScaffoldMessenger.of(context).showSnackBar(
        const SnackBar(content: Text('Do something on tap')),
      );
    },
);
// use the created gestureRecognizer on a TextSpan ...

and this is the code for the hook implementation:

typedef GestureRecognizerBuilder<T extends GestureRecognizer> = T Function();

T useGestureRecognizer<T extends GestureRecognizer>(
    GestureRecognizerBuilder<T> builder,
    [List<Object?>? keys]) {
  return use(
    _GestureRecognizerHook(
      builder: builder,
      keys: keys,
    ),
  );
}

class _GestureRecognizerHook<T extends GestureRecognizer> extends Hook<T> {
  const _GestureRecognizerHook({
    required this.builder,
    List<Object?>? keys,
  }) : super(keys: keys);

  final GestureRecognizerBuilder<T> builder;

  @override
  HookState<T, Hook<T>> createState() => _GestureRecognizerHookState();
}

class _GestureRecognizerHookState<T extends GestureRecognizer>
    extends HookState<T, _GestureRecognizerHook<T>> {
  late final T recognizer = hook.builder();

  @override
  T build(BuildContext context) => recognizer;

  @override
  void dispose() => recognizer.dispose();

  @override
  String get debugLabel => 'useGestureRecognizer';
}

I'm not 100% sure if this code works exactly the same as the useDisposable example from @rrousselGit , but I checked both cases and it didn't seem to be disposing at the same time.

@rrousselGit rrousselGit self-assigned this May 10, 2023
@SpeedReach
Copy link

Any updates on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants