Skip to content

feat: add TabPanel - #17263

Closed
rbtnn wants to merge 35 commits into
vim:masterfrom
rbtnn:tabsidebar
Closed

feat: add TabPanel#17263
rbtnn wants to merge 35 commits into
vim:masterfrom
rbtnn:tabsidebar

Conversation

@rbtnn

@rbtnn rbtnn commented May 5, 2025

Copy link
Copy Markdown
Contributor

hi, Vim developers .

This PR is to add TabPanel feature. TabPanel is a vertical sidebar that displays tab page labels along the side of the window. It looks like this:

image

This works on Huge build only.

@h-east

h-east commented May 5, 2025

Copy link
Copy Markdown
Member

Would you like to rebase?

@rbtnn

rbtnn commented May 5, 2025

Copy link
Copy Markdown
Contributor Author

Would you like to rebase?

Okay, I have rebased.

@chrisbra
chrisbra requested a review from Copilot May 5, 2025 15:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 adds a new TabSideBar feature by adjusting screen coordinate calculations to account for the sidebar's width, as well as refreshing the UI appropriately when certain commands modify buffers. Key changes include modifying the drawing routines (e.g. in ex_docmd.c, edit.c, and drawscreen.c) to add a TSB_LCOL offset, updating command execution functions to trigger tab sidebar redraws, and adding a new evaluation flag for "tabsidebar" in evalfunc.c.

Reviewed Changes

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

Show a summary per file
File Description
src/ex_docmd.c Adjusted windgoto() to add TSB_LCOL offset for proper positioning.
src/ex_cmds.c Triggered redraw_tabsidebar in response to file rename/write events.
src/evalfunc.c Added a "tabsidebar" feature flag with conditional evaluation.
src/edit.c Adjusted screen_putchar() calls to include TSB_LCOL offset.
src/drawscreen.c Updated several redraw and screen drawing calls to account for the new sidebar.
src/drawline.c Propagated the sidebar offset into the drawline functions.
src/clipboard.c Updated rectangle drawing with TSB_LCOL offset.
src/change.c Ensured tab sidebar is redrawn when buffer content changes.
Files not reviewed (12)
  • Filelist: Language not supported
  • runtime/doc/options.txt: Language not supported
  • runtime/doc/syntax.txt: Language not supported
  • runtime/doc/tabpage.txt: Language not supported
  • runtime/doc/tags: Language not supported
  • runtime/doc/various.txt: Language not supported
  • runtime/syntax/vim.vim: Language not supported
  • src/Make_ami.mak: Language not supported
  • src/Make_cyg_ming.mak: Language not supported
  • src/Make_mvc.mak: Language not supported
  • src/Make_vms.mms: Language not supported
  • src/Makefile: Language not supported

Comment thread src/edit.c Outdated
Comment thread src/edit.c Outdated
Comment thread src/drawscreen.c

@chrisbra chrisbra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks this looks useful. I left a few comments.
Can you please also update $VIMRUNTIME/optwin.vim?

Comment thread runtime/doc/options.txt Outdated
Comment thread runtime/doc/options.txt Outdated
Comment thread runtime/doc/options.txt Outdated
Comment thread runtime/syntax/vim.vim Outdated
Comment thread runtime/syntax/vim.vim Outdated
Comment thread src/screen.c Outdated
Comment thread src/tabsidebar.c Outdated
Comment thread src/tabsidebar.c Outdated
Comment thread src/window.c Outdated
Comment thread src/window.c Outdated
@habamax

habamax commented May 5, 2025

Copy link
Copy Markdown
Contributor

I wonder how it would look like if the filename is very long.

image

As an extreme example, would it take half(or more) of the screen if the buffer/file name is aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.txt?

image

Does it show the shorten path of the file/buffer (would there be the way to set it up?):

image

@zeertzjq

zeertzjq commented May 5, 2025

Copy link
Copy Markdown
Member

From a rough look at the code change I think there is some inconsistency:

  • win_screenpos() and screenpos() start counting column numbers from the right edge of the tabsidebar
  • But getmousepos() starts counting column numbers from the left edge of the screen

