Truncate extra_menu_info when completeopt is in popup mode #3682
Conversation
Fixes ycm-core#3547 Based entirely on: puremourning@0dd1b7d
Codecov Report
@@ Coverage Diff @@
## master #3682 +/- ##
==========================================
+ Coverage 89.33% 89.36% +0.02%
==========================================
Files 25 25
Lines 3151 3168 +17
==========================================
+ Hits 2815 2831 +16
- Misses 336 337 +1 |
There was a problem hiding this comment.
Thanks for doing this!
One thing with mocking the underlying methods in the test is that means we don't get coverage of them. I think we have a thing in the test suite called like MockVimModule or something that allows you to tell it the values of vim settings that it should return. That might be a better way to mock &columns for example.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @dhleong)
python/ycm/client/completion_request.py, line 197 at r1 (raw file):
if not preview_info.startswith( extra_menu_info ): preview_info = extra_menu_info + '\n\n' + preview_info extra_menu_info = extra_menu_info[ : ( max_width - 3 ) ] + '...'
thinking about it. do we need to do this ? I mean maybe set extra_menu_info to '' here ?
As you said, we're sticking it in the popup anyway and the truncated version is probably of relatively little use....
There was a problem hiding this comment.
But otherwise 
Reviewed 5 of 5 files at r1.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @dhleong)
There was a problem hiding this comment.
Makes sense, will do.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @puremourning)
python/ycm/client/completion_request.py, line 197 at r1 (raw file):
Previously, puremourning (Ben Jackson) wrote…
thinking about it. do we need to do this ? I mean maybe set
extra_menu_infoto''here ?As you said, we're sticking it in the popup anyway and the truncated version is probably of relatively little use....
I thought about that too, but here's two reasons to keep it as-is:
- You get to keep some glanceable information, without scrolling to select an item to see everything
- It would result in an inconsistent look—some items that fit within the
max_widthwould show up in the "extras" column, while some would not.
There was a problem hiding this comment.
@dhleong Thanks for the pull request!
Reviewed 5 of 5 files at r1.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @dhleong and @puremourning)
doc/youcompleteme.txt, line 2939 at r1 (raw file):
When this option is set to '1', YCM will add the 'preview' string to Vim's 'completeopt' option (see ':h completeopt'). If your 'completeopt' option already has 'preview' set, there will be no effect. Alternatively, when set to
Instead of doc/youcompleteme.txt you should edit README.md and then use vim-tools to generate the update to the doc/youcompleteme.txt.
python/ycm/vimsupport.py, line 1314 at r1 (raw file):
def UsingPreviewPopup(): return 'popup' in ToUnicode( vim.options[ 'completeopt' ] ).split( ',' )
.split( ',' ) isn't needed here.
Also return b'popup' in vim.options[ 'completeopt' ] works, so even ToUnicode() can be avoided.
python/ycm/client/completion_request.py, line 197 at r1 (raw file):
Previously, dhleong (Daniel Leong) wrote…
I thought about that too, but here's two reasons to keep it as-is:
- You get to keep some glanceable information, without scrolling to select an item to see everything
- It would result in an inconsistent look—some items that fit within the
max_widthwould show up in the "extras" column, while some would not.
I agree with @dhleong on this. I believe we should keep a truncated extra_menu_info.
There was a problem hiding this comment.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic and @puremourning)
doc/youcompleteme.txt, line 2939 at r1 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
Instead of
doc/youcompleteme.txtyou should editREADME.mdand then use vim-tools to generate the update to thedoc/youcompleteme.txt.
Ah, right. I went ahead and updated README.md but am running into this error trying to use ycm-core/vim-tools:
Traceback (most recent call last):
File "./vimdoctool.py", line 42, in <module>
logger.addHandler(coloredlogs.ColoredStreamHandler(show_name=True))
AttributeError: module 'coloredlogs' has no attribute 'ColoredStreamHandler'
python/ycm/vimsupport.py, line 1314 at r1 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
.split( ',' )isn't needed here.Also
return b'popup' in vim.options[ 'completeopt' ]works, so evenToUnicode()can be avoided.
Done.
Allows us to get coverage of DisplayWidth()
There was a problem hiding this comment.
I was able to @patch the vim.options value, but I didn't find an obvious way to mock &columns, so I modified _MockVimOptionsEval to pull from that so tests can override such things in a non-destructive way.
I didn't find an easy way to mock the result of strdisplaywidth, since DisplayWidth and DisplayWidthOfString both use GetIntValue. We could bake something into _MockVimFunctionsEval but that seems brittle, and might cause weird test failures for future features.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic and @puremourning)
Thanks! Here's what i usually do: |
|
fantastic contribution, BTW. Really appreciate it :) |
|
Aha, that worked! And thanks :) I can't really take much credit for it, though, since it's basically just your code. |
There was a problem hiding this comment.
I didn't find an obvious way to mock
&columns
All vim options should be possible to be updated through ycm.tests.test_utils.VIM_OPTIONS. Since this would be a destructive change, there's a wrapper for this ugliness in ycm.tests.conftest module called UserOptions.
You can see how it works in ycm.tests.youcompleteme_test module, in test YouCompleteMe_NoPythonInterpreterFound_test, but it's basically:
def SomeNewTest_test():
with UserOptions( { '&any':'options', '&you':'want' } ):
# The body of the testThis should be able to clean up the code significantly. That's why we've never before had t @patch( 'vim.options' ).
Reviewed 4 of 4 files at r2, 1 of 1 files at r3.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @dhleong and @puremourning)
python/ycm/tests/test_utils.py, line 186 at r2 (raw file):
def _MockVimOptionsEval( value ): result = VIM_MOCK.options.get( value )
Can't we just add &completeopt to VIM_OPTIONS?
Hah. No i just did the easy bit. you've done the hard part |
There was a problem hiding this comment.
All vim options should be possible to be updated through ycm.tests.test_utils.VIM_OPTIONS. Since this would be a destructive change, there's a wrapper for this ugliness in ycm.tests.conftest module called UserOptions
Aha, very cool. I figured there'd be some way to do this!
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic and @puremourning)
python/ycm/tests/test_utils.py, line 186 at r2 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
Can't we just add
&completeopttoVIM_OPTIONS?
Reverted this change in favor of leaning on UserOptions, and added some code to ensure vim.options reads from VIM_OPTIONS.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @dhleong and @puremourning)
python/ycm/client/completion_request.py, line 197 at r1 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
I agree with @dhleong on this. I believe we should keep a truncated
extra_menu_info.
@puremourning Any comments or is this "resolved"?
python/ycm/tests/test_utils.py, line 645 at r4 (raw file):
VIM_MOCK.error = VimError VIM_MOCK.options = MagicMock() VIM_MOCK.options.__getitem__.side_effect = _MockVimOptions
Mind explaining why this is needed or useful? How does it help us make sure that vim.options reads from VIM_OPTIONS?
There was a problem hiding this comment.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @dhleong and @puremourning)
python/ycm/tests/client/completion_request_test.py, line 220 at r4 (raw file):
@patch( "ycm.vimsupport.DisplayWidthOfString", len )
Hmm... this leaves DisplayWidthOfString() not covered by the tests.
We do have _MockVimCommand, which is made the side effect of VIM_MOCK object. I should be possible to extend it to support strdisplaywidth() vim function.
If I'm reading the code correctly, just making _MockVimCommand() "aware" of strdisplaywidth() would make tests work without @patch and would improve the coverage.
There was a problem hiding this comment.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic and @puremourning)
python/ycm/tests/test_utils.py, line 645 at r4 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
Mind explaining why this is needed or useful? How does it help us make sure that
vim.optionsreads fromVIM_OPTIONS?
Without this, getting a key from vim.options doesn't seem to return anything, so the vim.options['completeopt'] check always returns False.
python/ycm/tests/client/completion_request_test.py, line 220 at r4 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
Hmm... this leaves
DisplayWidthOfString()not covered by the tests.We do have
_MockVimCommand, which is made the side effect ofVIM_MOCKobject. I should be possible to extend it to supportstrdisplaywidth()vim function.If I'm reading the code correctly, just making
_MockVimCommand()"aware" ofstrdisplaywidth()would make tests work without@patchand would improve the coverage.
My concern is that I've patched it to be implemented as len, but that's not actually the case: emoji, for example, could be two chars but render as 1. This implementation is fine for these tests, but if any functions in the future use DisplayWidthOfString, it could be misleading?
There was a problem hiding this comment.
Thanks for the effort. I can live with the one method not covered.
Reviewed 2 of 4 files at r2, 1 of 1 files at r3, 2 of 2 files at r4.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic and @dhleong)
python/ycm/vimsupport.py, line 1314 at r4 (raw file):
def UsingPreviewPopup(): return b'popup' in vim.options[ 'completeopt' ]
Why did this change ? If a future option is added called nopopup, this won't work:
python/ycm/client/completion_request.py, line 197 at r1 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
@puremourning Any comments or is this "resolved"?
yep, reasons make sense.
There was a problem hiding this comment.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)
python/ycm/vimsupport.py, line 1314 at r4 (raw file):
Previously, puremourning (Ben Jackson) wrote…
Why did this change ? If a future option is added called
nopopup, this won't work:
Hmm interesting point. It was a suggestion from @bstaletic to simplify the code. Looks like there's also a popuphidden option; should we handle that in some way?
There was a problem hiding this comment.
@dhleong Thanks for the pull request.
I usually don't mind imperfect coverage. Here I'm more concerned about having one or two tests that work differently than the rest, since Ben and I will be the ones maintaining this code for an indefinite amount of time.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @dhleong and @puremourning)
python/ycm/vimsupport.py, line 1314 at r4 (raw file):
Previously, dhleong (Daniel Leong) wrote…
Hmm interesting point. It was a suggestion from @bstaletic to simplify the code. Looks like there's also a
popuphiddenoption; should we handle that in some way?
Just revert the change that I requested. I wasn't thinking of these other possibilities.
python/ycm/tests/client/completion_request_test.py, line 220 at r4 (raw file):
Previously, dhleong (Daniel Leong) wrote…
My concern is that I've patched it to be implemented as
len, but that's not actually the case: emoji, for example, could be two chars but render as1. This implementation is fine for these tests, but if any functions in the future useDisplayWidthOfString, it could be misleading?
You can use @patch( 'thing', return_value = 42 ).
There was a problem hiding this comment.
@bstaletic Totally understand, and happy to work together to find a solution that seems maintainable to you.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic and @puremourning)
python/ycm/vimsupport.py, line 1314 at r4 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
Just revert the change that I requested. I wasn't thinking of these other possibilities.
Done.
python/ycm/tests/client/completion_request_test.py, line 220 at r4 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
You can use
@patch( 'thing', return_value = 42 ).
I'm afraid I don't follow what you're suggesting here. I'm already using @patch to provide a "good enough" implementation of DisplayWidthOfString for these specific cases. We could build out a regex and emulate strdisplaywidth() and provide a @patch-able function in the ycm.tests.test_utils namespace that forces tests to provide their own implementation, eg:
def _MockStrDisplayWidth( s )
raise Exception("You must @patch ycm.tests.test_utils._MockStrDisplayWidth")
But without the DisplayWidthOfString patch we already get an error Unexpected evaluation: strdisplaywidth so I'm not sure what that gains us.
We could also provide a fake implementation of that function, which would indeed let us avoid @patch, but if anyone needs to write a test in the future that feeds emoji or asian letters into something that depends on DisplayWidthOfString, their results will not match what Vim would do, and it would not be obvious that they need to patch something in ycm.tests.test_utils.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)
python/ycm/tests/client/completion_request_test.py, line 220 at r4 (raw file):
Previously, dhleong (Daniel Leong) wrote…
I'm afraid I don't follow what you're suggesting here. I'm already using
@patchto provide a "good enough" implementation ofDisplayWidthOfStringfor these specific cases. We could build out a regex and emulatestrdisplaywidth()and provide a@patch-able function in theycm.tests.test_utilsnamespace that forces tests to provide their own implementation, eg:def _MockStrDisplayWidth( s ) raise Exception("You must @patch ycm.tests.test_utils._MockStrDisplayWidth")But without the
DisplayWidthOfStringpatch we already get an errorUnexpected evaluation: strdisplaywidthso I'm not sure what that gains us.We could also provide a fake implementation of that function, which would indeed let us avoid
@patch, but if anyone needs to write a test in the future that feeds emoji or asian letters into something that depends onDisplayWidthOfString, their results will not match what Vim would do, and it would not be obvious that they need to patch something inycm.tests.test_utils.
Boris is just saying that you can patch it here to return the specific value required for this test. It's not a big deal len is fine for now.
There was a problem hiding this comment.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @dhleong)
python/ycm/tests/client/completion_request_test.py, line 220 at r4 (raw file):
Previously, puremourning (Ben Jackson) wrote…
Boris is just saying that you can patch it here to return the specific value required for this test. It's not a big deal
lenis fine for now.
@dhleong Sorry for the delay. I didn't want to talk out of my ass, so I went and implemented the changes I was complaining about. The following patch makes the tests work exactly the same, but is consistent with how we mock other commands.
diff --git a/python/ycm/tests/client/completion_request_test.py b/python/ycm/tests/client/completion_request_test.py
index 522c7e80..3435ef9a 100644
--- a/python/ycm/tests/client/completion_request_test.py
+++ b/python/ycm/tests/client/completion_request_test.py
@@ -217,7 +217,6 @@ class ConvertCompletionResponseToVimDatas_test:
} )
- @patch( "ycm.vimsupport.DisplayWidthOfString", len )
def TruncateForPopup_test( self, *args ):
with UserOptions( { '&columns': 60, '&completeopt': b'popup,menuone' } ):
extra_data = {
@@ -244,7 +243,6 @@ class ConvertCompletionResponseToVimDatas_test:
} )
- @patch( "ycm.vimsupport.DisplayWidthOfString", len )
def OnlyTruncateForPopupIfNecessary_test( self, *args ):
with UserOptions( { '&columns': 60, '&completeopt': b'popup,menuone' } ):
extra_data = {
@@ -270,7 +268,6 @@ class ConvertCompletionResponseToVimDatas_test:
} )
- @patch( "ycm.vimsupport.DisplayWidthOfString", len )
def DontTruncateIfNotPopup_test( self, *args ):
with UserOptions( { '&columns': 60, '&completeopt': b'preview,menuone' } ):
extra_data = {
@@ -296,7 +293,6 @@ class ConvertCompletionResponseToVimDatas_test:
} )
- @patch( "ycm.vimsupport.DisplayWidthOfString", len )
def TruncateForPopupWithoutDuplication_test( self, *args ):
with UserOptions( { '&columns': 60, '&completeopt': b'popup,menuone' } ):
extra_data = {
diff --git a/python/ycm/tests/test_utils.py b/python/ycm/tests/test_utils.py
index 85ad9f00..2ba12678 100644
--- a/python/ycm/tests/test_utils.py
+++ b/python/ycm/tests/test_utils.py
@@ -43,6 +43,8 @@ MATCHDELETE_REGEX = re.compile( '^matchdelete\\((?P<id>\\d+)\\)$' )
OMNIFUNC_REGEX_FORMAT = (
'^{omnifunc_name}\\((?P<findstart>[01]),[\'"](?P<base>.*)[\'"]\\)$' )
FNAMEESCAPE_REGEX = re.compile( '^fnameescape\\(\'(?P<filepath>.+)\'\\)$' )
+STRDISPLAYWIDTH_REGEX = re.compile(
+ '^strdisplaywidth\\( ?\'(?P<text>.+)\' ?\\)$' )
SIGN_LIST_REGEX = re.compile(
'^silent! sign place buffer=(?P<bufnr>\\d+)$' )
SIGN_PLACE_REGEX = re.compile(
@@ -290,6 +292,10 @@ def _MockVimEval( value ):
if value == REDIR[ 'variable' ]:
return REDIR[ 'output' ]
+ match = STRDISPLAYWIDTH_REGEX.search( value )
+ if match:
+ return len( match.group( 'text' ) )
+
raise VimError( 'Unexpected evaluation: {}'.format( value ) )
Unless @puremourning has something to say against this, I'd apply this patch.
There was a problem hiding this comment.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic and @puremourning)
python/ycm/tests/client/completion_request_test.py, line 220 at r4 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
@dhleong Sorry for the delay. I didn't want to talk out of my ass, so I went and implemented the changes I was complaining about. The following patch makes the tests work exactly the same, but is consistent with how we mock other commands.
diff --git a/python/ycm/tests/client/completion_request_test.py b/python/ycm/tests/client/completion_request_test.py index 522c7e80..3435ef9a 100644 --- a/python/ycm/tests/client/completion_request_test.py +++ b/python/ycm/tests/client/completion_request_test.py @@ -217,7 +217,6 @@ class ConvertCompletionResponseToVimDatas_test: } ) - @patch( "ycm.vimsupport.DisplayWidthOfString", len ) def TruncateForPopup_test( self, *args ): with UserOptions( { '&columns': 60, '&completeopt': b'popup,menuone' } ): extra_data = { @@ -244,7 +243,6 @@ class ConvertCompletionResponseToVimDatas_test: } ) - @patch( "ycm.vimsupport.DisplayWidthOfString", len ) def OnlyTruncateForPopupIfNecessary_test( self, *args ): with UserOptions( { '&columns': 60, '&completeopt': b'popup,menuone' } ): extra_data = { @@ -270,7 +268,6 @@ class ConvertCompletionResponseToVimDatas_test: } ) - @patch( "ycm.vimsupport.DisplayWidthOfString", len ) def DontTruncateIfNotPopup_test( self, *args ): with UserOptions( { '&columns': 60, '&completeopt': b'preview,menuone' } ): extra_data = { @@ -296,7 +293,6 @@ class ConvertCompletionResponseToVimDatas_test: } ) - @patch( "ycm.vimsupport.DisplayWidthOfString", len ) def TruncateForPopupWithoutDuplication_test( self, *args ): with UserOptions( { '&columns': 60, '&completeopt': b'popup,menuone' } ): extra_data = { diff --git a/python/ycm/tests/test_utils.py b/python/ycm/tests/test_utils.py index 85ad9f00..2ba12678 100644 --- a/python/ycm/tests/test_utils.py +++ b/python/ycm/tests/test_utils.py @@ -43,6 +43,8 @@ MATCHDELETE_REGEX = re.compile( '^matchdelete\\((?P<id>\\d+)\\)$' ) OMNIFUNC_REGEX_FORMAT = ( '^{omnifunc_name}\\((?P<findstart>[01]),[\'"](?P<base>.*)[\'"]\\)$' ) FNAMEESCAPE_REGEX = re.compile( '^fnameescape\\(\'(?P<filepath>.+)\'\\)$' ) +STRDISPLAYWIDTH_REGEX = re.compile( + '^strdisplaywidth\\( ?\'(?P<text>.+)\' ?\\)$' ) SIGN_LIST_REGEX = re.compile( '^silent! sign place buffer=(?P<bufnr>\\d+)$' ) SIGN_PLACE_REGEX = re.compile( @@ -290,6 +292,10 @@ def _MockVimEval( value ): if value == REDIR[ 'variable' ]: return REDIR[ 'output' ] + match = STRDISPLAYWIDTH_REGEX.search( value ) + if match: + return len( match.group( 'text' ) ) + raise VimError( 'Unexpected evaluation: {}'.format( value ) )Unless @puremourning has something to say against this, I'd apply this patch.
After being away from this for a while, I see the advantage of doing it this way now. It might still be confusing for the odd case of certain strings that don't have a display width equal to their length, but in the general case this is probably easier to maintain. Patch applied!
There was a problem hiding this comment.
Once again, thanks for the pull request.
Reviewed 2 of 2 files at r6.
Reviewable status: 1 of 2 LGTMs obtained (and 1 stale)
There was a problem hiding this comment.
Mucho appreciado. THanks so much 👍
Reviewed 2 of 2 files at r6.
Reviewable status:complete! 2 of 2 LGTMs obtained
|
Thanks for sending a PR! |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r7.
Reviewable status:
|
Thanks for sending a PR! |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r8.
Reviewable status:
|
Thanks for sending a PR! |
1 similar comment
|
Thanks for sending a PR! |
|
Finally. Thanks, bot. |
Fixes #3547
PR Prelude
Thank you for working on YCM! :)
Please complete these steps and check these boxes (by putting an
xinsidethe brackets) before filing your PR:
rationale for why I haven't.
actually perform all of these steps.
Why this change is necessary and useful
[Please explain in detail why the changes in this PR are needed.]
Per discussion in #3547 the popup used when 'popup' is included in completeopt does not get to specify a minimum width, so for languages with verbose signatures like Typescript, the extra information (IE: documentation) that should be viewable there gets very squished and hard to read.
By introducing this simple heuristic, created by @puremourning, of allowing the extra_menu_info to have no more than ~1/3rd of the screen's width in length, we are left with ~1/3rd of the screen for the actual completion
word, and ~1/3rd for the documentation. Since we are copying the extra_menu_info into the preview window, information loss is minimal.We could theoretically introduce an option that could disable this truncation behavior, but I think the real solution in that case is probably for the user to not enable the 'popup' completeopt, since the popup is pretty much unusable in cases where this truncation would kick in.
This change is