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.

Sign up for GitHub

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

Jump to bottom

Panel Scrollbar Update (clean) #749

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Panel Scrollbar Update (clean) #749

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a3ae42
Scrollbar Panel Fix
KenediaDev Sep 6, 2022
021fd08
Merge branch 'blish-hud:dev' into dev
KenediaDev Aug 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 73 additions & 10 deletions Blish HUD/Controls/Container.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,60 @@
set => SetProperty(ref _autoSizePadding, value);
}

protected Scrollbar _panelScrollbar;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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);
}
Comment on lines +129 to +136
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/// <summary>
/// Indicates whether or not there is a <see cref="Scrollbar"/> drawn inside the <see cref="Panel"/>.
/// </summary>
public bool ScrollbarVisible {
//get => _panelScrollbar != null && _panelScrollbar.Drawn;
//get => _panelScrollbar != null;
Comment on lines +142 to +143
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs cleaned up.

get => _scrollbarVisible;
protected set => SetProperty(ref _scrollbarVisible, value, true);
}
protected bool _scrollbarVisible = false;

/// <summary>
/// Space which is taken from the <see cref="Scrollbar"/> to be drawn inside the <see cref="Panel"/>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// </summary>
public int ScrollbarWidth {
get => _panelScrollbar != null ? (_panelScrollbar.ScrollbarWidth + (int) ScrollPadding.X) : 0;
}

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);
}
Comment on lines +156 to +179
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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.

  2. 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).

  3. 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.

  4. 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.


protected override CaptureType CapturesInput() => CaptureType.Mouse | CaptureType.MouseWheel;

/// <summary>
Expand Down Expand Up @@ -162,6 +216,7 @@
/// </summary>
public bool AddChild(Control child) {
if (_children.Contains(child)) return true;
if (_panelScrollbar != null && child == _panelScrollbar) return true;

var resultingChildren = _children.ToList();
resultingChildren.Add(child);
Expand Down Expand Up @@ -253,35 +308,40 @@
}
}

public sealed override void DoUpdate(GameTime gameTime) {
UpdateContainer(gameTime);

Control[] children = _children.ToArray();
var filteredChildren = children.Where(c => c.GetType() != typeof(Scrollbar) && c.Visible).ToArray();

_contentBounds = ControlUtil.GetControlBounds(filteredChildren);

_contentBounds = ControlUtil.GetControlBounds(children);
var limitSize = _maxSize != Point.Zero;

// Update our size based on the sizing mode
var parent = this.Parent;
if (parent != null) {
if (parent != null) {
this.Size = new Point(GetUpdatedSizing(this.WidthSizingMode,
this.Width,
_contentBounds.X + (this.Width - this.ContentRegion.Width) + _autoSizePadding.X,
parent.ContentRegion.Width - this.Left),
limitSize ? Math.Min(_maxSize.X, this.Width) : this.Width,
limitSize ? Math.Min(_maxSize.X, _contentBounds.X + (this.Width - this.ContentRegion.Width) + _autoSizePadding.X) : _contentBounds.X + (this.Width - this.ContentRegion.Width) + _autoSizePadding.X,
limitSize ? Math.Min(_maxSize.X, parent.ContentRegion.Width - this.Left) : parent.ContentRegion.Width - this.Left),
GetUpdatedSizing(this.HeightSizingMode,
this.Height,
_contentBounds.Y + (this.Height - this.ContentRegion.Height) + _autoSizePadding.Y,
parent.ContentRegion.Height - this.Top));
limitSize ? Math.Min(_maxSize.Y, this.Height) : this.Height,
limitSize ? Math.Min(_maxSize.Y, _contentBounds.Y + (this.Height - this.ContentRegion.Height) + _autoSizePadding.Y) : _contentBounds.Y + (this.Height - this.ContentRegion.Height) + _autoSizePadding.Y,
limitSize ? Math.Min(_maxSize.Y, parent.ContentRegion.Height - this.Top) : parent.ContentRegion.Height - this.Top));

ScrollbarVisible = _contentBounds.Y > this.Height;
}

// Update our children
foreach (var childControl in children) {
// Update child if it is visible or if it hasn't rendered yet (needs a first time calc)
if (childControl.Visible || childControl.LayoutState != LayoutState.Ready) {
childControl.Update(gameTime);
}
}
}

Check notice on line 344 in Blish HUD/Controls/Container.cs

View check run for this annotation

codefactor.io / CodeFactor

Blish HUD/Controls/Container.cs#L311-L344 

Complex Method
protected sealed override void Paint(SpriteBatch spriteBatch, Rectangle bounds) {
var controlScissor = spriteBatch.GraphicsDevice.ScissorRectangle.ScaleBy(1 / Graphics.UIScaleMultiplier);

Expand All @@ -303,8 +363,9 @@
public virtual void PaintBeforeChildren(SpriteBatch spriteBatch, Rectangle bounds) { /* NOOP */ }

protected void PaintChildren(SpriteBatch spriteBatch, Rectangle bounds, Rectangle scissor) {
var contentScissor = Rectangle.Intersect(scissor, ContentRegion.ToBounds(this.AbsoluteBounds));

var cR = ContentRegion.ToBounds(this.AbsoluteBounds);
var contentScissor = Rectangle.Intersect(scissor, cR);
Comment on lines +366 to +367
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this change for?


var zSortedChildren = _children.ToArray().OrderBy(i => i.ZIndex);

// Render each visible child
Expand All @@ -326,6 +387,8 @@
descendant.Dispose();
}

_panelScrollbar?.Dispose();

base.DisposeControl();
}

Expand Down
8 changes: 7 additions & 1 deletion Blish HUD/Controls/Control.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -662,6 +662,12 @@ public bool Enabled {
set => SetProperty(ref _enabled, value);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}


protected Color _backgroundColor = Color.Transparent;
/// <summary>
/// The <see cref="Color"/> rendered behind the <see cref="Control"/>.
Expand Down Expand Up @@ -904,7 +910,7 @@ public virtual void Draw(SpriteBatch spriteBatch, Rectangle drawBounds, Rectangl

// Draw background
if (_backgroundColor != Color.Transparent)
spriteBatch.DrawOnCtrl(this, ContentService.Textures.Pixel, drawBounds, _backgroundColor);
spriteBatch.DrawOnCtrl(this, ContentService.Textures.Pixel, _backgroundColorBounds == Rectangle.Empty ? drawBounds : _backgroundColorBounds, _backgroundColor);

if (!this.ClipsBounds) {
spriteBatch.GraphicsDevice.ScissorRectangle = Graphics.SpriteScreen.LocalBounds.ScaleBy(Graphics.UIScaleMultiplier);
Expand Down
10 changes: 5 additions & 5 deletions Blish HUD/Controls/FlowPanel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ private void ReflowChildLayoutLeftToRight(IEnumerable<Control> allChildren) {

foreach (var child in allChildren.Where(c => c.Visible)) {
// Need to flow over to the next row
if (child.Width >= this.Width - lastRight) {
if (child.Width >= this.ContentRegion.Width - lastRight) {
currentBottom = nextBottom + _controlPadding.Y;
lastRight = outerPadX;
}
Expand All @@ -192,13 +192,13 @@ private void ReflowChildLayoutRightToLeft(IEnumerable<Control> allChildren) {

float nextBottom = outerPadY;
float currentBottom = outerPadY;
float lastLeft = this.Width - outerPadX;
float lastLeft = this.ContentRegion.Width - outerPadX;

foreach (var child in allChildren.Where(c => c.Visible)) {
// Need to flow over to the next row
if (outerPadX > lastLeft - child.Width) {
currentBottom = nextBottom + _controlPadding.Y;
lastLeft = this.Width - outerPadX;
lastLeft = this.ContentRegion.Width - outerPadX;
}

child.Location = new Point((int) (lastLeft - child.Width), (int) currentBottom);
Expand Down Expand Up @@ -275,7 +275,7 @@ private void ReflowChildLayoutSingleRightToLeft(IEnumerable<Control> allChildren
float outerPadX = _padLeftBeforeControl ? _controlPadding.X : _outerControlPadding.X;
float outerPadY = _padTopBeforeControl ? _controlPadding.Y : _outerControlPadding.Y;

var lastLeft = this.Width - outerPadX;
var lastLeft = this.ContentRegion.Width - outerPadX;

foreach (var child in allChildren) {
child.Location = new Point((int) (lastLeft - child.Width), (int) outerPadY);
Expand Down Expand Up @@ -350,4 +350,4 @@ protected override void DisposeControl() {
}

}
}
}
30 changes: 20 additions & 10 deletions Blish HUD/Controls/Panel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@ public override SizingMode HeightSizingMode {
[JsonIgnore] public float AccentOpacity { get; set; } = 1f;

private Glide.Tween _collapseAnim;
private Scrollbar _panelScrollbar;

/// <inheritdoc />
public bool ToggleAccordionState() {
Expand Down Expand Up @@ -233,7 +232,7 @@ public override void RecalculateLayout() {
int cornerAccentWidth = Math.Min(_size.X, MAX_ACCENT_WIDTH);
_layoutTopLeftAccentBounds = new Rectangle(-2, topOffset - 12, cornerAccentWidth, _textureCornerAccent.Height);

_layoutBottomRightAccentBounds = new Rectangle(_size.X - cornerAccentWidth + 2, _size.Y - 59, cornerAccentWidth, _textureCornerAccent.Height);
_layoutBottomRightAccentBounds = new Rectangle(_size.X - cornerAccentWidth, _size.Y - 59, cornerAccentWidth, _textureCornerAccent.Height);

_layoutCornerAccentSrc = new Rectangle(MAX_ACCENT_WIDTH - cornerAccentWidth, 0, cornerAccentWidth, _textureCornerAccent.Height);

Expand All @@ -242,34 +241,47 @@ public override void RecalculateLayout() {
_layoutLeftAccentSrc = new Rectangle(0, 0, _textureLeftSideAccent.Width, _layoutLeftAccentBounds.Height);
}

ScrollbarVisible = _contentBounds.Y > (_size.Y - topOffset - bottomOffset);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

_size.Y - topOffset - bottomOffset);

_layoutHeaderBounds = new Rectangle(this.ContentRegion.Left, 0, this.ContentRegion.Width, HEADER_HEIGHT);
_layoutHeaderBounds = new Rectangle(this.ContentRegion.Left, 0, this.ContentRegion.Width, HEADER_HEIGHT);
_layoutHeaderTextBounds = new Rectangle(_layoutHeaderBounds.Left + 10, 0, _layoutHeaderBounds.Width - 10, HEADER_HEIGHT);

_layoutAccordionArrowOrigin = new Vector2((float)ARROW_SIZE / 2, (float)ARROW_SIZE / 2);
_layoutAccordionArrowBounds = new Rectangle(_layoutHeaderBounds.Right - ARROW_SIZE,
(topOffset - ARROW_SIZE) / 2,
ARROW_SIZE,
ARROW_SIZE).OffsetBy(_layoutAccordionArrowOrigin.ToPoint());

_backgroundColorBounds = new Rectangle(leftOffset,
topOffset,
_size.X - leftOffset - rightOffset - 2,
_size.Y - topOffset - bottomOffset);

if (_panelScrollbar != null) {
_panelScrollbar.Height = this.ContentRegion.Height - 20;
_panelScrollbar.Right = this.Right;
_panelScrollbar.Top = this.Top + this.ContentRegion.Top + 10;
}
}

private void UpdateScrollbar() {
/* TODO: Fix .CanScroll: currently you have to set it after you set other region changing settings for it
to work correctly */
if (this.CanScroll) {
if (_panelScrollbar == null)
if (_panelScrollbar == null)
_panelScrollbar = new Scrollbar(this);

this.PropertyChanged -= UpdatePanelScrollbarOnOwnPropertyChanged;
this.PropertyChanged += UpdatePanelScrollbarOnOwnPropertyChanged;

_panelScrollbar.Parent = this.Parent;
_panelScrollbar.Height = this.ContentRegion.Height - 20;
_panelScrollbar.Right = this.Right - _panelScrollbar.Width / 2;
_panelScrollbar.Right = this.Right;
_panelScrollbar.Top = this.Top + this.ContentRegion.Top + 10;
_panelScrollbar.Visible = this.Visible;
_panelScrollbar.ZIndex = this.ZIndex + 2;
Expand All @@ -291,7 +303,7 @@ private void UpdatePanelScrollbarOnOwnPropertyChanged(object? sender, PropertyCh
_panelScrollbar.Height = this.ContentRegion.Height - 20;
break;
case "Right":
_panelScrollbar.Right = this.Right - _panelScrollbar.Width / 2;
_panelScrollbar.Right = this.Right;
break;
case "Top":
_panelScrollbar.Top = this.Top + this.ContentRegion.Top + 10;
Expand Down Expand Up @@ -389,8 +401,6 @@ public override void PaintBeforeChildren(SpriteBatch spriteBatch, Rectangle boun
}

protected override void DisposeControl() {
_panelScrollbar?.Dispose();

foreach (var control in this._children) {
control.Resized -= UpdateContentRegionBounds;
control.Moved -= UpdateContentRegionBounds;
Expand All @@ -400,4 +410,4 @@ protected override void DisposeControl() {
}

}
}
}
18 changes: 17 additions & 1 deletion Blish HUD/Controls/Scrollbar.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,22 @@ private int ScrollbarHeight {

private double _scrollbarPercent = 1.0;

/// <summary>
/// See if the <see cref="Scrollbar"/> is beeing drawn.
/// </summary>
public bool Drawn
{
get => Visible && _scrollbarPercent < 0.99;
}
Comment on lines +94 to +100
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// <summary>
/// Width of the <see cref="Scrollbar"/>.
/// </summary>
public int ScrollbarWidth
{
get => _barBounds.Width;
}

private Container _associatedContainer;
public Container AssociatedContainer {
get => _associatedContainer;
Expand Down Expand Up @@ -298,4 +314,4 @@ protected override void Paint(SpriteBatch spriteBatch, Rectangle bounds) {
}

}
}
}