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

Use balloon hovers instead of cursorhold hovers to show documents #4106

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Shuangcheng-Ni
Copy link

@Shuangcheng-Ni Shuangcheng-Ni commented Dec 25, 2022

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

The advantages of balloon hovers

  • higher efficiency: Moving the mouse is much faster than moving the vim cursor.
  • more convenience: Suppose a user has entered insert mode. Suddenly, he/she wants to look up the document of a function elsewhere. If we use balloon hovers, the user just needs to move the mouse pointer onto that function, read the document and continue typing then. If we use cursorhold hovers, the user has to quit insert mode, move the vim cursor towards the function, read the document, and then move the vim cursor back and enter insert mode again.
  • less annoyance: I believe many ycm users have experienced the annoyance brought by cursorhold hovers. When updatetime is too short, cursorhold hovers are quite likely to pop up frequently when a user is moving the vim cursor, which overshadows file contents all the time. When updatetime is too long, a user has to wait for a long time before the desired cursorhold popup emerges. When the user disables auto hover, he/she has to manually trigger cursorhold hovers over and over again. All these cases are quite annoying. If we use balloon hovers, no popups will come to make trouble when we move the vim cursor as long as the mouse isn't moved. We can also set balloondelay to a moderate time, so we won't have to wait for too long.

My design: balloon hovers for auto hover and cursorhold hovers for manual hover

Considering these advantages, balloon hovers can be used for auto hover. Some users might prefer to use the mouse to point at the text while reading a long document. Balloon hovers are volatile in this case, so I still preserve cursorhold hovers which can be triggered manually.

Bad news

  • Quite a lot of changes!
  • Quite hard to test!

The changes are listed as follows

line 1726: youcompleteme#hover()

The youcompleteme#hover() function generates the content of a balloon hover. I use win_execute() here to ensure the s:Hover() function to be executed in the buffer/window where the mouse pointer is, since the mouse pointer and the vim cursor can be in different buffers/windows.

function! youcompleteme#hover()
  call win_execute( v:beval_winid, 'call s:Hover( "balloon" )' )
  return ''
endfunction

line 711: s:EnableAutoHover() & s:DisableAutoHover()

I use set (no)ballooneval/(no)balloonevalterm to enable/disable auto hover. In function s:EnableAutoHover(), the 'balloonexpr' option won't be overridden if &balloonexpr is not an empty string. Therefore, the 'balloonexpr' option set by vimspector will be preserved while debugging.

function! s:EnableAutoHover()
  if s:enable_hover
    if has( 'balloon_eval' )
      set ballooneval
    endif
    if has( 'balloon_eval_term' )
      set balloonevalterm
    endif
    if exists( '+balloonexpr' ) && empty( &balloonexpr )
      set balloonexpr=youcompleteme#hover()
    endif
  endif
endfunction


function! s:DisableAutoHover()
  call popup_clear()
  if has( 'balloon_eval' )
    set noballooneval
  endif
  if has( 'balloon_eval_term' )
    set noballoonevalterm
  endif
endfunction

line 1599: exists( '*popup_beval' )

I use exists( '*popup_beval' ) to judge whether the balloon popup feature is available.

if exists( '*popup_atcursor' ) && exists( '*popup_beval' )
  " some code that calls popup_atcursor() and popup_beval()
endif

line 1600: s:Hover()

The s:Hover() function is modified to take an argument named hover_type, which should be either 'cursorhold' or 'balloon'.

  • a:hover_type == 'balloon'
    In this case, the s:Hover() function should be called by call win_execute( v:beval_winid, 'call s:Hover()' ), so theb:ycm_hover variable is related to the buffer/window where the mouse pointer is and b:ycm_hover.command is executed at the mouse pointer. To get the location of the mouse pointer, I add some arguments to b:ycm_hover.command, including --bufnr=v:beval_bufnr, --line_num=v:beval_lnum and --column_num=v:beval_col. --line_num and --column_num are not supported by ycm currently. They depend on my previous PR.
  • a:hover_type == 'cursorhold'
    No changes.
  • some script variables
    Since the callback of youcompleteme#GetCommandResponseAsync() (i.e. s:ShowHoverResult()) is always executed in the buffer/window where the vim cursor is, I use s:hover_syntax to save b:ycm_hover.syntax which might be local to the buffer/window where the mouse pointer is. s:hover_pos saves the position of the identifier at the vim cursor/mouse pointer. s:HoverPopupFunc saves the Funcref of popup_atcursor()/popup_beval(). These script variables free me from writing two s:ShowHoverResult()s.
function s:Hover( hover_type )
  " ...
  let s:hover_syntax = b:ycm_hover.syntax
  if a:hover_type == 'cursorhold'
    let s:hover_pos = screenpos( win_getid(), line( '.' ), col( '.' ) )
    let s:HoverPopupFunc = function( 'popup_atcursor' )
    call youcompleteme#GetCommandResponseAsync(
          \ function( 's:ShowHoverResult' ),
          \ b:ycm_hover.command )
  elseif a:hover_type == 'balloon'
    let s:hover_pos = screenpos( v:beval_winid, v:beval_lnum, v:beval_col )
    let s:HoverPopupFunc = function( 'popup_beval' )
    call youcompleteme#GetCommandResponseAsync(
          \ function( 's:ShowHoverResult' ),
          \ b:ycm_hover.command,
          \ '--bufnr=' . v:beval_bufnr,
          \ '--line_num=' . v:beval_lnum,
          \ '--column_num=' . v:beval_col )
  endif
endfunction

line 1652: s:ShowHoverResult()

  • line 1665: I use strdisplaywidth() instead of len(). This fixes the problem of multi-byte characters the author mentioned here.
    " Find the longest line (FIXME: probably doesn't work well for multi-byte)
    let lines = split( a:response, "\n" )
    let len = max( map( copy( lines ), "strdisplaywidth( v:val )" ) )
  • line 1667: I've optimized some parameters of the popup.
    • border: I add a one-cell-wide border to the popup to make it more conspicuous.
      let border  = { 'above': 1, 'right': 1, 'below': 1, 'left': 1 }
      let padding = { 'above': 0, 'right': 1, 'below': 0, 'left': 1 }
      let left_right_reserve  = border.left  + border.right + padding.left  + padding.right
      let above_below_reserve = border.above + border.below + padding.above + padding.below
    • maxheight: Setting maxheight can prevent the popup from exceeding the bottom of the screen. In vim 8.1, it is possible for popups to exceed the bottom of the screen. In vim 9.0, the problem has been fixed. For compatibility, maxheight should be explicitly set here.
      let maxheight = max( [ s:hover_pos.row - 1, &lines - s:hover_pos.row ] ) - above_below_reserve
    • maxwidth: maxwidth is screen columns minus border and padding and sometimes the potential scrollbar.
      let maxwidth = &columns - left_right_reserve
      " ... (showing the popup)
      " check whether there is a scrollbar
      if popup_getpos( s:cursorhold_popup ).scrollbar
        call popup_setoptions( s:cursorhold_popup,
                            \ { 'maxwidth': maxwidth - 1 } )
      endif
    • minwidth: Setting minwidth can prevent the popup from getting wider or narrower during scrolling. The maximum line width in different pages of a popup might differ, so the width of the popup might change after scrolling down a page.
      let minwidth = min( [ len, maxwidth ] )
  • line 1685: Show the hover popup. Enable resizing and dragging.
    let s:cursorhold_popup = s:HoverPopupFunc(
          \   lines,
          \   {
          \     'col'      : col,
          \     'wrap'     : wrap,
          \     'border'   : [ border.above,  border.right,  border.below,  border.left  ],
          \     'padding'  : [ padding.above, padding.right, padding.below, padding.left ],
          \     'maxheight': maxheight,
          \     'maxwidth' : maxwidth,
          \     'minwidth' : minwidth,
          \     'close'    : 'click',
          \     'fixed'    : 0,
          \     'resize'   : 1,
          \     'drag'     : 1
          \   }
          \ )
  • Demo
    balloon hover

line 1711: s:ToggleHover()

When there is a visible cursorhold/balloon popup, this function will close it. Otherwise, a cursorhold popup will be generated. There is also the <plug>(YCMHover) mapping, which can be used to manually trigger cursorhold hovers or close an existing popup. I also add some mappings to enable/disable auto hover permanently, including <plug>(YCMHoverON) and <plug>(YCMHoverOFF).

function! s:ToggleHover()
  let pos = popup_getpos( s:cursorhold_popup )
  if !empty( pos ) && pos.visible
    call popup_close( s:cursorhold_popup )
    let s:cursorhold_popup = -1
  else
    call s:Hover( 'cursorhold' )
  endif
endfunction

let s:enable_hover = 1
nnoremap <silent> <plug>(YCMHover) :<C-u>call <SID>ToggleHover()<CR>
nnoremap <silent> <plug>(YCMHoverON) :<C-u>call <SID>EnableAutoHover()<CR>
nnoremap <silent> <plug>(YCMHoverOFF) :<C-u>call <SID>DisableAutoHover()<CR>

Balloon hovers might cause the completion menu and the info popup next to it to disappear in insert mode. This is a vim bug (see this issue). The problem has been fixed since vim-patch:9.0.1371. If one's vim doesn't contain this patch, he/she can disable auto hover before entering insert mode and enable auto hover again after leaving insert mode by adding the following code in <YouCompleteMe repo>/autoload/youcompleteme.vim.

autocmd InsertEnter <buffer> silent call s:DisableAutoHover()
autocmd InsertLeave <buffer> silent call s:EnableAutoHover()

line 1270: s:ShowInfoPopup()

In insert mode, the info popup next to the completion menu should not be shown if it contains nothing, so we should judge whether a:completion_item.info is empty. set completepopup+=align:menu prevents the info popup from jumping up and down. I also highlight the info popup as Pmenu, which is PmenuSel by default and that looks too bright.

set completepopup+=align:menu

function! s:ShowInfoPopup( completion_item )
  let id = popup_findinfo()
  if id && !empty( a:completion_item.info )
    call popup_setoptions( id, { 'highlight': 'Pmenu' } )
    call popup_settext( id, split( a:completion_item.info, '\n' ) )
    call popup_show( id )
  endif
endfunction
  • before changing this part
    completion-original
  • after changing this part
    completion-my

Remaining problems

  • I'm considering to let users choose the type of hovers by themselves through something like let g:ycm_hover_type = 'cursorhold/balloon'. (solved)
  • Since ballooneval/balloonevalterm/balloonexpr are global options, they might override the settings by user or from other plugins, or be overridden by other settings. (solved)
  • This change has undergone manual tests, but it lacks automatic tests. I'll try my best to write some tests as soon as possible.

This change is Reviewable

@puremourning
Copy link
Member

Thanks for the PR and the detailed explanation. It will take me a little while to review and test this, as the interactions in this area are kind of complex (also, using balloonexpr may conflict with things like vimspector, which is quite important for me to test).

@codecov
Copy link

codecov bot commented Jan 3, 2023

Codecov Report

Merging #4106 (2b52613) into master (b1a9de9) will increase coverage by 0.35%.
The diff coverage is 33.33%.

❗ Current head 2b52613 differs from pull request most recent head 728fb7e. Consider uploading reports for the commit 728fb7e to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4106      +/-   ##
==========================================
+ Coverage   78.67%   79.02%   +0.35%     
==========================================
  Files          28       28              
  Lines        2724     2703      -21     
==========================================
- Hits         2143     2136       -7     
+ Misses        581      567      -14     

@Shuangcheng-Ni
Copy link
Author

This PR did not include the related changes in my previous one when I commited it, which might cause problems in tests. I've added some commits to include these changes today. Also, I'd like to know whether this needs automatic tests or manual ones, since it is quite hard to test mouse operations using scripts.

@Shuangcheng-Ni Shuangcheng-Ni changed the title Show balloon hovers instead of cursorhold hovers Use balloon hovers instead of cursorhold hovers to show documents Mar 3, 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