Make use of ContentView explicit

[MihaMarkic]

Looking at how Window handles children is a bit of a mess because using ContentView as a (hidden) parent.
Why not let user wrap whatever it is used into a ContentView and restraint Add and Remove to ContentView?

[migueldeicaza]

Can you explain? I am not sure I understand exactly.

[MihaMarkic]

Sure.
When you add a child to a Window, it is wrapped into a ContentView and the actual child of a Window is a ContentView which in turns hosts your view.
Then Window shadows View.GetEnumerator to hide the presence of the ContentView. So, if you cast Window to a View and do a foreach on it you'd get different children (ContentView) than doing foreach over an Window. Which is kind of messy IMO.
Instead I would require that every Window's child is a ContentView in first place. Than the hierarchy is clear and there is no need to shadow View.GetEnumerator.

Does it explain a bit better?

[migueldeicaza]

I am not sure I like this.

[tig]

See this Issue for further related discussion: #508

I suspect Window (and FrameView1) was implemented using an embedded ContentView for ONE reason: As a hacky way to implement a window frame (client/non-client area).

I think Window should be re-factored to have a client & non-client area without resorting to an embedded View.

[BDisp]

The only advantage that I see to the use of ContentView is to handle with the border setting his size with the appropriate dimensions to accommodate all the subviews. Thus, it is much more faster and easy to handle with the positioning of all the subviews.
Do I'm wrong?

[BDisp]

The only advantage that I see to the use of ContentView is to handle with the border setting his size with the appropriate dimensions to accommodate all the subviews. Thus, it is much more faster and easy to handle with the positioning of all the subviews.
Do I'm wrong?

Well, I was totally wrong about ContentView. This must be as is because it is important how the subviews are called to be redrawing. If we push the subviews directly to the Window, when the subviews are redrawn we must use a foreach to redraw all the subviews. But if we call view.Redraw(view.Bounds) and this view has overridden the Redraw method, this will called directly instead calling the base class View first. So, to ensure that the base class Redraw method is always called from a Toplevel, we must use a ContentView class, which not overrides the Redraw method and will always calling the base Redraw method, which have an implementation of the foreach. Thus, the question is if it's better to implementing directly in the base class Toplevel the ContentView class as protected and restrict the constructor to add and remove always to the ContentView, like Window already does?
It's good or bad idea?

[tznind]

But if we call view.Redraw(view.Bounds) and this view has overridden the Redraw method, this will called directly instead calling the base class View first. So, to ensure that the base class Redraw method is always called from a Toplevel, we must [...]

Is the issue people inheriting TopLevel and overriding the Redraw method? or overriding of Redraw among the child views contained in a TopLevel instance? (or have I completely misunderstood?).

You can mark a method sealed to prevent overriding if that helps at all? Or just stipulate that all overriding implementations should make sure to call base.Redraw(bounds) in their implementation (usually calling the base implementation is best practice unless there's a good reason not to).

[BDisp]

Of course it's not an issue to inherit Toplevel and overriding the Redraw, but suppose that you want that the overriden Redraw method be always called by the base Redraw which already have a foreach to interact with all the subviews. Marking as sealed is not an option because the Redraw method is needed to be overriding most of the time. I think it's not guaranteed that the base Redraw is always called, unless there is a way to do that which I would like to know, because the user can delete the base.Redraw(bounds).
And I think that is the reason why @migueldeicaza created a ContentView class in the inherit Toplevel class Window.

[tznind]

What is the code in Redraw in the base class that breaks when people override it (and forget to call base.Redraw)? Is it this bit:

https://github.com/migueldeicaza/gui.cs/blob/5b490a8c8f77f735cc492df813aa34c1ef92108e/Terminal.Gui/Core/View.cs#L1358-L1375

Sorry if I am struggling to understand the issue of Redraw and how adding an intermediate (ContentView) avoids the issue. Is it possible to give a class definition of a derrived class that would not render correctly or shows the issue?

[BDisp]

Sorry, you're right to be surprised by my insistence, but I'm really seriously thinking about uniforming the windows to set the Window by default for multiple windows. In my PR #1331 I am doing a great evolution that will boost the way the Terminal.Gui will interact between all the Toplevel. But I was concentrating on changing only the Toplevel class without edges, as the Window class usual has to be used always with edges, and now I am thinking of changing the class Window seriously to be used as several windows and just Toplevel as a windows container, such as the container MDI. The biggest problem is to unlink the dependence of the Window for the use of the edges, but has the advantage to use the ContentView because its Frame is equal to the width of the Window less the size of the edges and so it is much easier to position the subviews, because they will be as a basis for their positioning the Frame of their SuperView, which is, in this case, the ContentView. Another advantage of the ContentView is not to worry about to handle his subviews by himself, but letting the base class View handling them by calling contentView.Redraw (contentView.Bounds);

[tig]

I do not understand the nature of this discussion.

Is there a serious usability issue in how all this works that users of Terminal.gui are complaining about that is now motivating this?

I think we should have absolute clarity on this before we set about re-engineering this hierarchy. Note that I spent a ton of time looking at this closely last year and while I saw a lot of issues with the current design, none of them were preventing people from building the apps they want to build. I decided to leave well enough alone and focus on things that are actually broken.

I also feel very strongly that any of the changes being discussed here MUST be in a backwards compatible way while ALSO not significantly adding more complexity to the API surface area. We have reached "1.0" status and have many more users of this library building cool things. We should focus our energies on meaningful changes that make their jobs easier, not just better design for design's sake.

I am NOT saying @BDisp's proposal above and in #1331 is a bad idea. I AM saying that the bar for such changes is higher than it was before 1.0 launched, particularly w.r.t. breaking changes. And we need to have real use cases that users of the library are asking for as motiviation.

[BDisp]

@tig this don't break anything. What do you think of this Mdi Container?

[BDisp]

@tig do you remember the CursesDriver issue with the Application.Shutdown, which was causing colors changes after close and open again the driver? After all that was caused by the CursesDriver color issue and not due to the Application.Shutdown.