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

Update settings at runtime #819

Closed

Conversation

irevoire
Copy link
Contributor

@irevoire irevoire commented Oct 8, 2021

Hello @pickfire, I was not happy with #798, and I was thinking about your comments, and I thought something was wrong with my first implementation.

Mainly I didn’t like this solution. I feel like matching on a string will be a kind of technical debt. Everytime we’ll update the Config structure we’ll need to remember to update this match statement in a totally random place in the code.

And then, while looking at what kakoune did, if we want something like them we’ll need to store “parts” of the config in multiple place.
For example, we could have our “base config” as we have currently. And then each window and buffer could have its own “incomplete” config.
And then to get the “real” config you would:

  1. Take the base config
  2. Apply the window config to it (=overwriting every field of the base config that are defined in the window config)
  3. Apply the buffer config in the same way

To make this “type-safe” I didn’t find any other solution than creating a “typed fsm” (I don’t know if it’s the term) where a config can either be Complete or Incomplete.

If the config is Complete, when we access one field it returns the expected value as before. If it’s Incomplete it returns an Option.
The code is a little bit raw right now; maybe we could create a macro or something like that.

But before spending too much time on it I would like to hear what you think of this idea. Is it something we want to merge?
It changes a lot of code, but I think it’ll allow us to do some great things later in a type-safe and fast manners!

/// Padding to keep between the edge of the screen and the cursor when scrolling. Defaults to 5.
pub scrolloff: usize,
scrolloff: Option<usize>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't have to do this, we can just use a default for it in case it isn't there rather than being None.

Copy link
Contributor

@pickfire pickfire left a comment

Choose a reason for hiding this comment

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

I think being able to change all runtime settings is what we want later. But I don't think we need to keep track of whether values is set or not, we can just use the default and when it is set, override it with what the values was set.

@irevoire
Copy link
Contributor Author

But then if I have a "line-number = none" in my global config and I want to change it back to the default value: "line-number = absolute".
I'll get a config struct with all its field at the default value and I can't detect what changed?

@pickfire
Copy link
Contributor

I'll get a config struct with all its field at the default value and I can't detect what changed?

Why do you want to detect what's changed? Line number none is a value by itself, which is not the default. If user set it to none obviously user wants it being none.

@irevoire
Copy link
Contributor Author

irevoire commented Oct 11, 2021

If the user set it to none obviously user wants it being none.

Yes, my question was not about setting the value to none, this should work without any effort.
My question was: when the user wants to set it back to absolute at runtime, I'm going to end up with a struct where all its fields are at the default value where I can't know what the user wanted to do.

@pickfire
Copy link
Contributor

My question was: when the user wants to set it back to absolute at runtime, I'm going to end up with a struct where all its fields are at the default value where I can't know what the user wanted to do.

I don't think we need to do that, we can just mention the defaults. That's how vim and kakoune did it IIRC, they just mentioned the default and you can set it back to that value yourself, not like set ft=default.

@irevoire
Copy link
Contributor Author

If you really want that I can do it with the other PR. But from a user perspective, I find it quite bad personally.

If I think “I want to set absolute number” I’m not thinking “I want the default settings back”... I don’t even know what is the default setting usually 😒
I’ll need to remember the default for each setting because if I don’t my modification will just get ignored without any warning or error... Yeah not a fan.

@pickfire
Copy link
Contributor

If I think “I want to set absolute number” I’m not thinking “I want the default settings back”... I don’t even know what is the default setting usually unamused

Regarding this, it should be shown in infobox like what kakoune did, since we can't expect user to remember.

So I guess if we display it in the infobox it solves the issue?

@archseer
Copy link
Member

Sorry for the delay, I'm not completely satisfied with the amount of boilerplate code here (and needing to subtype config into complete/incomplete). Trying to think if there's a better way to accomplish this.

I think I agree with @pickfire in that it's not too important to us to track which values are set and which fall back to the defaults. Rather, I'd keep a single config object (like we have now, that merges built-in defaults with the config file), then :set would directly change it at runtime as needed.

If I think “I want to set absolute number” I’m not thinking “I want the default settings back”... I don’t even know what is the default setting usually

Precisely. If I change a value I can always manually change it back.

@irevoire irevoire closed this Oct 17, 2021
@irevoire
Copy link
Contributor Author

Closing in favour of #798

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.

3 participants