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

Fixes #224. Border BorderBrush and Background causes compiler error when his value is -1. #225

Merged
merged 19 commits into from
Sep 3, 2023

Conversation

BDisp
Copy link
Contributor

@BDisp BDisp commented Aug 25, 2023

Also updated to net7.0. Now the unit tests pass.

Now is possible to set the Border.BorderBrush and the Border.Background.

border.mp4

@BDisp
Copy link
Contributor Author

BDisp commented Aug 25, 2023

A better option is to show all the public read/write properties of the derived class to allow the user set anything.

@tznind
Copy link
Collaborator

tznind commented Aug 26, 2023

Thanks for creating this. I will take a closer look at it shortly.

I think though that we want to make changes in Property not Design. And we want it to be generic i.e. we teach Property how to deal with Color enum instead of adding methods like GetBorderBorderBrush()

If I understand the new Terminal.Gui syntax correctly:

  • -1 means 'no explicit color declared'?

So I think the correct behaviour is:

  • When a Property with a Type of Color has a value of -1

    • Interpret as 'none'
    • Do not emit any code in the generated c#
    • Show as -1 or null or none in the gui
    • If this is the case then do not emit any line of code
  • When a Property with a Type of Color has an enum value

    • Treat normally.

This I think is more consistent than the if statements and custom methods? I can make this change and see what happens.

I notice that in the current implementation of this PR the FrameView now has Black on Green for border explicitly (different from main).

A better option is to show all the public read/write properties of the derived class to allow the user set anything.

In the interests of stability I have taken an approach of manually adding each property as designable so that it can be tested thouroughly before released. The problem with reflection is new stuff can creep in untested and result in unstable/wrong behaviour of editor.

@tznind
Copy link
Collaborator

tznind commented Aug 26, 2023

I've created a feature branch for this where I will look at alternative approach. Here is a draft PR to easily see the diff in approach:

BDisp#1

@tznind
Copy link
Collaborator

tznind commented Aug 26, 2023

Also worth noting that I have disabled Borders completely in the v2 branch because they were not working very well.

@tznind
Copy link
Collaborator

tznind commented Aug 26, 2023

With this new change when you add a FrameView to a Window and save it you get the following:

BUT when you reload it it shows green and black and it's Color properties are 0. I will try to reduce this to minimum repro and see if it is an issue with Terminal.Gui itself. The code generated looks fine.

image

 private Terminal.Gui.FrameView frameView;
        
        private void InitializeComponent() {
            this.frameView = new Terminal.Gui.FrameView();
            this.Width = Dim.Fill(0);
            this.Height = Dim.Fill(0);
            this.X = 0;
            this.Y = 0;
            this.Modal = false;
            this.Border.BorderStyle = Terminal.Gui.BorderStyle.Single;
            this.Border.Effect3D = false;
            this.Border.Effect3DBrush = null;
            this.Border.DrawMarginFrame = true;
            this.TextAlignment = Terminal.Gui.TextAlignment.Left;
            this.Title = "";
            this.frameView.Width = 10;
            this.frameView.Height = 5;
            this.frameView.X = 0;
            this.frameView.Y = 0;
            this.frameView.Data = "frameView";
            this.frameView.Border.BorderStyle = Terminal.Gui.BorderStyle.Single;
            this.frameView.Border.Effect3D = false;
            this.frameView.Border.Effect3DBrush = null;
            this.frameView.Border.DrawMarginFrame = true;
            this.frameView.TextAlignment = Terminal.Gui.TextAlignment.Left;
            this.frameView.Title = "Heya";
            this.Add(this.frameView);
        }

@BDisp
Copy link
Contributor Author

BDisp commented Aug 26, 2023

Also worth noting that I have disabled Borders completely in the v2 branch because they were not working very well.

Border doesn't not exist on v2, now it's Frame. I recommend you maintaining the two versions for users that still want use the v1 and for the one who want to use the v2.

@BDisp
Copy link
Contributor Author

BDisp commented Aug 26, 2023

I don't have any wrong behavior with the generated code.

imagem

And using the out FrameView o value, the test pass. I think there is something wrong with the returning result from the RoundTrip method.

        [Test]
        public void TestRoundTrip_BorderColors_NeverSet()
        {
            var result = RoundTrip<Window, FrameView>((d, v) =>
            {
                // Our view should not have any border color to start with
                Assert.AreEqual(-1,(int) v.Border.BorderBrush);
                Assert.AreEqual(-1,(int) v.Border.Background);

            },out FrameView o);

            // Since there were no changes we would expect them to stay the same
            // after reload
            Assert.AreEqual(-1, (int)o.Border.BorderBrush);
            Assert.AreEqual(-1, (int)o.Border.Background);
        }