@habamax

habamax commented May 5, 2025

Copy link
Copy Markdown
Contributor

I wonder how it would look like if the filename is very long.

Aha, I can see option to wrap it.

Comment thread runtime/doc/options.txt

@thinca thinca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I helped a little bit with the development of the tabsidebar feature.
I replied to the best of my knowledge.

Comment thread runtime/doc/options.txt Outdated
Comment thread runtime/doc/options.txt
Comment thread runtime/doc/options.txt Outdated
Comment thread runtime/doc/options.txt Outdated
Comment thread runtime/syntax/vim.vim Outdated
@h-east

h-east commented May 6, 2025

Copy link
Copy Markdown
Member

Some TbbSideBar related options would be better grouped under 'tabsidebaropt', like 'diffopt'.

Example:

set tabsidebaropt=align:left,columns:0,wrap

Comment thread runtime/doc/options.txt Outdated
Comment thread runtime/doc/options.txt Outdated
Comment thread runtime/doc/options.txt Outdated
Comment thread runtime/doc/tabpage.txt Outdated
Comment thread runtime/doc/options.txt Outdated
Comment thread runtime/doc/syntax.txt Outdated
@jmdevin

jmdevin commented May 6, 2025

Copy link
Copy Markdown
Contributor

I know it sounds like bikeshedding, but I think this feature is better named as 'TabPanel' than 'TabSideBar'. The option for it could be tabpanelopts. I elaborated a bit in another comment: link.

@rbtnn

rbtnn commented May 6, 2025

Copy link
Copy Markdown
Contributor Author

I know it sounds like bikeshedding, but I think this feature is better named as 'TabPanel' than 'TabSideBar'. The option for it could be tabpanelopts. I elaborated a bit in another comment: link.

Okay, I will rename to TabPanel from TabSideBar.

@rbtnn rbtnn changed the title feat: add TabSideBar feat: add TabPanel May 6, 2025
@bfrg

bfrg commented May 6, 2025

Copy link
Copy Markdown
Contributor

Why does this have to be hard coded into vim? Why not a vimscript plugin?

@chrisbra chrisbra closed this in be5bd4d May 14, 2025
@chrisbra

Copy link
Copy Markdown
Member

thanks all!

@rbtnn

rbtnn commented May 14, 2025

Copy link
Copy Markdown
Contributor Author

Thank you for the code review. I look forward to working with you further.

@zeertzjq

zeertzjq commented May 14, 2025

Copy link
Copy Markdown
Member

From a rough look at the code change I think there is some inconsistency:

* `win_screenpos()` and `screenpos()` start counting column numbers from the right edge of the tabsidebar

* But `getmousepos()` starts counting column numbers from the left edge of the screen

This problem still isn't addressed.

Example: :set showtabpanel=2 and click on the top-left corner of the current window. getmousepos() returns

{'screencol': 21, 'coladd': 20, 'screenrow': 1, 'column': 1, 'winrow': 1, 'line': 1, 'wincol': 1, 'winid': 1000}

but screenpos(0, 1, 1) returns

{'col': 1, 'row': 1, 'endcol': 1, 'curscol': 1}

as you can see, col from screenpos() is inconsistent with screencol from getmousepos().

Note that the current behavior is inconsistent with 'tabline', where both screenpos() and getmousepos() start counting row numbers from the top of the screen. If you :set showtabline=2 and click on the top-left cursor of the current window, getmousepose() returns

{'screencol': 1, 'coladd': 0, 'screenrow': 2, 'column': 1, 'winrow': 1, 'line': 1, 'wincol': 1, 'winid': 1000}

while screenpos(0, 1, 1) returns

{'col': 1, 'row': 2, 'endcol': 1, 'curscol': 1}

so row from screenpos() is consistent with screenrow from getmousepos().

@habamax

habamax commented May 14, 2025

Copy link
Copy Markdown
Contributor

This doesn't look right:

with set showtabpanel=1

image

without it:

image

Note to vim/colorschemes -- tabpanel probably needs different background.

@habamax

habamax commented May 14, 2025

Copy link
Copy Markdown
Contributor

There is an issue with :tabonly, let me create a separate one.

@h-east

h-east commented May 15, 2025

Copy link
Copy Markdown
Member

@rbtnn
I think that both of my suggestions 1 and 2 below should be addressed.
#17263 (comment)

You are using too few words, so I cannot understand your thoughts. Could you please not only show the code changes but also articulate your thoughts in writing?

@Shane-XB-Qian

This comment was marked as off-topic.

@h-east

h-east commented May 20, 2025

Copy link
Copy Markdown
Member

@Shane-XB-Qian

not sure if you (anyone) noticed typing became flaky when tabpanel opened, seems 'redraw' too often?

Could you please provide the settings (.vimrc) from that time in the simplest configuration, if possible?

@Shane-XB-Qian

This comment was marked as off-topic.

@h-east

h-east commented May 20, 2025

Copy link
Copy Markdown
Member

I couldn't reproduce it. (ms-win: gvim.exe, vim.exe, Ubuntu 22.04: CLI Vim) (Latest version of Vim)
Does it occur without any input?
Could you please try the following at least?

$ vim --clean +"set showtabpanel=2 tabpanelopt=vert" +tabnew

@Shane-XB-Qian

This comment was marked as off-topic.

@h-east

h-east commented May 22, 2025

Copy link
Copy Markdown
Member

expanding tabpanelopt cli compl items seems wrong

  1. there is no wrap option in its doc which looks should be removed compl items
  2. seems option vert no value can be inputted, so it should be just vert vs not vert:

Both of these are bugs in the tabpanelopt completion feature. What's written in the documentation is correct.
Thanks for letting me know.

chrisbra pushed a commit that referenced this pull request May 22, 2025
Problem:  expansion of 'tabpanelopt' value adds wrong values
          (Shane-XB-Qian, after v9.1.1391)
Solution: update tabpanelopt expansion function and expand only valid
          values (Hirohito Higashi)

related: #17263
closes: #17359

Signed-off-by: Hirohito Higashi <h.east.727@gmail.com>
Signed-off-by: Christian Brabandt <cb@256bit.org>
@jmdevin

jmdevin commented May 27, 2025

Copy link
Copy Markdown
Contributor

@chrisbra I think it would have been good to @ the people in this thread again requesting a review before committing this pr. When I made my comment pointing out some bugs (link), I just reported the first couple I found; I didn't do a fuller deep dive into the feature because I assumed that someone would comment saying it was fixed, please test it again. Looking at a few of the issues reported since this pr was accepted, I'm pretty sure we would have discovered at least a few, if not most, of them beforehand. Issue #17322 is basically the same as what I showed in my initial comment, so I think I would have found it.

I know vim doesn't use a separate development/stable branch, so if this is just how development is done I understand; I just want to make it clear I would've been happy to do some additional testing and bug reporting before this got accepted.

@chrisbra

Copy link
Copy Markdown
Member

Thanks for the feedback. I'll keep it in mind for the future.

@benknoble

Copy link
Copy Markdown
Contributor

Hm. I suppose it's too late to rename g:actual_curtabpage? There's some redundancy there that feels awkward.

@h-east

h-east commented Jan 31, 2026

Copy link
Copy Markdown
Member

g:actual_curbuf and g:actual_curwin existed before this feature was added, so I think it was changed to match those.
Perhaps you want to change it to g:actual_curtab?
Hmm, tab has various interpretations, so I think they went with tabpage.
In fact, tab does not appear alone in :h tabpage.txt (except for :tab as a command).

@chrisbra

Copy link
Copy Markdown
Member

I would also think curtabpage is fine. It's a bit verbose but clear on what is meant.

@benknoble

Copy link
Copy Markdown
Contributor

g:actual_curbuf and g:actual_curwin existed before this feature was added, so I think it was changed to match those.

Ah, I was unaware of that. Consistency is probably best, even if I would drop the "actual" here (it makes it seem like there's a g:curbuf/win/tabpage that's not the real one, as in the "final actual real final version" kind of naming ;).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.