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

Draft: Better Configuration/Settings #2143

Closed
wants to merge 4 commits into from
Closed

Conversation

Kethku
Copy link
Member

@Kethku Kethku commented Nov 22, 2023

This PR contains an attempt at improving the state of our settings code to have a more central structure.
Atm this branch doesn't address the goals below beyond just cleaning up the current state of things a bit by moving some files around and refactoring a little. I figured we should discuss the way forward before I just implement something arbitrary.

Goals

  • Currently we source settings from a config file, environment variables, command line arguments, and global neovim state through variables and options. Each method of reading settings is handled differently which is confusing to new contributors, so whatever we do should unify these things in some way
  • Currently our settings are forced to be flat variables that have a single value. This worked pretty ok up till now but falls apart when we want to specify things like fallbacks or allow the user to configure deeper structures. An example usecase is for our font configuration. To handle it properly we likely need to have a slightly more complicated structure as outlined here: Add support for variable font weights #2121 (comment)
  • Neovim plugin configuration is standardizing on tree structures set in lua. Ideally our settings would mirror that structure. Its also possible we could implement our setting change detection using lua metatables which would be a nice change.

To summarize some of the goals, I wrote the following at the top of the settings/mod.rs file which somewhat encapsulates where I'd like us to get:

//! Neovide's settings are stored in a central lazy_static variable called SETTINGS. It contains
//! a hashmap of sub-settings structs that are indexed by their type.
//!
//! The value of each setting are sourced from 4 different places and overwrite each other in this
//! order:
//! 1. Config File
//! 2. Environment Variables
//! 3. Command Line Arguments
//! 4. Neovim Variables and Options
//!
//! This order was selected such that settings methods that change more infrequently are overwritten by
//! settings that change more frequently. A command line argument is more effermable than an environment
//! variable. Similarly a user's config may define commands which increment or decrement a global
//! neovide variable which they wouldn't want to be overwritten by the value in the file.
//!
//! Lastly, some settings are not changeable after startup. These settings are required to be set
//! either before window creation or before the neovim process is started and so cannot be sourced
//! easily from neovim global state.

Possible Directions to Explore

Here are some things I've considered in no particular order:

Bevy Reflect

One source of complexity for our current approach is that we use a derive macro to implement change application. This lets us easily implement these settings structs close to the code that uses them, but is pretty confusing to understand for new comers and in my experience kinda tough to update as you have to reason about what is available where.

The bevy_reflect crate might be a way out of this as it lets us dynamically iterate over the fields and structure of types. It also provides a useful primitive in Dynamic types which can be applied to other values. I envision this as being a useful for achieving the precedence rules. Each source of settings could produce a DynamicStruct which contains some subset of the fields in the final settings structs. Then when a given setting struct is requested, the DynamicStructs could be applied in order to a default setting list to get the final set.

I'm not yet sure how the bevy_reflect crate interacts with other attribute metadata. I believe we will still want to allow renaming of final settings or adjusting the user facing structure via attributes. I hope that bevy_reflect can support such a thing but I didn't see it in my first glance. Its possible we can get around that but im not positive.

Central Setting Tree

If we have a reflection based setting approach, maybe it makes sense to adjust our structure to use a single struct that contains substruct fields for each category of setting. Those sub structs could be defined near the code that uses it like we do today, but then its not just manually registering the types in a central hashmap.

Simplify Drastically

Our current settings approach is pretty complicated and does a lot of "magic". On the one hand that lets the code that uses it be pretty simple, but I also think its possible that the magic gets in the way of people understanding and contributing. Maybe theres a much simpler approach that defines the structure with a little more boilerplate but without playing derive macro tricks? That might be worth exploring as well.

What kind of change does this PR introduce?

  • Fix
  • Feature
  • Refactor

Did this PR introduce a breaking change?

  • Yes
    These changes will almost certainly result in some breaking changes, but if done correctly we should arrive at something that can last longer term.

@Kethku Kethku marked this pull request as draft November 22, 2023 21:24
@fredizzimo
Copy link
Member

I would prefer a central setting tree approach, where everything is defined in a single place, also for the actual setting structs. That way it's easy to see what settings are available and how it's structured. I don't think it's that beneficial to keep the settings near the actual code, since the settings should just be defined using primitive types and perhaps simple enums for some of the settings. It also gets rid of problems, like where the setting should be placed if it's something that's used in a lot of different places.

I think we should keep the current derive based approach, in the end I'm not sure if bevy reflect will be that much simpler, and I think it's a bit overkill for our application. Furthermore, only people making changes to the actual serialisation and notifications and things like that need to actually understand it, the usage is simple and you can typically follow existing examples.

I would also prefer lua and metatabletts for that side of the implementation, but that's a bit controversial, since it makes it harder to use from vimscript. Ideally our config file would use the same approach, but that probably means that we need our own lua interpreter, so that the parsing can be done before Neovim is launched. But if we limit the settings we support then we might get around with parsing them before we call ui_attach.

@Kethku Kethku closed this May 11, 2024
@Kethku Kethku deleted the better-neovide-config branch May 11, 2024 23:41
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

2 participants