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

ScrollView may not be honoring clip region; CustomButton shows outside #2331

Closed
eonyanov opened this issue Feb 8, 2023 · 9 comments · Fixed by #2338
Closed

ScrollView may not be honoring clip region; CustomButton shows outside #2331

eonyanov opened this issue Feb 8, 2023 · 9 comments · Fixed by #2338
Labels

Comments

@eonyanov
Copy link

eonyanov commented Feb 8, 2023

I created CustomButton class with FrameView, one Label for fill and one Label for text inside.
Then I put some buttons inside the scrollview.

image

Buttons that are outside of the scrollview display a label.

@BDisp
Copy link
Collaborator

BDisp commented Feb 8, 2023

Can you provide the CustomButton code, please. I seems that you are redraw it in a manner that it don't be respecting the ScrollView clip area. FrameView when is redrawing also set his clip area and may be are over-spilling the ScrollView clip area.

@eonyanov
Copy link
Author

eonyanov commented Feb 8, 2023

public class ASCIICustomButton : Button
    {
        public string Description => $"Description of: {id}";

        public event Action<string> PointerEnter = s => {}; 
        
        private Label fill;
        private FrameView border;
        private string id;

        public ASCIICustomButton(string text, Pos x, Pos y, int width, int height) : base(text)
        {
            CustomInitialize("", text, x, y, width, height);
        }
        
        public ASCIICustomButton(string id, string text, Pos x, Pos y, int width, int height) : base(text)
        {
            CustomInitialize(id, text, x, y, width, height);
        }

        private void CustomInitialize(string id, string text, Pos x, Pos y, int width, int height)
        {
            this.id = id;
            X = x;
            Y = y; 
            
            Frame = new Rect
            {
                Width = width,
                Height = height
            };

            border = new FrameView()
            {
                Width = width,
                Height = height
            };

            AutoSize = false;

            var fillText = new StringBuilder();
            for (int i = 0; i < Bounds.Height; i++)
            {
                if (i > 0)
                {
                    fillText.AppendLine("");
                }
                for (int j = 0; j < Bounds.Width; j++)
                {
                    fillText.Append("█");                    
                }
            }

            fill = new Label(fillText.ToString())
            {
                Visible = false, CanFocus = false
            };

            var title = new Label(text)
            {
                X = Pos.Center(),
                Y = Pos.Center(),
            };

            Add(border, fill, title);
        }

        public override bool OnEnter(View view)
        {
            border.Visible = false;
            fill.Visible = true;
            PointerEnter.Invoke(id);
            view = this;
            return base.OnEnter(view);
        }

        public override bool OnLeave(View view)
        {
            border.Visible = true;
            fill.Visible = false;
            if (view == null)
                view = this;
            return base.OnLeave(view);
        }
    }

@eonyanov
Copy link
Author

eonyanov commented Feb 8, 2023

Add how I create window with ScrollView

 public class ScrollViewTestWindow : Window
    {
        private List<Button> buttons;
        private const int BUTTONS_ON_PAGE = 7;
        private const int BUTTON_HEIGHT = 3;

        private ScrollView scrollView;
        
        public ScrollViewTestWindow()
        {
            Title = "ScrollViewTestWindow";
            Width = 80;
            Height = 25;
            
            var titleLabel = new Label("DOCUMENTS")
            {
                X = 0,
                Y = 0
            };
            
            scrollView = new ScrollView()
            {
                X = 0,
                Y = 1,
                Width = 27,
                Height = BUTTONS_ON_PAGE * BUTTON_HEIGHT,
                ShowVerticalScrollIndicator = true,
                ShowHorizontalScrollIndicator = false
            };
            scrollView.ClearKeybindings();          
          
            buttons = new List<Button>();
            Button prevButton = null;
            int count = 20;
            for (int j = 0; j < count; j++)
            {
                Pos yPos = prevButton == null ? 0 : Pos.Bottom(prevButton);
                var button = new ASCIICustomButton(j.ToString(), $"section {j}", 0, yPos, 25, BUTTON_HEIGHT);
                button.Id = $"button{j}";
                scrollView.Add(button);
                buttons.Add(button);
                prevButton = button;
            }

            var closeButton = new ASCIICustomButton( "close", "Close", 0, Pos.Bottom(prevButton), 25, BUTTON_HEIGHT);         
            scrollView.Add(closeButton);
            buttons.Add(closeButton);
            
            var pages = buttons.Count / BUTTONS_ON_PAGE;
            if (buttons.Count % BUTTONS_ON_PAGE > 0)
                pages++;
            
            scrollView.ContentSize = new Size(25, pages * BUTTONS_ON_PAGE * BUTTON_HEIGHT);
            
            Add(titleLabel, scrollView);
        }
    }

@tig tig changed the title Label show outbound ScrollView ScrollView may not be honoring clip region; CustomButton shows outside Feb 9, 2023
@tig tig added the bug label Feb 9, 2023
@tig
Copy link
Collaborator

tig commented Feb 9, 2023

I renamed this issue to hopefully make it more clear. Please let me know if I didn't get it right.

@BDisp
Copy link
Collaborator

BDisp commented Feb 9, 2023

I see in your code that the ScrollViewTestWindow force the width to 80 and the height to 25 but in your image seems that isn't been respected. So, I deduce you are initialize the app by one of the follows ways:
Application.Run (new ScrollViewTestWindow ());
or
Application.Run<ScrollViewTestWindow> ();

These two ways doesn't force the terminal size to what you want because the ScrollViewTestWindow will be the Application.Top and thus will have always the terminal size.
If you really want to force the size for what you want you must add it before run, like this:

Application.Top.Add (new ScrollViewTestWindow ());
Application.Run ();

This is the result:
custombutton-issue

But the bug still exists and I'll investigate it, but was only to clarify what is what you want. But perhaps was a workaround to hidden the bug, was isn't? :-)

Do you allow I add a scenario with the code you provide to the UICalalog?

BDisp added a commit to BDisp/Terminal.Gui that referenced this issue Feb 9, 2023
BDisp added a commit to BDisp/Terminal.Gui that referenced this issue Feb 9, 2023
@tig
Copy link
Collaborator

tig commented Feb 10, 2023

These two ways doesn't force the terminal size to what you want because the ScrollViewTestWindow will be the Application.Top and thus will have always the terminal size. If you really want to force the size for what you want you must add it before run, like this:

Application.Top.Add (new ScrollViewTestWindow ());
Application.Run ();

@BDisp do you think using Application.Run<ScrollViewTestWindow> (); should behave correctly? It seems to me that it should.

@tig tig closed this as completed in #2338 Feb 10, 2023
tig added a commit that referenced this issue Feb 10, 2023
Fixes #2331. ScrollView may not be honoring clip region; CustomButton shows outside
@eonyanov
Copy link
Author

eonyanov commented Feb 10, 2023

Do you allow I add a scenario with the code you provide to the UICalalog?

Yes, sure.

I see in your code that the ScrollViewTestWindow force the width to 80 and the height to 25 but in your image seems that isn't been respected.

I mean that the scrollview is not always the same height as the screen, and in case it is smaller in height, this bug occurs. But as a quick fix, this might be helpful.

By the way, if I use TextView in CustomButton instead of a label, then there is no such problem. But textview does not allow to do text alignment.
Perhaps this will help you find a solution.
image

Anyway, thanks for help :)

@BDisp
Copy link
Collaborator

BDisp commented Feb 10, 2023

Thanks @eonyanov. This is already fixed but to test you have to use the current develop branch instead of the nuget package. The problem was caused by the control was derived from the Button which also uses two Label. These two views use the View base Redraw method that wasn't checking for right container bounds.

@BDisp
Copy link
Collaborator

BDisp commented Feb 10, 2023

These two ways doesn't force the terminal size to what you want because the ScrollViewTestWindow will be the Application.Top and thus will have always the terminal size. If you really want to force the size for what you want you must add it before run, like this:

Application.Top.Add (new ScrollViewTestWindow ());
Application.Run ();

@BDisp do you think using Application.Run<ScrollViewTestWindow> (); should behave correctly? It seems to me that it should.

Yes @tig, it does what it should. So to clarity these bellow samples are similar:

