Allows a subview reference the superview of the subview superview.

[BDisp]

@tig if you run the unit test without this fix it will fail.

[tig]

@tig if you run the unit test without this fix it will fail.

Hmmmm... they are not failing for me. What do you mean?

[tig]

Oh, you mean the new unit test you added in this PR?

Did this issue occur before my PR?

[BDisp]

Did this issue occur before my PR?

Yes, but isn't worth fixes in the develop because the superview param doesn't exist.

[BDisp]

Why TopologicalSort method is static in this PR?

[BDisp]

Did this issue occur before my PR?

Yes, but isn't worth fixes in the develop because the superview param doesn't exist.

I also did this change in the develop in gui-cs#2383

[BDisp]

@tig I did a rebase from the develop branch into your PR to be more easy to find the changes that are afterwards. It seams there is some re-bugs in this PR that was already fixed on the developer branch, like in the Clear method, but perhaps may be have others as well.

[BDisp]

@tig I did a rebase from the develop branch into your PR to be more easy to find the changes that are afterwards. It seams there is some re-bugs in this PR that was already fixed on the developer branch, like in the Clear method, but perhaps may be have others as well.

@tig I found the offending commit and fixed in 534d4ea.
The Revert "Illustrates #2331 (Scrollview not respecting clip) does not reproduce (#2332)" was committed twice on 6bb90ed and 28d7be7

[BDisp]

@tig the changes you made on the Border class is behaving weird redrawing a black rectangle when shrink as bellow:

But it should redraw a right border.

[BDisp]

@tig I think this already working as before more or less. I finished it, so when you have time, please review.

[tig]

Why TopologicalSort method is static in this PR?

Because it doesn't access any instance vars, and making it static will make breaking it out to a separate Layout class in v2 easier. That was my thinking anyway.

[tig]

@tig I did a rebase from the develop branch into your PR to be more easy to find the changes that are afterwards. It seams there is some re-bugs in this PR that was already fixed on the developer branch, like in the Clear method, but perhaps may be have others as well.

@tig I found the offending commit and fixed in 534d4ea. The Revert "Illustrates #2331 (Scrollview not respecting clip) does not reproduce (#2332)" was committed twice on 6bb90ed and 28d7be7

Thank you.

I'm really struggling with merging these days. I don't know if its me doing things incorrectly, or just the complexity.

It's one of the reasons I've started naming my branches with either v1_ or v2_ prefixes (to help me avoid being confused).

[BDisp]

Why TopologicalSort method is static in this PR?

Because it doesn't access any instance vars, and making it static will make breaking it out to a separate Layout class in v2 easier. That was my thinking anyway.

It was only using the this instance, but with the superview param added resolved that. My doubt is do layout be used on other objects than the View class and thus reside on a separate Layout class?

[BDisp]

Thank you.

You welcome.

I'm really struggling with merging these days. I don't know if its me doing things incorrectly, or just the complexity.

I understand you, believe me. I also had struggling with the weird behavior of this PR and only rebasing allowed to find what was causing it. When a lot of conflicts come out it's very hard to resolve if we don't know all the modifications.

It's one of the reasons I've started naming my branches with either v1_ or v2_ prefixes (to help me avoid being confused).

Well thought, that really helps to filter the branches.

@tig I need you help me to reason about the following:
As you know the button in the BordersOnXXX scenarios aren't printed because you are setting the Y position after the smartView is already instantiation with smartView.Y = Pos.Bottom (replaceBorder) + 1;. As you know Bounds is view relative coords and Frame is screen relative coords. The GetMaxNeedDisplay method is calculating well the Bounds that is needed to clear before redrawing because the localization Y was changed and thus calculate a bigger height. Well I think I found the culprit of this issue. See these values when I debugging into this:

this (Button): {{X=52,Y=13,Width=13,Height=1}} the frame isn't set yet.
oldFrame: {{X=52,Y=13,Width=13,Height=1}}
newFrame: {{X=8,Y=2,Width=13,Height=1}}
rect: {{X=8,Y=2,Width=57,Height=12}}
Superview (ContentView): {{X=3,Y=5,Width=30,Height=6}}
Superview.Superview (FrameView): {{X=39,Y=7,Width=40,Height=20}}

As you can analyze the Frame of the ContentView is always relative to the FrameView and not relative to the screen coords. In this case the GetMaxNeedDisplay will always fails when the superview is a content view. In my opinion the Frame property must be always the screen relative coords, but must be content views an exception to the rule?
I'll try to fix this but it must be in both versions v1 and v2.

[tig]

Thank you.

You welcome.

I'm really struggling with merging these days. I don't know if its me doing things incorrectly, or just the complexity.

I understand you, believe me. I also had struggling with the weird behavior of this PR and only rebasing allowed to find what was causing it. When a lot of conflicts come out it's very hard to resolve if we don't know all the modifications.

It's one of the reasons I've started naming my branches with either v1_ or v2_ prefixes (to help me avoid being confused).

Well thought, that really helps to filter the branches.

@tig I need you help me to reason about the following: As you know the button in the BordersOnXXX scenarios aren't printed because you are setting the Y position after the smartView is already instantiation with smartView.Y = Pos.Bottom (replaceBorder) + 1;. As you know Bounds is view relative coords and Frame is screen relative coords. The GetMaxNeedDisplay method is calculating well the Bounds that is needed to clear before redrawing because the localization Y was changed and thus calculate a bigger height. Well I think I found the culprit of this issue. See these values when I debugging into this:

this (Button): {{X=52,Y=13,Width=13,Height=1}} the frame isn't set yet.
oldFrame: {{X=52,Y=13,Width=13,Height=1}}
newFrame: {{X=8,Y=2,Width=13,Height=1}}
rect: {{X=8,Y=2,Width=57,Height=12}}
Superview (ContentView): {{X=3,Y=5,Width=30,Height=6}}
Superview.Superview (FrameView): {{X=39,Y=7,Width=40,Height=20}}

As you can analyze the Frame of the ContentView is always relative to the FrameView and not relative to the screen coords. In this case the GetMaxNeedDisplay will always fails when there is a container inside a view. In my opinion the Frame property must be always the screen relative coords, but must be containers an exception to the rule? I'll try to fix this but it must be in both versions v1 and v2.

Here's my opinion:

The original design of gui.cs, involving Bounds and Frame had a serious flaw. That flaw was Bounds.Location was to always be 0, 0. Another way of saying this is Bounds could have been a Size not a Rect. As a result of this design decision, the way the original border/Title was implemented in Window and FrameView was via the embedded ContentView. This worked well initially but was flawed for the long term.

Since then we have added more and more functionality like Border that needed to work around this awkward design leading to an increasing amount of complexity and fragility to the codebase. We now have many situations where clean OO principles have been broken with views referencing superviews and parents of children of superviews that might also be containers of other superviews and toplevels that might be the top-toplevel but also might be a MdiContaier or a super-view of a child that's not a container that has an embedded TopLevel class because it has a border that needs to paint outside of it's bounds, etc... ;-) Meanwhile clipping, which should just be automatic, has become something that most views have to do special casing around.

In my proposed design for v2 (documented in ./docfx/articles/view.md in my view2_experiment branch (gui-cs#2361) I have tried to think this all through and come up with a new design that does not require the hacky ContentView concept and all of the complex referencing of superviews. I hope it's possible to significantly simplify the codebase while also enabling all the rich features like fancy borders, non-full-screen apps, overlapping/movable/sizable views, etc...

I came to the conclusion that Frame DOES need to be superview-relative (except for the top-toplevel which will have Location = 0,0 where 0,0 is screen relative) and Bounds is relative to Frame. I can't imagine how Frame being screen-relative will ever work.

I think we leave v1 as it was at v1.9 and just admit that it's not worth hacking in more and more small fixes to try and work around this design flaw. There are dozens of users of v1 that are perfectly happy with the features and capabilities of v1.9 and any bugs they hit due to this flaw can be worked around relatively easily.

I think we use v2 as an opportunity fix this design flaw at a very deep level.

Here is a practical proposal (that may feel painful):

1. Document all of the Issues that were fixed in develop since v1.9. Any that we determine were REGRESSIONS from v1.8 or earlier that significantly impact devs, we target at a v1.10 release. The rest we PUNT to v2.0.
2. Save off develop so all of the work that's gone into it since v1.9 is available for reference.
3. Reset develop to the v1.9 tag and rename it v1_develop. We use v1_develop to release v1.10 which will be the LAST release of v1 (unless we find super-critical bugs). main continues to be the v1 release branch until we declare v2.0 "done*.
4. Rename the v2 branch to v2_develop.
5. Run a bunch of experiments where we validate my new design (or disprove it and come up with something better) in the v2 branches.
6. Once we all agree we have a new design we love, we start re-implementing things like fancy borders, 3d effects, etc...
7. We launch v2.0 to a bunch of very excited developers.

Please discuss this here: gui-cs#1940

[BDisp]

I agree and I like the v1.10 idea. I already having a fix for the content view issue. How about I submitted the fix to this branch and if is all right you merge it. Then after you create the v1_develop from the latest v1.91 (I think) you can merge your PR by recreating it to target the v1_develop.

[BDisp]

@tig I'm closing this because I submitted the gui-cs#2385 on the v2. The behavior with the button redraw doesn't reproduce anymore. I hope you don't mind.