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 preview hiding #10889

Merged
merged 4 commits into from
Jul 16, 2020
Merged

Fix preview hiding #10889

merged 4 commits into from
Jul 16, 2020

Conversation

mmisol
Copy link
Collaborator

@mmisol mmisol commented Jul 14, 2020

Purpose

Fixes a bug which would make the preview hide even when the mouse was
moved over a visible part of it. This happened only when the preview
area was wider than the node.

The cause of the bug I would argue is some very strange behavior of
HitTest. When run on the NodeView, this test would not consider the
right part of the preview, like if the box was as wide as the node but
displaced to the left.

Likewise, running a HitTest on the PreviewControl itself provided
inconsistent results. Initially this would work as expected, but after
the preview was expanded and recondensed, the box would remain forever
bloated as if still expanded. This resulted in inconsistencies with the
leave event, which used the real margins of the control.

The problem was fixed by checking explicitly that the preview visible
area is abandoned before hiding the preview.

ezgif com-video-to-gif(2)

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.

Reviewers

@QilongTang @mjkkirschner

FYIs

@DynamoDS/dynamo

/// </summary>
/// <param name="e">A mouse event</param>
/// <returns>Whether the mouse is over the preview or not</returns>
private bool IsMouseInsidePreview(MouseEventArgs e)
Copy link
Contributor

@aparajit-pratap aparajit-pratap Jul 14, 2020

Choose a reason for hiding this comment

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

How is this method different in functionality from the method with the similar name: IsMouseInsideNodeOrPreview? Does the latter method address the second point above: HitTest on NodeView => Anomalous region that skips right part of preview if larger than node, and is therefore an insufficient test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that's right @aparajit-pratap . I know it's kinda strange for a method summary, but why it was needed was hard to explain otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

do we know why the anomalous region exists? 😉 💀

@aparajit-pratap
Copy link
Contributor

Nice fix to have. This was quite frustrating.

Copy link
Member

@mjkkirschner mjkkirschner left a comment

Choose a reason for hiding this comment

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

It looks good, are you still thinking of creating a test that raises leave events manually?

@mmisol
Copy link
Collaborator Author

mmisol commented Jul 15, 2020

@mjkkirschner I haven't found a way to do that which would allow me to specify a position. Will search a little more tomorrow morning.

@mmisol
Copy link
Collaborator Author

mmisol commented Jul 16, 2020

Merging as we are doing a UI automation test for this

@mmisol mmisol merged commit e15a260 into DynamoDS:master Jul 16, 2020
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