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

Apps should never reference ConsoleDriver #3622

Closed
tig opened this issue Jul 20, 2024 · 1 comment · Fixed by #3798
Closed

Apps should never reference ConsoleDriver #3622

tig opened this issue Jul 20, 2024 · 1 comment · Fixed by #3798
Assignees
Labels
Milestone

Comments

@tig
Copy link
Collaborator

tig commented Jul 20, 2024

Originally posted by @dodexahedron in #3615 (comment)

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.

@tig tig added the bug label Jul 20, 2024
@tig tig added this to the V2 Beta milestone Jul 20, 2024
@dodexahedron
Copy link
Collaborator

dodexahedron commented Jul 29, 2024

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.1

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.

Footnotes

  1. 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.

@tig tig self-assigned this Aug 6, 2024
@tig tig moved this to 📋 Approved - Need Owner in Terminal.Gui V2 Initial Release Aug 6, 2024
@tig tig moved this from 📋 Approved - Need Owner to 🏗 Approved - In progress in Terminal.Gui V2 Initial Release Aug 6, 2024
tig added a commit that referenced this issue Nov 10, 2024
@tig tig closed this as completed in #3798 Nov 10, 2024
@github-project-automation github-project-automation bot moved this from 🏗 Approved - In progress to ✅ Done in Terminal.Gui V2 Initial Release Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
2 participants