search

withoutboats / notty

A new kind of terminal
GNU Affero General Public License v3.0
2.31k stars 40 forks source link

Store Config info on the Terminal struct #21

Closed withoutboats closed 8 years ago

withoutboats commented 8 years ago

As of writing this issue, config information is stored on a global constant called CONFIG, which is imported into several scopes, both in notty and in notty-cairo. Because configuration info is stored on a constant, the only way to reconfigure notty is to recompile it. This is not a great user experience.

There are several steps that need to be taken to make notty configs convenient to change, this issue is only the first step. Here is a broad roadmap of the whole process to making notty configured dynamically:

  1. The Terminal holds a Config struct, from which all the configurable values for that terminal are accessed. At first, all terminals can be instantiated with the CONFIG constant for this. This is what this issue is about.
  2. This Config struct is made into an argument, and the config info in src/cfg.rs is moved into the scaffolding terminal (though the Config type definition will stay inside of notty).
  3. Add a way for scaffolding to create a Config struct at runtime from some source (probably a TOML file). This way, users can configure the scaffolding terminal without recompiling notty.

This is a great task for someone to pick up to learn the notty code base better (and probably also Rust). It doesn't interact much with the core logic of notty, but it does require you to explore how it is structured. I am glad to provide guidance and answer any questions about this task.

To sort of restate the issue, once this is done, cfg::CONFIG should be invoked in only one location, stored somewhere on the Terminal struct, and everywhere it is invoked now should instead invoke the value stored on the Terminal.

Please drop a note here if you start to work on this and I will assign the issue to you.

waynr commented 8 years ago

@withoutboats I am about to start working on this, please assign the issue to me.

withoutboats commented 8 years ago

I did not know this but in order to assign an issue to someone they have to have push access, so you have push access now. Let's both continue submitting pull requests with changes, which I will merge into master when I see them.

EDIT: Feel free to push your changes to branches here instead of your fork if you want, though, just please don't push to master.

withoutboats commented 8 years ago

Its occurred to me that the Config struct (for now) will need to be on the CharGrid struct, not the Terminal. There are also several things that need to change (like the 'default' Styles object won't be able to use the Default trait, because it needs to be constructed from info on a Config).

waynr commented 8 years ago

I won't push to master! By the way, I think github has a feature that allows you to "protect" some branches, you can try configuring it at https://github.com/withoutboats/notty/settings/branches

Also, I think the Config struct will have to be on Terminal also in order to pass values from it to the TextRenderer used in notty-cairo/src/lib.rs

withoutboats commented 8 years ago

This has been resolved, thanks @waynr.