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

[READY] Handle scrolling when hover popup is open #4209

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

bstaletic
Copy link
Collaborator

@bstaletic bstaletic commented Jan 2, 2024

PR Prelude

Thank you for working on YCM! :)

Please complete these steps and check these boxes (by putting an x inside
the brackets) before filing your PR:

  • I have read and understood YCM's CONTRIBUTING document.
  • I have read and understood YCM's CODE_OF_CONDUCT document.
  • I have included tests for the changes in my PR. If not, I have included a
    rationale for why I haven't.
  • I understand my PR may be closed if it becomes obvious I didn't
    actually perform all of these steps.

Why this change is necessary and useful

There are two kinds of scrolling that need to be handled:

  1. Scrolling the buffer in a window shifts the buffer, but popups stay in place. Instead, we close the popup whenever we receive a WinScrolled event.
  2. Scrolling the popup contents themselves resets the updatetime timer and can trigger a second CursorHold event, which leads to YCM resetting the hover popup. Instead, only re-display the hover popup if it is not already visible.

Open questions:

  • Should we close other popups after WinScrolled?
  • Tests... How do we even test "do X if mouse pointer at (x, y) position"? The behaviour differs when mouse pointer is inside or outside the popup area.

This change is Reviewable

Copy link

codecov bot commented Jan 2, 2024

Codecov Report

Merging #4209 (cd48033) into master (05bbb07) will decrease coverage by 0.02%.
The diff coverage is 88.88%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4209      +/-   ##
==========================================
- Coverage   89.59%   89.58%   -0.02%     
==========================================
  Files          34       34              
  Lines        4440     4445       +5     
==========================================
+ Hits         3978     3982       +4     
- Misses        462      463       +1     

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Tests... How do we even test "do X if mouse pointer at (x, y) position"? The behaviour differs when mouse pointer is inside or outside the popup area.

there's a special vim function test_setmouse()

iirc you can also do feedkeys("\<MouseMove>") or something similar

Reviewable status: 0 of 2 LGTMs obtained

@bstaletic bstaletic force-pushed the popup_scrolling branch 7 times, most recently from 38c1148 to 5f5fc9c Compare February 5, 2024 16:04
@bstaletic bstaletic changed the title [RFC] Handle scrolling when hover popup is open [READY] Handle scrolling when hover popup is open Feb 14, 2024
@bstaletic
Copy link
Collaborator Author

The test was working fine on my machine, but was inexplicably failing on CI. That's why I scrapped the test.

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)

There are two kinds of scrolling that need to be handled:
1. Scrolling the buffer in a window shifts the buffer, but popups stay
   in place. Instead, we close the popup whenever we receive a
   WinScrolled event.
2. Scrolling the popup contents themselves resets the `updatetime` timer
   and can trigger a second CursorHold event, which leads to YCM
   resetting the hover popup. Instead, only re-display the hover popup
   if it is not already visible.
@bstaletic bstaletic added the Ship It! Manual override to merge a PR by maintainer label Feb 14, 2024
Copy link
Contributor

mergify bot commented Feb 14, 2024

Thanks for sending a PR!

@mergify mergify bot merged commit f078924 into ycm-core:master Feb 14, 2024
10 of 13 checks passed
@bstaletic bstaletic deleted the popup_scrolling branch February 14, 2024 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ship It! Manual override to merge a PR by maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants