Panel Scrollbar Update (clean) #749

KenediaDev · 2022-09-06T08:53:38Z

Container.cs

Properties:

added a _panelScrollbar property which is either null or the scrollbar of the container
added a new public property ScrollPadding which determines the Padding on the left side of the Scrollbar
added a new public bool property ScrollbarVisible which tells if the scrollbar is visible or not
added a new public int property ScrollbarWidth which returns the current scrollbar width (0 if null)
added a new public Point property MaxSize which lets you limit the size a container would get resized to
added a new public int property MaxWidth which lets you limit the width a container would get resized to -> MaxSize.x
added a new public int property MaxHeight which lets you limit the width a container would get resized to -> MaxSize.y

Methods:

exclude the scrollbar from the Child handling of the container
take MaxSize into account when resizing the container
disposing the scrollbar on dispose

Scrollbar.cs

Properties:

added a new public bool property Drawn which tells if the scrollbar is visible and drawn or not
added a new public int property ScrollbarWidth which returns the scrollbar width

Panel.cs

Methods:
RecalculateLayout

updating the ScrollbarVisible property
updating the _panelScrollbar if not null to match the container size
updating _backgroundColorBounds to only color up to the scrollbar mid

UpdateScrollbar

adjusted _panelScrollbar.Right to get the position right

FlowPanel.cs

Methods:
all variants of ReflowChildLayout

using the ContentRegion instead of the container bounds for layout calculations to avoid the scrollbar bounds

Control.cs

Properties:

added a new protected rectangle property _backgroundColorBounds which sets the rectangle which would be colorized by BackgroundColor. If not set it will colorize the whole Control like it used to do. This is required to colorize not the whole panel but only until the mid of the scrollbar.

Methods:
Draw

take the _backgroundColorBounds over drawBounds if set

dlamkins

I don't think the solution to the scrollbar problem is going to be to adjust the ContentRegion for it.

It makes sense for things like webbrowsers because the content is assumed to be reactive on its own for various sizes, but we're not fluid like HTML + CSS allows. We're far more like WinForms. Since the ContentRegion can be defined, this gets into some weird behavior.

As a big issue with the scrollbar currently is that it can overlap the content in the panel, it may be ideal to just provide a virtual function that returns a Rectangle which is used to then place the scrollbar. The default implementation would implement it for the Panel control where it sits in the middle of the border line (which is visible when ShowBorder is true). Other controls which inherit from Panel could then provide their own Rectangle to position the scrollbar if they need something special. I think this helps things remain more standard and gives more flexibility than the Scrollbar Padding property.

I think we can get a PR through for the MaxWidth and MaxHeight through much quicker if we can focus on them seperately. I think that the scrollbar still needs more discussion before we decide on the solution here.

dlamkins · 2022-09-07T21:31:38Z

Blish HUD/Controls/Scrollbar.cs

+        /// <summary>
+        /// See if the <see cref="Scrollbar"/> is beeing drawn.
+        /// </summary>
+        public bool Drawn 
+        {
+            get => Visible && _scrollbarPercent < 0.99;
+        }        


Since the plan is to move towards not having a scrollbar control at all, I'd like to avoid adding any "short-cut" APIs that can be done by the consumer itself.

dlamkins · 2022-09-07T21:37:46Z

Blish HUD/Controls/Container.cs

+            //get => _panelScrollbar != null && _panelScrollbar.Drawn;
+            //get => _panelScrollbar != null;


Needs cleaned up.

dlamkins · 2022-09-07T21:39:52Z

Blish HUD/Controls/Container.cs

+        protected Vector2 _scrollPadding = new Vector2(4, 0);
+        /// <summary>
+        /// Padding on the left side of the <see cref="Scrollbar"/>.
+        /// </summary>
+        public Vector2 ScrollPadding {
+            get => _scrollPadding;
+            set => SetProperty(ref _scrollPadding, value, true);
+        }


What is ScrollPadding for? I don't see it referenced anywhere else except in the ScrollbarWidth (and that appears to only use the X).

dlamkins · 2022-09-07T21:43:01Z

Blish HUD/Controls/Container.cs

+        protected bool _scrollbarVisible = false;
+
+        /// <summary>
+        /// Space which is taken from the <see cref="Scrollbar"/> to be drawn inside the <see cref="Panel"/>.


This isn't a panel. This is a container.

dlamkins · 2022-09-07T21:47:18Z

Blish HUD/Controls/Control.cs

+        /// The rectangle which will be colored behind the <see cref="Control"/> by BackgroundColor.
+        /// <br/><br/>Default: <see cref="Rectangle.Empty"/>
+        /// </summary>
+        protected Rectangle _backgroundColorBounds;


I think we perhaps had a misunderstanding about this. Background color should fill the full bounds. We can get rid of this.

The tint I was referring to shown here is drawn by the panel control itself and fills the ContentRegion.

Blish-HUD/Blish HUD/Controls/Panel.cs

Lines 313 to 318 in 736ba25

if (_showTint) {

spriteBatch.DrawOnCtrl(this,

ContentService.Textures.Pixel,

this.ContentRegion,

Color.Black * 0.4f);

}

dlamkins · 2022-09-07T21:57:32Z

Blish HUD/Controls/Panel.cs

            this.ContentRegion = new Rectangle(leftOffset,
                                               topOffset,
-                                               _size.X - leftOffset - rightOffset,
+                                               _size.X - leftOffset - rightOffset - (ScrollbarVisible ? (ScrollbarWidth / 2) + (ScrollbarVisible ? 0 : RIGHT_PADDING) : 0),


I don't think we should be magically updating the boundaries of ContentRegions like this. It's confusing if your ContentRegion is not the size that you have specified as a developer and further it gets us into weird domino behaviors with panels that manage the position of their children based on the content region size.

dlamkins · 2022-09-07T22:02:36Z

Blish HUD/Controls/Container.cs

+            var cR = ContentRegion.ToBounds(this.AbsoluteBounds);
+            var contentScissor = Rectangle.Intersect(scissor, cR);


What is this change for?

dlamkins · 2022-09-07T22:20:19Z

Blish HUD/Controls/Container.cs

+        protected Point _maxSize = Point.Zero;
+        /// <summary>
+        /// Maximum Size the <see cref="Container"/> can autoresize to.
+        /// </summary>
+        public Point MaxSize {
+            get => _maxSize;
+            protected set => SetProperty(ref _maxSize, value, true);
+        }
+
+        /// <summary>
+        /// Maximum Width the <see cref="Container"/> can autoresize to.
+        /// </summary>
+        public int MaxWidth {
+            get => _maxSize.X;
+            protected set => SetProperty(ref _maxSize.X, value, true);
+        }
+
+        /// <summary>
+        /// Maximum Height the <see cref="Container"/> can autoresize to.
+        /// </summary>
+        public int MaxHeight{
+            get => _maxSize.Y;
+            protected set => SetProperty(ref _maxSize.Y, value, true);
+        }


It currently doesn't make any sense to have all three of these. Setting MaxWidth or MaxHeight on its own would result in a control that has either a 0 width or a 0 height since the check below only compares it against Point.Zero and doesn't check the individual dimensions.

You're setting the value on the structs X and Y values which I don't think is a good idea. These would need to create a new Point like we do in the Control Size shortcuts (Width and Height properties).

Since sizing modes are available for both width and height individually, it seems reasonable to me to just have MaxWidth and MaxHeight. No need for MaxSize. They can be kept in separate ints anyways. At no point do we actually use them as a point so it's just an unnecessary data type holding something we can store directly.

The description helps to clear things up, but it'd probably be best to apply the MaxWidth and MaxHeight to the control at all times instead of just when autoresizing. It removes any confusion in the behavior. That is to say that setting Size, for example, should also be limited by the MaxWidth and MaxHeight.

dlamkins · 2022-09-07T22:23:03Z

Blish HUD/Controls/Container.cs

@@ -118,6 +118,60 @@ public abstract class Container : Control, IEnumerable<Control> {
            set => SetProperty(ref _autoSizePadding, value);
        }

+        protected Scrollbar _panelScrollbar;


Anything for the scrollbar itself should be in the Panel control. Panel is the highest level control that has implemented the scrollbar and I don't see a reason to move its implementation up into the Container. Containers are intended to be fairly barebones.

If an implementer wishes to share this functionality then they can implement the Panel control as they do now.

KenediaDev · 2022-09-08T04:16:24Z

Oh well... Tbh then we might discard the whole PR. We've talked about it to implement it as a eventually temporary solution to the scrollbar overlapping the content, as I did not felt comfortable to adjust the scrollbar to "not be a control anymore".

The reason it is implemented as it is, is that it has to be within the control bounds to not be cut off.

Sizes had to update in order to adjust properly to window size if a window is resizeable.

As I've said, if this is not a solution you want to see implemented we can discard it, i will implement custom classes for my stuff until there is a fix.

I might look into it again, but before I do I really need to know how your solution has to look, but at that point it might be easier/better to let you deal with it.

sonarcloud · 2023-08-29T09:28:08Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

Scrollbar Panel Fix

0a3ae42

KenediaDev mentioned this pull request Sep 6, 2022

Panel scrollbar update #726

Closed

dlamkins requested changes Sep 7, 2022

View reviewed changes

Merge branch 'blish-hud:dev' into dev

021fd08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Panel Scrollbar Update (clean) #749

Panel Scrollbar Update (clean) #749

KenediaDev commented Sep 6, 2022 •

edited

Loading

dlamkins left a comment

dlamkins Sep 7, 2022

dlamkins Sep 7, 2022

dlamkins Sep 7, 2022

dlamkins Sep 7, 2022

dlamkins Sep 7, 2022

dlamkins Sep 7, 2022

dlamkins Sep 7, 2022

dlamkins Sep 7, 2022

dlamkins Sep 7, 2022

KenediaDev commented Sep 8, 2022

sonarcloud bot commented Aug 29, 2023

		//get => _panelScrollbar != null && _panelScrollbar.Drawn;
		//get => _panelScrollbar != null;

	if (_showTint) {
	spriteBatch.DrawOnCtrl(this,
	ContentService.Textures.Pixel,
	this.ContentRegion,
	Color.Black * 0.4f);
	}

		var cR = ContentRegion.ToBounds(this.AbsoluteBounds);
		var contentScissor = Rectangle.Intersect(scissor, cR);

Panel Scrollbar Update (clean) #749

Are you sure you want to change the base?

Panel Scrollbar Update (clean) #749

Conversation

KenediaDev commented Sep 6, 2022 • edited Loading

Container.cs

Scrollbar.cs

Panel.cs

FlowPanel.cs

Control.cs

dlamkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KenediaDev commented Sep 8, 2022

sonarcloud bot commented Aug 29, 2023

KenediaDev commented Sep 6, 2022 •

edited

Loading