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

Remove uibridge, allow basically unmodified nvim binary #1015

Merged
merged 38 commits into from
Nov 8, 2023

Conversation

georgeharker
Copy link
Collaborator

This allows newer versions of neovim which don't have uibridge, recreates the functionality with api commands and autocommands, and assuming the neovim patch gets accepted (minor listen startup difference) would allow externally installed neovim to be used.

@qvacua
Copy link
Owner

qvacua commented Oct 31, 2023

I'm sorry for being mute for some time. I've been inactive due to some private reasons. I'll get slowly get restarted and go through all PRs.

@georgeharker
Copy link
Collaborator Author

georgeharker commented Oct 31, 2023

Thanks for looking at this.

Most of the changes are code relating to removal of uibridge. I think I did a reasonable job recreating functionality. But some things might not be perfect. I'd kinda like to convert the auto commands to lua and put them in a nice augroup.

I did alter some build and layout to get things running and also to allow a subproject to point at neovim upstream. That meant juggling the packages to point at a stock subproject for neovim.

I'm neither a swift nor react expert- so there's conceivably better or cleaner ways to do a few things.

Lmk if you have any questions on why I did what.

I suspect the string encodings for the messages are non ideal. But that's what you get with the stick messgepack interface. I probably should have converted them to strong enums for cleanliness (which I did for cursor shapes etc).

Happy to help get things sorted / fix stuff up.

@georgeharker
Copy link
Collaborator Author

Upstream neovim changes are in. So from now with the above any Nvim can be pointed at - hopefully avoiding the continual update neovim work.

Copy link
Owner

@qvacua qvacua left a comment

Choose a reason for hiding this comment

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

Just some small comments.

VimR/VimR/PrefMiddleware.swift Outdated Show resolved Hide resolved
@@ -20,6 +20,7 @@ final class PrefMiddleware: MiddlewareType {
do {
let dictionary: [String: Any] = try dictEncoder.encode(appState)
defaults.set(dictionary, forKey: PrefMiddleware.compatibleVersion)
defaults.synchronize()
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need this? According to the doc, it's not needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had a really hard time getting prefs to stick without it and found references to it being needed elsewhere. It's possible that's because I was often killing and restarting vimR in the debugger, but I think it doesn't sync quickly by default.

VimR/VimR/States.swift Show resolved Hide resolved
@georgeharker georgeharker changed the base branch from master to stock-nvim November 5, 2023 13:48

rm -rf ./build
rm -rf ./.deps
make distclean
Copy link

@wookayin wookayin Nov 6, 2023

Choose a reason for hiding this comment

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

I'm getting make: *** No rule to make target 'distclean' errors here (and in NvimServer/NvimServer/bin/build_libnvim.sh). Am I missing something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

May have forgotten to update the clean_all script. Or possibly stock vim only has make clean. I’ll look into that.

Copy link
Owner

Choose a reason for hiding this comment

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

@wookayin: Yes, Neovim has the distclean target. Since the NvimServer folder is almost empty in this PR, make distclean won't work. I'll have a call with @georgeharker in a few days to align on how to build Neovim. Afterwards, we'll tidy up the scripts.

@qvacua qvacua merged commit ffb3423 into qvacua:stock-nvim Nov 8, 2023
@qvacua
Copy link
Owner

qvacua commented Nov 8, 2023

@georgeharker and I will continue to work on https://github.com/qvacua/vimr/tree/stock-nvim . Once that branch is merged to master (hopefully soon), I'll release a snapshot version.


import PackageDescription

let package = Package(
name: "Commons",
platforms: [.macOS(.v10_15)],
platforms: [.macOS(.v13)],
Copy link

Choose a reason for hiding this comment

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

Why the MacOS version bump? I'm still on Big Sur 😭

@wookayin wookayin mentioned this pull request Nov 29, 2023
shanesmith added a commit to shanesmith/vimr that referenced this pull request Jun 23, 2024
The `neoVimBuffer` function was updated in qvacua#1015 to used a lua script
through `nvimExecLua`. The script returns the dictionary retrieved from
the `getbufinfo` vim functions. The issue is that this can grow very
large, especially the `variables` entry, and can cause the lua stack to
grow too big and error.

In the end the `neoVimBuffer` function only needs a small handful of the
entries from `getbufinfo`, and so the lua script has been updated to
return a dictionary with only those entries.

At the same time the `hasDirtyBuffers` function was found to also return
the `getbufinfo` dictionary and was similarly fixed. It was also noticed
that the argument passed into `getbuinfo` here was a vim style
dictionary and not valid lua. This made `hasDirtyBuffers` fail and
caused an issue in `MainWindow.windowShouldClose` where selecting `File
> Close Window` would close the window even when dirty buffers were
present. The proper lua dictionary syntax is now used and fixes this
issues.

Fixes qvacua#1044
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.

4 participants