@BDisp
Copy link
Contributor Author

BDisp commented Aug 27, 2023

See #227 please. Thanks.

@tznind
Copy link
Collaborator

tznind commented Aug 27, 2023

Any time I move a FrameView or Window the border changes to black.

bad-border

While ff1f6f8f4fbf849a22a4e364fc3845dc3ba42bd4 makes the test pass, it doesn't fix the issue in the editor (See above). Also it feels very much like a workaround.

I think the issue could be with Terminal.Gui itself:

  • Why is there a 'magic' -1 value which doesn't even appear in Color (resulting in having to cast). Why not just make nullable i.e. Color? for BorderBrush/Background or add a new enum Color.None or Color.Inherit or Color.Transparent?
  • It feels like there is a bug in the core library around border styles changing when a View is moved to a new parent or being cached so not updating when a view moves or some other circumstances.

I will dig some more around bullet point 2 when I have some time but at the moment Border seems to be too unstable to merge.

@BDisp
Copy link
Contributor Author

BDisp commented Aug 27, 2023

Any time I move a FrameView or Window the border changes to black.

Yes I already stated that in the #227.

While ff1f6f8f4fbf849a22a4e364fc3845dc3ba42bd4 makes the test pass, it doesn't fix the issue in the editor (See above). Also it feels very much like a workaround.

Yes, it was 😞

I think the issue could be with Terminal.Gui itself:

* Why is there a 'magic' -1 value which doesn't even appear in `Color` (resulting in having to cast).  Why not just make nullable i.e. `Color?` for BorderBrush/Background or add a new enum `Color.None` or `Color.Inherit` or `Color.Transparent`?

Well, you know that wasn't me who decided to invent a nonexistent -1 for the Color enum.
For an undefined Color make it nullable is the better option and was that I did for the backing fields private Color? borderBrush; and private Color? background;, but I couldn't do that for the properties public Color BorderBrush and public Color Background because it will imply in a breaking change and force the users to change all these properties to add (Color) casting. But I think it would worth it, but wouldn't be approved.
What value you'll using for the enum Color.None or Color.Inherit? The 0 already exist, so Color.Transparent with a -1 could be a valid option but not all drivers know how to deal with it.

* It feels like there is a bug in the core library around border styles changing when a View is moved to a new parent or being cached so not updating when a view moves or some other circumstances.

Normally a View inherit the parent ColorScheme and that normally is available at runtime when is redrawing. I'll try to find a better workaround for that, although I know I can't spend much time with the v1, but it's the one users actually mostly uses.

I will dig some more around bullet point 2 when I have some time but at the moment Border seems to be too unstable to merge.

I agree with you about the around with bullet 2 and I also will try discover a better fix for that in the core library.

@BDisp
Copy link
Contributor Author

BDisp commented Aug 27, 2023

If you use with the v1_develop branch all the unit tests pass. Only the copy/paste have the issue to get the border black. But at least was something that was fix this some what. I'll try to investigate more.

@BDisp
Copy link
Contributor Author

BDisp commented Aug 27, 2023

@tznind can you provide a unit test where a FrameView copy/paste will fail, please. Thanks.

Edit:
This issue also happens with the Window.

@tznind
Copy link
Collaborator

tznind commented Aug 28, 2023

If you use with the v1_develop branch all the unit tests pass.

Can confirm that v1_develop works much better for the border. Once there is a new nuget package we can bump to that and merge this border change.

Only the copy/paste have the issue to get the border black. But at least was something that was fix this some what. I'll try to investigate more.

Can confirm that copy/paste results in a black border. However saving and reloading does not persist it. I will look into writing a unit test asap.

I wonder if setting the property to -1 at runtime causes the behaviour.

@BDisp
Copy link
Contributor Author

BDisp commented Aug 28, 2023

I already submit the PR gui-cs/Terminal.Gui#2835 that fix this issue. The unit tests will fail without the fix.

@tznind
Copy link
Collaborator

tznind commented Sep 3, 2023

Is gui-cs/Terminal.Gui#2835 in 1.14.0?

Even with v1.14 I am getting test failures with this branch. And dragging a FrameView changes its border to black/green.

@BDisp
Copy link
Contributor Author

BDisp commented Sep 3, 2023

In this PR all tests passed and not having FrameView dragging behavior issue. If you only updated the main with the current Terminal.Gui version, you'll still having fails because you need to merge this PR.

@tznind tznind merged commit 8e3411b into gui-cs:main Sep 3, 2023
@BDisp BDisp deleted the unit-tests-fix_224 branch September 3, 2023 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants