gui-cs / Terminal.Gui

Cross Platform Terminal UI toolkit for .NET
MIT License
9.54k stars 680 forks source link

ScrollView.ContentSize no setter #3501

Closed tznind closed 3 months ago

tznind commented 3 months ago

Describe the bug

TGD allows you to design properties on views. For ScrollView this includes ContentSize. This functionality relies on both a public get; and set;

It seems the setter no longer exists for this property. Instead there is a method.

image

Is it possible to revert this back to being a property?

yield return this.CreateProperty(nameof(ScrollView.ContentSize));

TGD code that advertises ContentSize as designable

tig commented 3 months ago

First, ScrollView is going away so please just #ifdef around this specific issue until

is done.

However, needing to set the new View.ContentSize via a property setter is news to me. I changed it to SetContentSize () because the logic of dealing with ContentSize_set side-effects was hairy and it seemed to make more sense for it to be an explicit set method.

Happy to change this back, but I'd like to makes sure I understand why first.

I also want to make sure you fully grok the new content scrolling ability built in to View as well. Do you?

If you're not sure, I spent a ton of time on API docs and conceptual docs:

https://gui-cs.github.io/Terminal.GuiV2Docs/docs/layout.html

tznind commented 3 months ago

Happy to change this back, but I'd like to makes sure I understand why first.


From Designer Perspective

For TGD, the Property class handles configurable 'things' on a View.

To create the .Designer.cs file it will write out an assignment LHS = RHS

I could do a workaround e.g. add a new subclass of Property for ContentSize that does arbitrary code gen - but be a bit hacky so thought I would ask first.


From API Perspective

Typically, if a property is settable you would expect to see it on the class. Having it as a method results in:

Again nothing here is a deal breaker.

tznind commented 3 months ago

If you're not sure, I spent a ton of time on API docs and conceptual docs:

I will take a look into this, at the moment I was just trying to get all the tests passing and the ScrollView ones are currently broken.

If all View become scrollable then its probably best to leave them in and try to port the tests to a use case where a View is marked scrollable somehow in the designer.

tig commented 3 months ago

The problem is I've enabled two scenarios and they are fighting with eadch other. I need to rethink this.

1) Virtual content area/Scrolling - ContentSize being explicitly set means the view's content size independent of Viewport.Size - Scrolling is enabled. 2) AutoSize based on ContentSize - ContentSize being explicitly set means if Width or Height is Dim.Auto (DimAutoStyle.Content), the autosize mechanism will use ContentSize to size the view.

I'll get you a PR ASAP that unblocks you by reverting to having set_ContentSize work.