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

Width and Height are throwing before IsInitialized is true. #3324

Closed
BDisp opened this issue Mar 14, 2024 · 14 comments
Closed

Width and Height are throwing before IsInitialized is true. #3324

BDisp opened this issue Mar 14, 2024 · 14 comments
Labels
design Issues regarding Terminal.Gui design (bugs, guidelines, debates, etc...)

Comments

@BDisp
Copy link
Collaborator

BDisp commented Mar 14, 2024

When setting the Dim Width and Height before set AutoSize to false throws System.InvalidOperationException: 'Must set AutoSize to false before setting Width.'
Running this unit test will fail.

    [Fact]
    public void Setting_Width_Height_Before_Set_AutoSize_To_False_Do_Not_Throws ()
    {
        var exception = Record.Exception (() => new Label { Width = 10, AutoSize = false });
        Assert.Null(exception);

        exception = Record.Exception (() => new Label { Height = 10, AutoSize = false });
        Assert.Null(exception);
    }
@tig
Copy link
Collaborator

tig commented Mar 15, 2024

This is the correct behavior.

Do not set Width or Height of autosize is true. It makes no sense to do so.

@BDisp
Copy link
Collaborator Author

BDisp commented Mar 15, 2024

This is the correct behavior.

Do not set Width or Height of autosize is true. It makes no sense to do so.

Please, can you understand this. On the constructor we are setting the properties, no matter what order is used. So, we are collecting data for further process. After initialized, if AutoSize is true and trying to set the Width or Height is normal throwing the error, but not at object initializer.
Here other example where this is wrong is on the Editor scenario. See the video and try it and you'll see it throwing the exception now when before doesn't.

WindowsTerminal_XOjkXRKVD2.mp4

@tig
Copy link
Collaborator

tig commented Mar 15, 2024

Setting properties in an initializer is not a constructor.

All it does is set the properties specified in the order provided.

To fix your code simply set AutoSize = false in the initializer before you set Width or Height.

@BDisp
Copy link
Collaborator Author

BDisp commented Mar 15, 2024

Setting properties in an initializer is not a constructor.

I know that, but it's on an object initializer. Did you forget that now mostly of the views are parameter less constructors.

All it does is set the properties specified in the order provided.

That is the correct way if the properties are passed on the constructor parameter, but not now.

To fix your code simply set AutoSize = false in the initializer before you set Width or Height.

Where is that documented? Why a hell a user will bother with that on an object initializer? You was right when you said since a long time ago that the properties checking should be done after a view is initialized, but now you are being incoherent with yourself. See the changed unit test f2998e0. Maybe now you understand this better now.

Note:
I didn't needed to change or comment out any unit tests for all to pass.

@tig
Copy link
Collaborator

tig commented Mar 15, 2024

These three blocks of code do exactly the same thing:

Label label  = new () 
{ 
   Width = 1,
   AutoSize = false
};
Label label = new ();
label.Width = 1;
label.AutoSize = false;
View view = new ();
view.AutoSize = true;
view.Width = 1;
view.AutoSize = false;

All three of these examples are INCORRECT and will cause the exception to be thrown telling the programmer they've made a mistake.

In these 2 PRs, the behavior of AutoSize was updated to be more deterministic. Specifically, I made it clear that if AutoSize = true, it makes no sense to set Height or Width. Setting Height or Width when AutoSize = true will cause erroneous behavior:

When I coded this, I explicitly added the exception on the setters to help find any code that was setting Height or Width if AutoSize was true. I hoped that I had found all instances, but, obviously I did not, because you found one in Editor.... and you found it because the code I wrote is doing its job correctly.

I failed to update the API docs to reflect this.

The API docs for AutoSize should say

Because Width or Height is nonsensical if AutoSize is set to true, setting either Width or Height will throw an exception.

@BDisp
Copy link
Collaborator Author

BDisp commented Mar 15, 2024

But why you don't understand that the user want to set AutoSize to false on initialization, but only because he set the Width and Height first, he get the exception even when the view wasn't initialized. I give up with you.

@tig
Copy link
Collaborator

tig commented Mar 15, 2024

But why you don't understand that the user want to set AutoSize to false on initialization, but only because he set the Width and Height first, he get the exception even when the view wasn't initialized. I give up with you.

But why don't you understand that in a C# intializer, the order in which you set properties matters. If the user wants to set AutoSize to false, so he can then set Width or Height he should do this:

Label label  = new () 
{ 
   AutoSize = false
   Width = 1,
};

@BDisp
Copy link
Collaborator Author

BDisp commented Mar 15, 2024

Because in my opinion that isn't mandatory. As you can see in my test if the user set badly after initialization the AutoSize to true and then set the Width or Height he'll get the exception anyway, do you understand?

@tig
Copy link
Collaborator

tig commented Mar 15, 2024

I understand perfectly. This has NOTHING to do with View.IsInitialized IMO.

The rule of "Don't set Height or Width if AutoSize is true" applies ALWAYS.

I mean, it COULD be softened to only apply if IsInitialized = true, but that just adds complexity to the code (that is all going away once Dim.Auto is complete).

@BDisp
Copy link
Collaborator Author

BDisp commented Mar 15, 2024

Ah ok that is an explanation that I understand because will be big changes.

@dodexahedron
Copy link
Collaborator

dodexahedron commented Mar 15, 2024

These three blocks of code do exactly the same thing:

Label label  = new () 
{ 
   Width = 1,
   AutoSize = false
};
Label label = new ();
label.Width = 1;
label.AutoSize = false;
View view = new ();
view.AutoSize = true;
view.Width = 1;
view.AutoSize = false;

There's a really important distinction between an initializer and explicit sequential assignments that makes this statement not true.

An initializer is special in that the object reference is not pushed onto the evaluation stack if the initializer does not complete successfully. You can sorta consider it as an appendix to whichever constructor was used.

If an exception happens in the first example, you never have a Label object, the newobj never returned a reference, and the heap allocation that was performed is already GC-eligible.

In the second and third examples, the Label and View objects do exist and their references are currently on the stack, and their heap allocations have valid live handles in the current scope.

Concepts such as init accessors and the required keyword would not have any meaning if initializers were the same as explicit assignments. You also wouldn't be able to set members marked readonly from within an initializer if they were the same as assignments.

Key points:

  • Initializer code is do-or-die for creation of the object in the first place.
  • Initializer code runs with the context of a constructor, and is in fact compiled as a method call at the end of the chosen constructor, not after it.

@dodexahedron
Copy link
Collaborator

And thinking about it anyway...

Throwing an exception in the setters for that is bad form. One reason is precisely the fact that the order of setting the properties matters in a program-breaking way.

Property setters are supposed to avoid exceptions unless critically necessary and are supposed to try to keep things in a consistent state if they encounter exceptional situations within themselves.

Proper behavior would be for the setter to check AutoSize first and immediately return without making any state changes if the operation would be illegal.

@tig
Copy link
Collaborator

tig commented Mar 16, 2024

I know throwing exceptions from setters is bad form. I also know initializers are special in how you point out.

That's all irrelevant to this topic.

I put the exception in to find code that was using AutoSize wrong. It is doing its job.

AutoSize is going away Once Dim.Auto is done.

@tig tig added the design Issues regarding Terminal.Gui design (bugs, guidelines, debates, etc...) label Mar 18, 2024
@tig
Copy link
Collaborator

tig commented Mar 18, 2024

Closing as by design.

@tig tig closed this as completed Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Issues regarding Terminal.Gui design (bugs, guidelines, debates, etc...)
Projects
None yet
Development

No branches or pull requests

3 participants