Skip to content

fix(coc): correct pum col for concealed characters#5590

Open
fannheyward wants to merge 5 commits intomasterfrom
fix/concealed-columns
Open

fix(coc): correct pum col for concealed characters#5590
fannheyward wants to merge 5 commits intomasterfrom
fix/concealed-columns

Conversation

@fannheyward
Copy link
Copy Markdown
Member

@fannheyward fannheyward commented Mar 24, 2026

Closes #5582

This fix was made by Codex with GPT-5.4

@fannheyward fannheyward force-pushed the fix/concealed-columns branch from e498eea to 90da16b Compare March 24, 2026 03:56
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.91%. Comparing base (31b8b5e) to head (f9398d2).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5590      +/-   ##
==========================================
- Coverage   97.91%   97.91%   -0.01%     
==========================================
  Files         280      280              
  Lines       27927    27927              
  Branches     5791     5791              
==========================================
- Hits        27346    27344       -2     
  Misses        111      111              
- Partials      470      472       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fannheyward
Copy link
Copy Markdown
Member Author

@A4-Tacks give this a test

@fannheyward fannheyward changed the title fix(coc): use screenpos to compute columns fix(pum): use screenpos to compute columns Mar 24, 2026
@fannheyward fannheyward requested a review from Copilot March 24, 2026 05:07
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes incorrect popup menu (pum) horizontal positioning when completion happens on lines containing concealed text (and other display-width-affecting characters), addressing issue #5582 by switching to screen-cell–accurate column calculations.

Changes:

  • Use screenpos() to compute the pum’s horizontal offset using actual on-screen columns (so conceal/wide chars don’t skew placement).
  • Add a fallback path to the prior strdisplaywidth()-based calculation when screenpos() yields invalid positions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread autoload/coc/pum.vim Outdated
Comment thread autoload/coc/pum.vim Outdated
@A4-Tacks
Copy link
Copy Markdown
Contributor

This doesn't work

@fannheyward fannheyward force-pushed the fix/concealed-columns branch from 90da16b to 92e478b Compare March 31, 2026 01:53
@fannheyward fannheyward changed the title fix(pum): use screenpos to compute columns fix(coc): correct pum col for concealed characters Mar 31, 2026
@fannheyward fannheyward requested a review from Copilot March 31, 2026 01:57
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread autoload/coc/pum.vim Outdated
Comment thread autoload/coc/pum.vim
Comment thread autoload/coc/pum.vim
@fannheyward
Copy link
Copy Markdown
Member Author

@A4-Tacks

@A4-Tacks
Copy link
Copy Markdown
Contributor

This works correctly in set concealcursor=i, but incorrectly in set concealcursor=

Perhaps we should judge &concealcursor =~# 'i'

This fix also caused a misalignment of set nu, seemingly without considering the window edge

@fannheyward
Copy link
Copy Markdown
Member Author

This works correctly in set concealcursor=i, but incorrectly in set concealcursor=

Perhaps we should judge &concealcursor =~# 'i'

This fix also caused a misalignment of set nu, seemingly without considering the window edge

fixed

@A4-Tacks
Copy link
Copy Markdown
Contributor

Poor 1 error

Expect

foo f
    foo        [A]
   ________________

Actual

foo f
     foo        [A]
    ________________

@fannheyward fannheyward force-pushed the fix/concealed-columns branch from 04a4f9b to b0b1f86 Compare March 31, 2026 09:30
@A4-Tacks
Copy link
Copy Markdown
Contributor

A4-Tacks commented Mar 31, 2026

When the line is too long (out of columns), the pum position at the end of the line is incorrect

There are two situations, nowrap and wrap

@fannheyward
Copy link
Copy Markdown
Member Author

When the line is too long (out of columns), the pum position at the end of the line is incorrect

any screenshot?

@A4-Tacks
Copy link
Copy Markdown
Contributor

echo $COLUMNS outputs 118

  1. Run vim -u mini.vim
  2. Input 150a. <esc> I conceal <esc>
  3. Input A c

set nowrap:
Screenshot_20260331_180406
set wrap:
Screenshot_20260331_180424

@A4-Tacks
Copy link
Copy Markdown
Contributor

It seems that nothing has been fixed in this commit, including the wrap and nowrap

@fannheyward
Copy link
Copy Markdown
Member Author

mini.vim

set nocompatible
set runtimepath^=~/.local/share/nvim/plugged/coc.nvim
filetype plugin indent on
syntax on
syn match Foo /conceal/ conceal
set conceallevel=2 concealcursor=i

set wrap
77305

set nowrap
image

@A4-Tacks
Copy link
Copy Markdown
Contributor

A4-Tacks commented Apr 1, 2026

log.zip

2026-04-01_1775015534212.mp4

@fannheyward fannheyward force-pushed the fix/concealed-columns branch from 4157ad5 to 1554cec Compare April 7, 2026 06:26
fannheyward and others added 4 commits April 14, 2026 20:22
Closes #5582

Use screenpos() on neovim to compute the screen column
offset between the completion start and cursor, avoiding
incorrect results when concealed text is present.

For vim, add s:screen_col() that walks characters with
synconcealed() to account for conceal replacements when
computing the screen column position.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@fannheyward fannheyward force-pushed the fix/concealed-columns branch from 1554cec to 9e3d314 Compare April 14, 2026 12:23
@A4-Tacks
Copy link
Copy Markdown
Contributor

Incorrect, what about you?

@fannheyward
Copy link
Copy Markdown
Member Author

@A4-Tacks should be fixed on wrap/nowrap cases

@A4-Tacks
Copy link
Copy Markdown
Contributor

Still incorrect, perhaps this is too difficult?

@fannheyward
Copy link
Copy Markdown
Member Author

fannheyward commented Apr 16, 2026

The wrap/nowrap cases: if you first input conceal on the begin of the line, then input at the end of the line, the col is incorrect.

If no conceal input at the begin of line first, works as expected.

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.

Incorrect pum position on concealed line

3 participants