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

fix: remove tooltip on exit chart area #63

Merged
merged 1 commit into from Mar 22, 2022

Conversation

canastro
Copy link
Contributor

Just opening this as a show-case of my failed attempt to fix #62.

It fixes the case where the user moves the mouse outside the area of the chart, but if the user alt-tabs to another window and then back to the flutter window the tooltip is left open forever.

Maybe this is ok, and two other things could be added:

  1. a TTL for tooltips
  2. a way of allowing the consumer of the chart to indicate if hover is enabled, it would to the consumer to control if the window is focused or not.

Maybe n.2 is possible, I'm still pretty new with this lib.

Let me know your thoughts about this.

@entronad
Copy link
Owner

entronad commented Mar 17, 2022

Thanks a lot!

In the beginning Flutter is only for mobile, so the gesture system mainly focuses on touching cases (so called "touch first").

When using fingers, there is no problem: Wen your finger left the screen, you still want the tooltip shown; and you rarely "drag out" your finger from the chart border. If you want the tooltip hidden, just need to double tap.

But when web and desktop devices added to Flutter, the mouse interaction is getting important. It is quite different: your mouse is forever on the screen (in the status of "hover"); and people don't like to "click" or "drag" to trigger the tooltip. So "hover out" the chart becomes a frequent case.

The basic logic of selection(which controls the tooltip) of Graphic is that it triggers by on signals and hides by off signals. Which means "the on signals disappear" or "there is no signal" is not the condition to hide the tooltip.

The problem is there is no such event like "onmouseleave" in the Flutter gesture detector. It's troublesome to handle, because a principle of Graphic gesture definition is "the same as Flutter gesture detector", so creating new type may not be a good idea.

import 'polar.dart';
import 'rect.dart';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these were due to my auto saving formatter. I'll rollback if we move forward with this PR.

late bool _mouseIsConnected;

@override
void initState() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the approach of Flutter's tooltips. I check if theres a mouse, and if yes, I add a MouseRegion.

@canastro
Copy link
Contributor Author

Thank you for your quick answer @entronad!
So I tried something a bit different and more inline on how Flutter deals with tooltips.

Basically a MouseRegion is only added if a mouse is connected. And then a mouseExit "gesture" can be used to clear the tooltip.

Screen.Recording.2022-03-17.at.13.03.11.mov

Let me know what you think of this approach.

@entronad
Copy link
Owner

Your approach seems fine as I reviewed. It's quite a good work!

I will be glad to merge this PR.

@canastro canastro marked this pull request as ready for review March 21, 2022 08:43
@canastro
Copy link
Contributor Author

Thank you! Reverted the files that only changes were imports reordering and made the PR ready for review.

Hopefully we (me and @bernarndobelchior) will be able to use this in our project and we'll be contributing with more inputs to this already awesome charting library!

@entronad entronad merged commit 047be27 into entronad:main Mar 22, 2022
@entronad
Copy link
Owner

Thanks for your praise! If convenient, please introduce this lib to the dev community in your country.

@canastro
Copy link
Contributor Author

Once we get more familiar with it we’ll definitely write something about it ! In my Opinion, this lib is light years ahead of the “competition“, and it’s amazing how you did all this by yourself

@entronad
Copy link
Owner

entronad commented Apr 5, 2022

Published in v0.9.2

@entronad
Copy link
Owner

Is the _mouseIsConnected checking really necessary?

The key part of this PR is that the MouseRegion provides the onEnter and onExit, the _mouseIsConnected checking is to make sure they only emitted for mouse.

But as I tried, if not checking the mouse and always adding the MouseRegion, they are also only emitted for mouse. Thus the checking seems unnecessary, especially it takes a lot of code involving RendererBinding.

@canastro
Copy link
Contributor Author

It might be a premature optimization, but it was something I copied from Flutter's tooltip implementation here: https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/material/tooltip.dart#L354

@entronad
Copy link
Owner

@canastro
Got it. That's good.

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.

Remove hover tooltip when chart is no longer being hovered.
2 participants