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

fix handling multiple windows for a buffer - fix #11 #12

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

Conversation

FelipeLema
Copy link

@FelipeLema FelipeLema commented Aug 3, 2023

fixes #11

@FelipeLema FelipeLema marked this pull request as ready for review August 17, 2023 23:07
@FelipeLema
Copy link
Author

FelipeLema commented Aug 17, 2023

it's working now, although it's missing tests... but you may want to add those in a separate PR

@FelipeLema
Copy link
Author

cause I'm uncertain of when can I add them

@FelipeLema FelipeLema marked this pull request as draft August 28, 2023 17:03
@FelipeLema FelipeLema marked this pull request as ready for review August 29, 2023 20:36
@wookayin wookayin self-assigned this Sep 2, 2023
Copy link
Owner

@wookayin wookayin left a comment

Choose a reason for hiding this comment

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

This looks good overall, but I'm not 100% sure about the change in the way the viewport is obtained. We get the viewport inside the autocmd handler, and would you please give more context/explanation about why this change is necessary and makes more sense? (the previous logic was written by the original author in 2019, so I'm not quite familiar with why @numiria had needed this -- see 3dc14aa for the context).

rplugin/python3/semshi/handler.py Outdated Show resolved Hide resolved
rplugin/python3/semshi/handler.py Outdated Show resolved Hide resolved
@FelipeLema
Copy link
Author

oh, I see ... it seems fetching the viewports from python can be costly (not sure how bad)

the change to obtaining them from python was rather because I prefer to have as little vimscript as possible... I don't mind making the changes to fetch all viewports from vimscript. will do just that

@FelipeLema
Copy link
Author

ready to go

I re-implemented the get_viewports() function in lua, but it could be written in vimscript in the future

@FelipeLema FelipeLema force-pushed the fix-handling-multiple-windows-for-single-buffer branch from 7c82ba6 to 6225d4b Compare October 2, 2023 15:06
@FelipeLema FelipeLema marked this pull request as draft October 2, 2023 15:32
@FelipeLema FelipeLema marked this pull request as ready for review October 2, 2023 15:41
@wookayin
Copy link
Owner

wookayin commented Oct 2, 2023

Thanks for the contribution again. Actually I'm having a hard time reproducing the bug. I tried this session but haven't had the bug. Can you make a more clean step please, possibly using neither mksession nor absolute (user-specific) paths, starting from nvim --clean? Or writing a unit test will be even better, but I can take care of it as long as it's easily reproducible.

@FelipeLema
Copy link
Author

lemme try and add an unit test

It's probably not going to be ready this week. Hopefully this month

@FelipeLema
Copy link
Author

oh, I see that we're not testing highlights. the to-be-implemented test points to neovim/neovim#6412, which has been solved

we may want to address that TODO first

@wookayin
Copy link
Owner

wookayin commented Oct 2, 2023

I see. I will work on refactoring to switch to extmarks from the legacy highlight API soon. Adding a test is not a big priority.

BTW using the classic vim API synIDattr(synID(line, col, 1), 'name') would still work.

@wookayin
Copy link
Owner

Actually I don't mind we don't have tests yet as we're lacking a infra for testing highlights, but I still would like to reproduce the issue.

@FelipeLema
Copy link
Author

Will pick up the MVE-as-test, although I was shifting my focus into implementing an LSP server that only does semantic highlight using code from this repo

anyone reading this can chip in with such test in the meantime

@FelipeLema FelipeLema force-pushed the fix-handling-multiple-windows-for-single-buffer branch from 594a67e to b5123e3 Compare April 17, 2024 16:20
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.

handle multiple windows for same buffer
2 participants