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: Add HoverCallbacks #2706

Merged
merged 20 commits into from Sep 10, 2023
Merged

feat: Add HoverCallbacks #2706

merged 20 commits into from Sep 10, 2023

Conversation

luanpotter
Copy link
Member

Description

Currently Flame is in a state of disarray. Hoverables (and HasHoverables) are deprecated, claiming that they should be replaced with "HoverCallbacks". However, try as you might, "HoverCallbacks" does not seem to exist.

In fact, upon further analysis, there does not seem to be any way whatsoever to replace the currently deprecated classes. I do not think it is acceptable to have a deprecated method without a replacement, so this provides one.

This creates HoverCallbacks (and PointerMoveCallbacks) to replicate the Hoverables behaviour in the new camera and event system.

The internal wiring becomes a bit messy because wiring it via a GestureRecognizer does not seem to work. The gesture recognizer seems to only receive pointer events when dragging the mouse.

That can potentially be improved on followups, as long as the functionality is maintained.

Checklist

  • I have followed the Contributor Guide when preparing my PR.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples or docs.

Breaking Change?

  • Yes, this PR is a breaking change.
  • No, this PR is not a breaking change.

}

@override
String toString() => 'PointerMoveEvent(devicePosition: $devicePosition, '
Copy link
Member Author

Choose a reason for hiding this comment

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

I will try to followup with some simplifications around duplicated code between "events"


/// Set by the PointerMoveDispatcher to receive mouse events from the
/// game widget.
void Function(PointerHoverEvent event)? mouseDetector;
Copy link
Member Author

Choose a reason for hiding this comment

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

sadly this does not seem to be possible to be included under the gestureDetectors - events don't fire as intended.

@spydon
Copy link
Member

spydon commented Sep 4, 2023

There are two other people that have worked on the Hover callbacks so far, one in #1834 and the other is a thread on Discord, would be good if you could check with them what they have done so far.

@luanpotter
Copy link
Member Author

@spydon this idea is interesting:

#1733

even though it is my understanding that we are removing tapables, etc.

but the way to handle the wiring of the events between game widget and game could be through a handlers: List<Handle> list bound to the game.

that would serve a similar purpose to our gestures map but more general bc would use our custom handler interface. so could work for mouse movements as well.

does this make sense? if so I can try to put a draft together to see what it would look like

I wonder if that should be a followup though - I think we are sorely lacking a replacement for HasHoverables within the new system so wanted to find the quickest way to provide that on the current structure

@spydon
Copy link
Member

spydon commented Sep 4, 2023

@spydon this idea is interesting:

#1733

even though it is my understanding that we are removing tapables, etc.

but the way to handle the wiring of the events between game widget and game could be through a handlers: List<Handle> list bound to the game.

that would serve a similar purpose to our gestures map but more general bc would use our custom handler interface. so could work for mouse movements as well.

does this make sense? if so I can try to put a draft together to see what it would look like

I wonder if that should be a followup though - I think we are sorely lacking a replacement for HasHoverables within the new system so wanted to find the quickest way to provide that on the current structure

Up to you, I don't have time to properly review anything until Wednesday anyways. The hoverables are the least used detector, so I don't think it is that much of a rush.

Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

I think it looks good! I would have expected it to be more messy.
Anything more than docs missing?

examples/lib/stories/input/hoverables_example.dart Outdated Show resolved Hide resolved
@luanpotter luanpotter changed the title feat: Add HoverCallbacks [draft] feat: Add HoverCallbacks Sep 10, 2023
@luanpotter luanpotter marked this pull request as ready for review September 10, 2023 15:17
Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Lgtm! Just docs missing

@luanpotter
Copy link
Member Author

@spydon added docs, please take a look! :)

doc/flame/inputs/pointer_events.md Outdated Show resolved Hide resolved
doc/flame/inputs/pointer_events.md Outdated Show resolved Hide resolved
doc/flame/inputs/pointer_events.md Outdated Show resolved Hide resolved
doc/flame/inputs/pointer_events.md Outdated Show resolved Hide resolved
Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Lgtm!!

@luanpotter luanpotter merged commit d460b84 into main Sep 10, 2023
7 checks passed
@luanpotter luanpotter deleted the luan.hover branch September 10, 2023 18:49
@spydon spydon mentioned this pull request Sep 11, 2023
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.

None yet

2 participants