(ISSUE-21) Work on distribution of notty configuration data throughout library.

[waynr]

@withoutboats here's what I've got so far.

Currently I am working to remove all usage of Config::default() as the source of configuration other than in scaffolding and the tests in various modules since I'm not sure how else we would make use of configuration with the approach I've taken so far of replacing default() methods with new(config: Config) for all structs that need to be passed configuration data.

The problem I am running into currently is what appears to be overgeneralization of the Grid class: https://github.com/withoutboats/notty/blob/master/src/terminal/char_grid/grid.rs#L24

Looking through the rest of the source code I only see two specific types of Grid: Grid<CharCell> and Grid<i32>. The reason I think of this as problematic is because there are a number of T::default() calls used to construct Grid elements--which sort of enforces the need for CharCell structs to call Config::default() in CharCell::default() to obtain configuration data, which makes it difficult for me to imagine a way to pass the Terminal-owned Config down to the CharCell. Here are a couple possiblities I am considering:

• Create a Configurable trait that requires a configure(&self, config: Config) method, add that to the Grid type constraints; then use this to configure the CharCell created by T::default().
• Remove generics from Grid entirely since the only specific type used in this library is Grid. I think this is my preferred approach, unless you anticipate new specific types of Grid in the near future.

[withoutboats]

Haha! This is a pretty interesting case, I'm glad it emerged because its a pretty fun problem to think about imo.

I think there are actually a few other options in addition to the two you mentioned, and I think any of them would be suitable except the one you said you preferred - removing the type parameter from Grid. Let me explain why.

The most obvious advantage of generics is as you identified - that we can easily reuse a Grid<T> with a wide variety of different types. And you're 100% right that this advantage is unimportant for this use case - we use a Grid<i32> for testing, but in practice we only care about Grid<CharCell>.

Generics have another advantage, though, which I think is even more important: they enforce encapsulation. Grid<CharCell> knows everything there is to know about CharCell; Grid<T> only knows what we tell it about T. I'd much rather have the compiler enforce that I don't call arbitrary methods on CharCell than be responsible for enforcing that rule myself, and I'm willing to write a little bit of boilerplate to get this guarantee.

Currently, we have allowed Grid to expect that T is Default + Clone. But it turns out our T is not Default anymore, so what we need to do is rewrite Grid so it doesn't have that Default requirement anymore, while telling Grid as little as possible about T - Grid needs to know how to make a T when it resizes itself so that it can fill itself with 'default' Ts, and there are a few ways we could communicate this to Grid:

• As you mentioned, we could replace Default with a new trait, which constructs a T from a Config struct, and implement that trait for both CharCell and i32. Note that I think the function defined in the trait should be a constructor, not a method (fn(Config) -> Self rather than fn(&self, Config) -> Self) This is a very valid option, but it raises the question: do we pass Config into every method that could create a new T, or do we store a Config on Grid? Either variant is a valid solution, and I don't feel strongly about the difference between them.
• We could also require a 'default' value to be passed to the Grid constructor, store that value on the Grid, and then clone it every time we need to create a new member of the grid. This would reduce the bounds on T to just T: Clone.
• Finally, every function that currently calls T::default could instead accept a function as an argument, with the bound Fn() -> T. For the Grid<i32> test cases, we would pass || 0 as this argument, and for the actual cases in CharGrid we would pass a closure like || CharCell::config(self.config) or whatever. Something that is both an advantage and a disadvantage here is that every time you call a method, you can make a different decision about how it should construct a T.

Any of these are fine solutions in my opinion; there are differences between them in terms of performance, boilerplate, and flexibility, but I don't think they are significant (in particular any performance difference is essentially nil, don't worry about that at all). Whichever of these sounds best to you I'd be happy with.

[waynr]

@withoutboats so this is pretty much ready for review, although it needs a little more cleanup to be completely ready for merge. Left to do:

• [x] Always pass and store Config objects by reference.
• [ ] Write some more Config test cases to validate behavior under various error conditions.
• [ ] Move Palette definition to somwhere within notty...? It's not clear to me from the current state of discussion on withoutboats/notty-encoding#3 whether you still think it should be moved here. I personally still think it' would be useful for primarily ANSI-based apps to tap into a small part of the notty protocol to define an application-specific Palette but don't care enough at this point to keep arguing.
• [ ] Move the bg, fg, and cursor color definitions into the Palette struct.

[withoutboats]

I'll review this either tomorrow or Thursday, thanks!

[withoutboats]

This looks pretty good. I made an inline comment about a way you can avoid some type conversions. There's one larger thing I'd like to change, sorry: I think the TOML parsing should happen in scaffolding, and not in notty. Implementations other than scaffolding may want to store configs in a totally different way, and I don't want them to have to have the dependency on toml that they don't use. I also think of notty as the "reference implementation" for a kind of terminal that will need to be copied widely if its to be actually useful, and storing configs in toml definitely isn't a part of the 'notty standard.'

This is kind of annoying I know because you can't implement this stuff as methods directly on the config struct if they're in scaffolding. :frowning:

It's not clear to me from the current state of discussion on withoutboats/notty-encoding#3 whether you still think it should be moved here. I personally still think it' would be useful for primarily ANSI-based apps to tap into a small part of the notty protocol to define an application-specific Palette but don't care enough at this point to keep arguing.

Yeah, I'm sorry, I let the conversation drift into a more philosophical direction. If you think there are useful commands that can be implemented using a Palette argument, I'll look forward to those proposals. For now I'll review that PR and we'll store Palette in the encoding library in preparation for that. Is it ready to land?

[waynr]

I think the TOML parsing should happen in scaffolding, and not in notty. Implementations other than scaffolding may want to store configs in a totally different way, and I don't want them to have to have the dependency on toml that they don't use.

Is it practical to hide the toml-using code behind a conditional compilation flag ? I don't have a real strong objection to moving most of the Config code, but I also don't see any reason not to provide hypothetical users of notty with the convenience of a means to read configuration values if there is a way they can easily opt out and provide their own config file reading code. Actually, my strongest objection is simply that I don't have much time..

I have considered factoring out most of the Config code here into a separate library that would provide macros that allow most of it to be generated as a mini "configuration DSL". This approach probably wouldn't be possible if we split the definition of the Config struct away from the definition of the methods that load it.

I also think of notty as the "reference implementation" for a kind of terminal that will need to be copied widely if its to be actually useful, and storing configs in toml definitely isn't a part of the 'notty standard.'

It's not clear to me how the use of toml in the reference implementation makes it part of the "notty standard". If that's the case, would you also expect any application that uses notty as a reference to also us a data structure exactly like the Config to manage their own configuration? And to construct the other object relationships exactly as is done here? I would think that the important part of this library as a reference implementation of the notty protocol is the way it renders and passes input/output between the user and the application(s). The use of toml vs ini vs yaml vs json seems like a relatively trivial matter.

This is kind of annoying I know because you can't implement this stuff as methods directly on the config struct if they're in scaffolding.

My understanding is that you can implement methods on items even if you didn't originally define those items. So if we do decide to move the toml code into scaffolding (which it looks like we will at this point) then it shouldn't be a problem to move the impl blocks also

For now I'll review that PR and we'll store Palette in the encoding library in preparation for that. Is it ready to land?

Let me take a look tomorrow after work and get back to you, I'm feeling pretty droopy right now.

[waynr]

@withoutboats so I gave it a little more thought and, considering the relatively immaturity of the toml library I agree that it would be better to move that to a higher-level library. I still don't necessarily understand why implementation details of notty have the same kind of protocol-defining importance of notty-encoding (or why the latter is signficant/separate if this is the case). I'm not sure exactly when I will move the code, maybe this weekend.

[waynr]

I think it would make sense to put the Config in an Rc, and have the Terminal and each CharGrid have a separate instance of it, but to pass it by reference to each function that needs it on any of the components of CharGrid.

Okay, I think I am coming around to your way of thinking. In fact, I would say that we shouldn't even pass the entire Config reference to the Cursor method, just call the method on Cursor that sets its color whenever appropriate. I'll try to finish this part tonight or tomorrow night.

So if we do decide to move the toml code into scaffolding (which it looks like we will at this point) then it shouldn't be a problem to move the impl blocks also

So I was wrong about this of course, it is not possible to impl methods on a struct defined in other libraries unless impling a particular trait on that struct. I think someone tried to explain this to me a couple weeks ago, too bad I don't learn just by having people say words at me...

As you can see, I have moved all the toml -> Config code into scaffolding in the past few commits. After doing this, I have somewhat of a concern about the Config object. Specifically, this toml -> Config code is directly dependent on every member of Config since it is responsible for populating new ones from compatible serialized types. I see this as a problem because now scaffolding is dependent on Config in a way that it was not before. If someone were to change the name or type of a Config member, it would be a backwards-incompatible change requiring a version bump in notty to avoid scaffolding accidentally upgrading to an incompatible minor or patch level version.

This means, for instance, that the comment in withoutboats/notty-encoding#3:

Also, if you're going to encapsulate the 'config colors' into a single type, I think you should include the default foreground and background colors as well.

ought to imply a version bump in notty itself.

[waynr]

So TL;DR for my last comments...I think I would like to hard reset this branch to 0a78bbf because it unnecessarily binds scaffolding, a user of notty, to an implementation detail of notty-specific Config.

[waynr]

Okay, I think I am coming around to your way of thinking. In fact, I would say that we shouldn't even pass the entire Config reference to the Cursor method, just call the method on Cursor that sets its color whenever appropriate.

This statement doesn't make sense, what I meant is "we shouldn't even pass the Config reference to the Cursor methods, instead just pass the necessary member values".

[withoutboats]

As you can see, I have moved all the toml -> Config code into scaffolding in the past few commits. After doing this, I have somewhat of a concern about the Config object. Specifically, this toml -> Config code is directly dependent on every member of Config since it is responsible for populating new ones from compatible serialized types. I see this as a problem because now scaffolding is dependent on Config in a way that it was not before. If someone were to change the name or type of a Config member, it would be a backwards-incompatible change requiring a version bump in notty to avoid scaffolding accidentally upgrading to an incompatible minor or patch level version.

Once notty is versioned, any breaking change to its public API requires a version bump, whether or not scaffolding depends on that part of the public API. Other, unknown dependencies (including those created in the future) need to be able to properly declare which versions of notty they're compatible with. notty is in exteme flux right now and so nearly every significant commit changes its public API.

While things are so unstable, scaffolding is only distributed with the source of notty, and the build_test script checks that scaffolding is in sync with the current public API (all commits merged into master should pass build_test, though I've been lazy and that hasn't always been the case).

Notty also exposes a lot right now, like very few of the methods on Terminal or CharGrid are actually things I think should be public, its a byproduct of how I structured it so I could iterate quickly while things were still changing so much.

This statement doesn't make sense, what I meant is "we shouldn't even pass the Config reference to the Cursor methods, instead just pass the necessary member values".

This makes a lot of sense, especially when its only 1 member of config that's passed in.

[waynr]

Once notty is versioned, any breaking change to its public API requires a version bump, whether or not scaffolding depends on that part of the public API.

This is true enough, I guess the reason I focused so strongly on this new tight binding between scaffolding and Config members is that scaffolding doesn't really have any reason to be exposed to details of Config except for the seemingly arbitrary constraint that notty not use toml directly. Maybe this will change someday in the future with something like a GUI color chooser? But even then, I wonder if it would be better design for the GUI to communicate using the notty protocol to set colors rather than directly editing notty's configuration. Can you think of any reasons that scaffolding or any other hypothetical user of notty would need access to details of notty::cfg::Config?

Anyway, I am beginning to annoy myself with my argumentativeness, I hope I'm not annoying you also :). You didn't directly respond to the preference I expressed for resetting this branch back to 0a78bbf--are you open to this?

Notty also exposes a lot right now, like very few of the methods on Terminal or CharGrid are actually things I think should be public, its a byproduct of how I structured it so I could iterate quickly while things were still changing so much.

So this makes me wonder: is notty stable enough for a community of contributors and users? For example, when could I reasonably expect to write my own notty application or terminal and file bugs here when crates are released that make changes to the public API?

[waynr]

Just updated Terminal and CharGrid to use Rc<Config>.

[withoutboats]

Hey, sorry for not responding to some of your more substantial comments for a few days. Been allocating my notty time to the sub-dividable screens branch #9, which is approaching review time. :smile:

I've been having a lot of back and forth about how exactly configuration should be layered. Its complicated because it cuts through so many of the layers that exist right now (as you've certainly noticed, having to put config references into all these methods). I think right now the important thing is getting any sort of dynamic configuration landed, and we can iterate on it over time if we have to.

The upshot of this is that I'm fine with stepping back to the previous commit or anything else that you think makes sense. For that question, and for where Palette lives, I'll leave up to you. So I think that means the main thing left on this branch is testing?

It'd be exciting to get both this and #9 landed in the next week or so! I'll ping you when #9 could use a review.

[waynr]

Rebasing this onto master is not trivial--I will hopefully feel like sitting at my computer sometime this weekend to work on it. Luckily all the commits in this branch were building and passing tests before I moved on to the next when I last worked on it.

[withoutboats]

Hey, I'll be happy to handle the rebase if you want. Thanks for working on this.

[withoutboats]

This has been squashed and merged, thanks for the PR!