Skip to content

fix: Admin Client disconnect and closed when ban or mute a player #2783

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

Closed
Sh4d0v wants to merge 2 commits into from
Closed

fix: Admin Client disconnect and closed when ban or mute a player #2783

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
559d9b5
fix: Admin Client disconnect and closed when ban or mute a player
Sh4d0v Dec 23, 2025
2e1d33f
Apply suggestions from code review
Sh4d0v Dec 23, 2025
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
86 changes: 39 additions & 47 deletions Intersect.Client.Core/Interface/Game/Admin/BanMuteBox.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,47 +8,23 @@ namespace Intersect.Client.Interface.Game.Admin;
public partial class BanMuteBox : WindowControl
{
private readonly TextBox _textboxReason;
private readonly ComboBox _comboboxDuration;
private readonly ComboBox? _comboboxDuration;
Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

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

The _comboboxDuration field is marked as nullable (ComboBox?), but it's initialized in the constructor on line 62 and should never be null during the object's lifetime. Making it nullable adds unnecessary null-checking overhead (lines 94, 119) without providing value. Consider making this field non-nullable to better reflect its actual behavior.

Copilot uses AI. Check for mistakes.
private readonly LabeledCheckBox _checkboxIP;

private readonly Dictionary<string, int> _dayCount = new()
private static readonly List<(string Label, int Days)> _durationOptions = new()
{
{
Strings.BanMute.OneDay, 1
},
{
Strings.BanMute.TwoDays, 2
},
{
Strings.BanMute.ThreeDays, 3
},
{
Strings.BanMute.FourDays, 4
},
{
Strings.BanMute.FiveDays, 5
},
{
Strings.BanMute.OneWeek, 7
},
{
Strings.BanMute.TwoWeeks, 14
},
{
Strings.BanMute.OneMonth, 30
},
{
Strings.BanMute.TwoMonths, 60
},
{
Strings.BanMute.SixMonths, 180
},
{
Strings.BanMute.OneYear, 365
},
{
Strings.BanMute.Forever, 999999
},
(Strings.BanMute.OneDay, 1),
(Strings.BanMute.TwoDays, 2),
(Strings.BanMute.ThreeDays, 3),
(Strings.BanMute.FourDays, 4),
(Strings.BanMute.FiveDays, 5),
(Strings.BanMute.OneWeek, 7),
(Strings.BanMute.TwoWeeks, 14),
(Strings.BanMute.OneMonth, 30),
(Strings.BanMute.TwoMonths, 60),
(Strings.BanMute.SixMonths, 180),
(Strings.BanMute.OneYear, 365),
(Strings.BanMute.Forever, 999999),
};

public BanMuteBox(string title, string prompt, EventHandler okayHandler) : base(
Expand Down Expand Up @@ -84,9 +60,9 @@ public BanMuteBox(string title, string prompt, EventHandler okayHandler) : base(

// Duration combobox
_comboboxDuration = new ComboBox(this, "ComboBoxDuration");
foreach (var day in _dayCount)
foreach (var option in _durationOptions)
{
_ = _comboboxDuration.AddItem(day.Key, userData: day.Value);
_ = _comboboxDuration.AddItem(option.Label, userData: option.Days);
}

// Include IP checkbox
Expand All @@ -103,17 +79,23 @@ public BanMuteBox(string title, string prompt, EventHandler okayHandler) : base(
buttonOkay.Clicked += (s, e) =>
{
okayHandler?.Invoke(this, EventArgs.Empty);
Dispose();
Close();
Comment on lines 81 to +82
Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

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

The Okay button now calls Close() instead of Dispose(), but AdminWindow.cs still explicitly calls Dispose() on the _banOrMuteWindow after the okayHandler is executed (lines 481 and 512 in AdminWindow.cs). This creates a double-disposal scenario: Close() is called first (line 82), followed by an explicit Dispose() call. Even though the Dispose() method now checks for _disposed and returns early, this pattern is confusing and could lead to issues if DeleteOnClose is set to true on this window (which would cause automatic disposal on Close()). Consider removing the explicit Dispose() calls in AdminWindow.cs.

Copilot uses AI. Check for mistakes.
};

var buttonCancel = new Button(this, "ButtonCancel")
{
Text = Strings.BanMute.Cancel,
};
buttonCancel.Clicked += (s, e) => Dispose();
buttonCancel.Clicked += (s, e) => Close();

LoadJsonUi(UI.InGame, Graphics.Renderer?.GetResolutionString(), true);

// Set the first element by default after loading the UI (ensures deterministic selection).
if (_comboboxDuration != null && _durationOptions.Count > 0)
{
_ = _comboboxDuration.SelectByUserData(_durationOptions[0].Days);
}

richLabelPrompt.ClearText();
richLabelPrompt.Width = promptContainer.Width - promptContainer.VerticalScrollBar.Width;
richLabelPrompt.AddText(prompt, labelPrompt);
Expand All @@ -122,24 +104,34 @@ public BanMuteBox(string title, string prompt, EventHandler okayHandler) : base(

protected override void Dispose(bool disposing)
{
Close();
Interface.GameUi.GameCanvas.RemoveChild(this, false);
Interface.InputBlockingComponents.Remove(this);

if (_textboxReason != null)
{
Interface.FocusComponents.Remove(_textboxReason);
}

Comment on lines +108 to 113
Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

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

The null-check for _textboxReason is unnecessary. The field is initialized in the constructor (line 52) and is not nullable, so it will never be null at this point in the Dispose method. This defensive check adds complexity without providing value.

Suggested change
if (_textboxReason != null)
{
Interface.FocusComponents.Remove(_textboxReason);
}
Interface.FocusComponents.Remove(_textboxReason);

Copilot uses AI. Check for mistakes.
base.Dispose(disposing);
}

public int GetDuration()
{
return (int)_comboboxDuration.SelectedItem.UserData;
var selected = _comboboxDuration?.SelectedItem;

if (selected?.UserData is int days)
{
return days;
}
return 1; // default value should be 1 day for safety
}

public string GetReason()
{
return _textboxReason.Text;
return _textboxReason?.Text ?? string.Empty;
}

public bool BanIp()
{
return _checkboxIP.IsChecked;
return _checkboxIP?.IsChecked ?? false;
}
}
55 changes: 34 additions & 21 deletions Intersect.Client.Framework/Gwen/Control/Base.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1295,23 +1295,27 @@ private static void PropagateDrawDebugOutlinesToChildren(Base @this, bool drawDe
/// </summary>
public void Dispose()
{
try
// TODO : GWEN: We need to spend more time on this.
if (_disposed)
{
ObjectDisposedException.ThrowIf(_disposed, this);
}
catch
{
throw;
return;
Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

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

The original code used ObjectDisposedException.ThrowIf to throw an exception if the object was already disposed. The new code silently returns early instead. This changes the behavior from failing loudly (which helps catch double-disposal bugs during development) to failing silently. Consider whether this is the desired behavior change, as the original pattern helped identify misuse of disposed objects. If the goal is to prevent exceptions, document why silent failure is acceptable in this case.

Suggested change
return;
throw new ObjectDisposedException(GetType().FullName);

Copilot uses AI. Check for mistakes.
}

// This stack trace is only used for diagnostics, so it is included only in DEBUG builds.
#if DEBUG
_disposeStack = new StackTrace(fNeedFileInfo: true);
#endif

// We mark that Dispose() has been called to prevent Dispose from being executed again
_disposed = true;

// Remove all pending tasks from the thread queue of this control
_threadQueue.ClearPending();

// Virtual Dispose() so that inherited classes can release their resources
Dispose(disposing: true);

// Reserve a temporary variable for the texture caching mechanism
ICacheToTexture? cache = default;

#pragma warning disable CA1031 // Do not catch general exception types
Expand All @@ -1325,34 +1329,35 @@ public void Dispose()
}
#pragma warning restore CA1031 // Do not catch general exception types

// If the control was cached in the texture cache, free that memory
if (ShouldCacheToTexture && cache != null)
{
cache.DisposeCachedTexture(this);
}

// Clear input indicators if they point to this control
if (InputHandler.HoveredControl == this)
{
InputHandler.HoveredControl = null;
}

if (InputHandler.KeyboardFocus == this)
{
InputHandler.KeyboardFocus = null;
}

if (InputHandler.MouseFocus == this)
{
InputHandler.MouseFocus = null;
Comment on lines 1339 to 1344
Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

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

The braces were removed from these if statements, making the code more compact but potentially less maintainable. While this follows some coding styles, it can increase the risk of bugs if additional statements are later added without re-adding braces. Consider whether consistency with the project's style guide is maintained.

Copilot uses AI. Check for mistakes.
}

DragAndDrop.ControlDeleted(this);
ToolTip.ControlDeleted(this);
Animation.Cancel(this);

// Remove and release all child controls
DisposeChildrenOf(this);

// TODO : check if we need to dispose tooltip
// if (_tooltip != null)
// _tooltip.Dispose();

// Prevents the finalizer from restarting for this object
GC.SuppressFinalize(this);

// Informs that the object has completed the resource release process
_disposeCompleted = true;

try
Expand All @@ -1370,6 +1375,14 @@ public void Dispose()
}
}

/// <summary>
/// Releases resources used by the control.
/// When <paramref name="disposing"/> is true, both managed and unmanaged resources
/// should be released. When false (finalizer), only unmanaged resources should be released.
/// Override this method in derived classes to dispose additional resources and ensure
/// to call the base implementation.
/// </summary>
/// <param name="disposing">True when called from <see cref="Dispose()"/>, false when called from finalizer.</param>
protected virtual void Dispose(bool disposing)
{
}
Expand Down Expand Up @@ -1591,7 +1604,7 @@ public void LoadJsonUi(GameContentManager.UI stage, string? resolution = default
}
catch (Exception exception)
{
//Log JSON UI Loading Error
// Log JSON UI Loading Error
throw new Exception("Error loading json ui for " + ParentQualifiedName, exception);
}
}
Expand Down Expand Up @@ -1717,30 +1730,30 @@ public virtual void LoadJson(JToken token, bool isRoot = default)

if (obj["DrawBackground"] != null)
{
ShouldDrawBackground = (bool) obj["DrawBackground"];
ShouldDrawBackground = (bool)obj["DrawBackground"];
}

if (obj["Disabled"] != null)
{
IsDisabled = (bool) obj["Disabled"];
IsDisabled = (bool)obj["Disabled"];
}

if (obj["Hidden"] != null)
{
IsHidden = (bool) obj["Hidden"];
IsHidden = (bool)obj["Hidden"];
}

if (obj[nameof(RestrictToParent)] != null)
{
RestrictToParent = (bool) obj[nameof(RestrictToParent)];
RestrictToParent = (bool)obj[nameof(RestrictToParent)];
}

if (obj[nameof(MouseInputEnabled)] != null)
{
MouseInputEnabled = (bool) obj[nameof(MouseInputEnabled)];
MouseInputEnabled = (bool)obj[nameof(MouseInputEnabled)];
}

if (obj["HideToolTip"] != null && (bool) obj["HideToolTip"])
if (obj["HideToolTip"] != null && (bool)obj["HideToolTip"])
{
_tooltipEnabled = false;
SetToolTipText(null);
Expand Down
Loading