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

12970 Fixes hover widget jumping #12971

Merged
merged 3 commits into from
Oct 12, 2023

Conversation

Zebsterpasha
Copy link
Contributor

What it does

Fixes #12970

How to test

  1. Start debugging
  2. Stop on breakpoint
  3. Hover the pointer above any variable
  4. Move the pointer side-to-side
  5. Debug hover widget is not moving after the pointer

Follow-ups

Review checklist

Reminder for reviewers

@JonasHelming JonasHelming requested a review from planger October 10, 2023 13:30
Copy link
Contributor

@planger planger left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution! Your changes definitely improve the current state and work quite well.

However, I found a few scenarios where the positioning is strange with the Mock Debug Adapter extension.

  1. when hovering above a variable and then move the the outer right of a variable, at some point the widget jumps to horizontal position 0. I could not reproduce this with master.
Peek.2023-10-10.16-27.mp4
  1. sometimes the vertical position is misplaced, but I can reproduce this effect also without this change, so it is likely unrelated.
Peek.2023-10-10.16-29.mp4

Finally, just a style nitpick: I think it may be worth extracting the code you added into an own method, as you eventually just update this.options.selection and the method is already very long. This way you could avoid making this long method even longer and assign a meaningful name to your code.

@Zebsterpasha
Copy link
Contributor Author

@planger Okey, I'll think about moving code to the method ) I've just fixed wrong behavior, showed in the first video)

@planger
Copy link
Contributor

planger commented Oct 11, 2023

@planger Okey, I'll think about moving code to the method ) I've just fixed wrong behavior, showed in the first video)

Thank you for looking into this again! However, now your change doesn't seem to have an impact for my test with the mock debug extension anymore, as the condition on line 180 is true, so your code is never executed. Or am I missing something?
What is your test scenario?

Can you please take another look? Thank you very much!

@Zebsterpasha
Copy link
Contributor Author

Zebsterpasha commented Oct 12, 2023

@planger Yeah, this is correct. In your situation my code, probably, should not work. Mock Debug Extension has its own ExpressionProvider
https://github.com/microsoft/vscode-mock-debug/blob/b8d3d3a436e94f73ca193bf0bfa7c5c416a8aa8c/src/activateMockDebug.ts#L104
and this provider choose expression to highlight. In your first video we can see some jumps, the reason is that Mock Provider choose one expression to highlight and one Range (it could be bigger than expression), but we overrided the Range and made it less.

My main scenario - we don't have any ExpressionProvider, just pure Editor and model, but I have tested the latest version of MockDebug without any changes, DebugWidget is not jumping after pointer.
I think, ExpressionProvider should manage range, but not we.
Based on debug-hover-widget source code it was meant to be.

@planger planger self-requested a review October 12, 2023 10:59
Copy link
Contributor

@planger planger left a comment

Choose a reason for hiding this comment

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

@Zebsterpasha Ok thanks, makes sense! I don't have objections to your changes and they already improve the situation in the context of the scenario you describe. So I'll go ahead and approve this PR to avoid offloading more on to your fix.

However, I think at least mid-term we should expand this work to improve the situation for all scenarios of debug hovers. At least for me with the MockDebugExtension (with and without your changes), the debug hover behavior is not ideal.

  1. It follows the pointer if moved above an expression
  2. It is not always nicely rendered vertically (above or below the expression but sometimes covers the expression)
  3. It is sometimes placed very weirdly
Peek.2023-10-12.13-06.mp4

@Zebsterpasha If you have the chance to additionally investigate the scenarios above for all debug hover cases, as you are already familiar with this code, we'd be very grateful! If you can't do that, we can merge this change and I'll open a new ticket for the future improvements.

Thanks again for your valuable contribution!

@Zebsterpasha
Copy link
Contributor Author

@planger Okey, thank you! I agree with you, sometimes the behavior is a little bit wierd, but transferred position in document is correct, maybe this is an editor's issue) Possibly, the convertation of the Position in editor to the Position on the screen is going wrong, depending on the position of the pointer.

I think that is better to merge this request into the master and open the new one, because of different type of error.

@planger
Copy link
Contributor

planger commented Oct 12, 2023

Alright, I've opened #12994

@planger planger merged commit e0c82d1 into eclipse-theia:master Oct 12, 2023
11 of 12 checks passed
@planger planger modified the milestones: 1.44.0, 1.43.0 Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Debug hover widget jumping after pointer
2 participants