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 #798

Merged
merged 6 commits into from
Dec 26, 2021

Conversation

irevoire
Copy link
Contributor

@irevoire irevoire commented Oct 1, 2021

Hello,

I would love to add this feature to helix, but there are multiple issues that I don't know how to solve currently.

  1. I'm currently deserializing and re-serializing the same struct over and over again.
    I can see multiples solutions here, but I'm not a fan of either of them:
    • We could do a match on the keys of the deserialized conf and updates the current_conf accordingly:
        let conf = toml::from_str::<Map<String, Value>>(&args.join(" "))?;
        for (key, value) in conf {
            match key {
                "scrolloff" => cx.editor.config.line_number = value.as_usize()?;
                ... => ...,
            }
    
    The problem with this solution is that every time the Configuration struct is going to evolve, we'll need to remember to update this match.
    • We could create a new struct exactly like the Configuration one but with every field as Optional, and then implements a merge between this new struct and the current one. Same problem as the previous solution.
    • We could keep the code as-is, maybe put one or two things in lazy_static and consider that this code is really not called often and don't need to be fast.
  2. For the autocompletion, if I understood correctly, my function is called for every word. Now we would need way more information about what part of the query we are auto-completing. For example, if we are autocompleting line-numbers I would like the autocomplete to propose "relative" or "absolute" instead of nothing or bad things

Here is a little demo of how it works currently: https://asciinema.org/a/0KjjI5CsWPFwXGuxqN7ERTh4x
Let me know if you are interested and have any idea on how to improve these things.

I would really love to see this feature implemented because when I'm sharing my screen with a colleague, it's easier for them to have absolute line numbers. While when I'm alone, I'm usually deactivating line-numbers entirely and having to update my global configuration is really boring 😒

helix-term/src/commands.rs Outdated Show resolved Hide resolved
@pickfire
Copy link
Contributor

pickfire commented Oct 2, 2021

@irevoire I think you should try kakoune and take inspiration from it, IIRC our current structure for this part is partly based on kakoune so I guess we could do a similar settings there. Like :set buffer xxx yyy.

@irevoire
Copy link
Contributor Author

irevoire commented Oct 3, 2021

Hey, I took a little bit of time to look into kakoune's conf option; I also like their command :set buffer/window/global command, but in our case, from what I see, we would need to add a Config in each buffer?
Maybe we could merge a first PR adding the set command only for the global scope and then a second PR adding the possibility to update the conf in a specific buffer to avoid having one big PR. What do you think?

Whatever happens, I would love to do this work if you are not in a hurry 😄

@irevoire irevoire force-pushed the settings_at_runtime branch 2 times, most recently from 1cbd1ab to 3608654 Compare October 3, 2021 04:31
@archseer archseer linked an issue Nov 10, 2021 that may be closed by this pull request
@irevoire irevoire force-pushed the settings_at_runtime branch 2 times, most recently from 74451fb to 2092d34 Compare December 20, 2021 23:13
fix the clippy warning
@irevoire
Copy link
Contributor Author

Hello @archseer @pickfire, sorry for being this slow to apply your suggestion.
I now use a match as you were asking when applying settings but still Deserialize the default structure once at the launch of the editor to generate all the keys when auto-completing. Is that ok?

Also, I went with a minimal implementation that does not handle the shell things and force you to have precisely three arguments.

Another issue that with this PR is the auto-completion of commands, though. Currently, we have no way to propose values or even detect that we are in the value part and stop trying to autocomplete with keys from what I understand 🤔

I think we can merge this PR as-is, and since the code is quite simple to follow, maybe someone will come back to it and improve it. I don't have a lot of time to put into helix currently, sadly 😔

(and sorry for all the force push, but there was a big rebase to do to come back to master)

helix-term/src/ui/mod.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-term/src/ui/mod.rs Outdated Show resolved Hide resolved
helix-view/src/editor.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
@archseer archseer merged commit a306a10 into helix-editor:master Dec 26, 2021
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.

Ability to set config values at runtime
4 participants