gui-cs / Terminal.Gui

Cross Platform Terminal UI toolkit for .NET
MIT License
9.63k stars 687 forks source link

Apps should never reference `ConsoleDriver` #3622

Open tig opened 2 months ago

tig commented 2 months ago

_Originally posted by @dodexahedron in https://github.com/gui-cs/Terminal.Gui/pull/3615#discussion_r1685170103_

I have never cared for this method's name, either.

Typically, an instance method on a type that has a simple verb name results in that verb being performed on the instance of the type. In other words, a Move instance method implies that the ConsoleDriver is being moved, which of course is not what's happening.

This repositions ConsoleDriver's equivalent of cursor position without moving the actual cursor, so is really just moving a 2D index for internal operations.

The method seems unnecessary anyway, however. The Row and Col properties are both public auto-properties. Why not make their set accessors public if the consumer can already effectively change them directly?

I semi-agree. My take is ConsoleDriver details should not be exposed to app code, AT ALL.

All of these operations (AddRune, Move, GetAttribute, ...) should be on either View or Application.

This issue is to track this.

dodexahedron commented 2 months ago

I also mostly agree, but perhaps have a different ideal or at least more words (ha - never) about how I think we should go about it.

As is common in even .net itself, it's totally cool and often good to hide the internal exemplar implementation that most people will probably end up using, meaning making ConsoleDriver internal is something I'm 100% in favor of.[^FrozenDogFood]

However...

It's still a good idea to abstract the concept of the console away, for the same reason it was originally done - there are differences between platforms, and there always will be. And if we don't have an abstraction, what public API surface we do expose ends up coupling consumer code to our implementation anyway, half defeating the purpose of the endeavor.

While the BCL may certainly continue to evolve to make the platform differences less and less apparent, as has been happening, there will always be something that needs to be special-cased. Without an abstraction, even if it's a pretty thin one, that just leads to significantly less-maintainable code, due to scattered workarounds that also end up being bug and regression factories later on.

Instead, I strongly believe we MUST create a public interface as the externally-visible abstraction of the low-level nuts and bolts.

This is a low-effort thing we can do (and really should do in an SDK in 2024) so that consumers can still implement their own extensions, at most, or that they can have a stable contract for interfacing with TG at a lower level, at minimum.

That just means we need to dogfood any such interface, as well.

Fortunately, ReSharper makes that a fairly simple task, at least on the surface, since you can "extract interface," which will grab all the visible members of the class and make an interface from them. There is a high probability that we would, however, need to make some modifications to things to eliminate assumptions made internally that directly affect the externally visible API surface.

That would also be a boon for testing, both internally and for consumers. Interfaces make things so much easier - especially in that realm.

In fact, I'd even go as far as to say I would support sealing the platform-specific drivers, as well, (but leaving them public), and if necessary, define platform-specific interfaces to be the sole means of extending them, because - as above - we don't want consumer code dependent on implementation of those either, if they're writing code at that level or lower. They'd be free to wrap them, since they'd still be public, and would be able to use and pass them around as either their native types or as the interface, but they'd be required to stick to the interface, otherwise, without ability to directly override the implementations of those drivers.

[^FrozenDogFood]: An example of this that has come up incidentally several times in the past few weeks, for me, is the System.Collections.Frozen types (another of one of the Stephens' babies). They follow exactly this pattern. All of the public types are either static or abstract, and the actual implementations are all internal sealed, completely transparent to the consumer, but able to adapt to the use case for more finely-tuned efficiency. That's why you can't instantiate any of those yourself, and have to use either a builder or a ToFrozenXxxx method (both of which are expensive and guarantee a copy of the collection, by the way). There are even several type-specific implementations, for Int32 and string keys, specifically, to help squeeze out better memory performance (in the int case) and better CPU performance for both (for both similar and different reasons). The analogy for us is platform or console host/shell/terminal/PTY/phase-of-the-moon idiosyncrasies.