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: Use own data directory instead of nvim's #2135

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

agraven
Copy link
Contributor

@agraven agraven commented Nov 16, 2023

Use a separate directory for storing application data, i.e. window settings, instead of re-using Neovim's.

What kind of change does this PR introduce?

Did this PR introduce a breaking change?

  • Arguably

Existing window settings will be lost when using a neovide version with this change, but that data is arguably somewhat ephemeral in nature, or at least trivial to recreate.

@sid-6581
Copy link
Contributor

I agree the location should change, but the function name neovim_std_datapath is a bit misleading after this change. Also, I don't think the file should be saved directly in the root of AppData/Local on Windows. I also don't know if .config/neovide is the right place for what I agree is ephemeral data. Maybe XDG_STATE_HOME?

As an aside, is the xdg crate really needed? Seems like dirs handles both Windows and Linux, while xdg overlaps in functionality and handles only Linux.

Use a separate directory for storing application data, i.e. window
settings, instead of re-using Neovim's.
@agraven
Copy link
Contributor Author

agraven commented Nov 17, 2023

Reasonable points all around. I updated the function names and unified the implementation to use dirs everywhere. I didn't remove xdg as it's still used in other places, and I figured a larger refactor would be out of scope.

re: it's location on unix, it will be going to .local/share/neovide with the current implementation, not .config/neovide

@Kethku
Copy link
Member

Kethku commented Nov 17, 2023

Hey thanks for the change! I'm spending some time looking into updating our configuration file and data path and this looks like the right way to go. Further, the dirs crate which we've used elsewhere seems to supersede the xdg crate so I'm inclined to use it instead everywhere we need known directories.

But I can snag that change while I'm in the area. I'll merge this in the meantime once the ci goes through

@Kethku Kethku merged commit 8dd735f into neovide:main Nov 17, 2023
2 checks passed
@agraven agraven mentioned this pull request Nov 20, 2023
@dev-ardi
Copy link
Contributor

Reasonable points all around. I updated the function names and unified the implementation to use dirs everywhere. I didn't remove xdg as it's still used in other places, and I figured a larger refactor would be out of scope.

re: it's location on unix, it will be going to .local/share/neovide with the current implementation, not .config/neovide

I'm not a fan of applications unnecesarily polluting my .config. Thanks for using .local/share!

@fredizzimo
Copy link
Member

Here's a previous discussion about dirs/xdg. It looks like dirs does not work as expected on macOS

#1119 (comment)

@fredizzimo
Copy link
Member

fredizzimo commented Nov 20, 2023

Another thing that never got done was to refactor all our configuration to use common functions instead of being scattered all around the place.

#1119 (comment)

@fredizzimo
Copy link
Member

The above two comments are mostly for @Kethku since she is going to look more into this.

@sid-6581
Copy link
Contributor

Another thing that should be fixed is that the json file is loaded multiple times instead of being loaded once and cached.

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.

None yet

5 participants