// If the driver is null the InternalInit will be called
Application.Run<ScrollViewTestWindow> ();

or

// The Init must be explicit called.
Application.Init ();
Application.Run (new ScrollViewTestWindow ());

But those two ways will start the app with the ScrollViewTestWindow as Application.Top and thus will always have the terminal size. For this reason I called the @eonyanov attention if he want the ScrollViewTestWindow a fixed size different from the terminal size, then it's needed to add the ScrollViewTestWindow to the Application.Top first, like bellow:

Application.Init ();
Application.Top.Add (new ScrollViewTestWindow ());
Application.Run ();

BDisp added a commit to BDisp/Terminal.Gui that referenced this issue Feb 13, 2023
BDisp added a commit to BDisp/Terminal.Gui that referenced this issue Feb 13, 2023
tig added a commit that referenced this issue Feb 20, 2023
…#2332)

* Proves that the issue #2331 don't have reason to happen.

* fixes #2336

* Fixes #2331. ScrollView may not be honoring clip region; CustomButton shows outside

* More appropriate solution for the issue #2331.

* Start refactoring LineCanvas for mixing line style support (e.g. double into single)

* Add remaining resolvers

* Implement corner border style mixing in LineCanvas

* Refactor and simplify resolvers

* Move tests to Core folder and namespace to Terminal.Gui.CoreTests

* Fixes #2333. TextField is selecting badly a word on double click.

* Add unit test deleting a word with accented char.

* Fixes 2331. ScrollView may not be honoring clip region.

* Add a custom button scenario.

* Fixes #2350. Clipping broke (see Clipping scenario).

* Is preferable use NeedDisplay instead of Bounds.

---------

Co-authored-by: Tig Kindel <tig@users.noreply.github.com>
Co-authored-by: tznind <tznind@dundee.ac.uk>
tig added a commit to tig/Terminal.Gui that referenced this issue Feb 20, 2023
tig added a commit that referenced this issue Feb 21, 2023
#2372)

* Illustrates #2331 (Scrollview not respecting clip) does not reproduce (#2332)

* Proves that the issue #2331 don't have reason to happen.

* fixes #2336

* Fixes #2331. ScrollView may not be honoring clip region; CustomButton shows outside

* More appropriate solution for the issue #2331.

* Start refactoring LineCanvas for mixing line style support (e.g. double into single)

* Add remaining resolvers

* Implement corner border style mixing in LineCanvas

* Refactor and simplify resolvers

* Move tests to Core folder and namespace to Terminal.Gui.CoreTests

* Fixes #2333. TextField is selecting badly a word on double click.

* Add unit test deleting a word with accented char.

* Fixes 2331. ScrollView may not be honoring clip region.

* Add a custom button scenario.

* Fixes #2350. Clipping broke (see Clipping scenario).

* Is preferable use NeedDisplay instead of Bounds.

---------

Co-authored-by: Tig Kindel <tig@users.noreply.github.com>
Co-authored-by: tznind <tznind@dundee.ac.uk>

* Fixes ASCIICustomButton scenario.

* Adds Snake Scenario (#2353)

* Add empty snake scenario

* Move snake head around

* Snake now has a tail

* Rest of logic implementation

* Ctrl K D layout fixes

* Game gets faster as you collect more apples

* Adjust speed increase rate down

* Use white on black for snake and border and red for apple

* Fix ScenarioTests not Disposing Scenario

* Add disposes and fix to use LineCanvas.GenerateImage

* Fix stack overflow, doh!

---------

Co-authored-by: Tig <tig@users.noreply.github.com>

* Fixes 2368. Nested views with height of 1 not rendering correctly.

---------

Co-authored-by: Tig Kindel <tig@users.noreply.github.com>
Co-authored-by: tznind <tznind@dundee.ac.uk>
Co-authored-by: Thomas Nind <31306100+tznind@users.noreply.github.com>
BDisp pushed a commit to BDisp/Terminal.Gui that referenced this issue Feb 27, 2023
BDisp added a commit to BDisp/Terminal.Gui that referenced this issue Feb 27, 2023
BDisp added a commit to BDisp/Terminal.Gui that referenced this issue Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants