From 6a8023cf8b1c969493f07da1b2816e9dd033e422 Mon Sep 17 00:00:00 2001 From: Leon Friedrich <60421075+ElectroJr@users.noreply.github.com> Date: Sun, 5 Nov 2023 02:58:26 +1100 Subject: [PATCH] Fix admin verb PVS issue (#21406) --- .../CustomControls/PlayerListControl.xaml.cs | 2 +- .../UI/Tabs/PlayerTab/PlayerTab.xaml.cs | 12 +++--- .../Systems/Admin/AdminUIController.cs | 2 +- .../Verbs/UI/VerbMenuUIController.cs | 28 +++++++++++--- Content.Client/Verbs/VerbSystem.cs | 37 ++++++++++++------- Content.Shared/Verbs/SharedVerbSystem.cs | 9 +++-- 6 files changed, 59 insertions(+), 31 deletions(-) diff --git a/Content.Client/Administration/UI/CustomControls/PlayerListControl.xaml.cs b/Content.Client/Administration/UI/CustomControls/PlayerListControl.xaml.cs index 5fa28bf1ac..39749f8ac6 100644 --- a/Content.Client/Administration/UI/CustomControls/PlayerListControl.xaml.cs +++ b/Content.Client/Administration/UI/CustomControls/PlayerListControl.xaml.cs @@ -60,7 +60,7 @@ namespace Content.Client.Administration.UI.CustomControls } else if (args.Event.Function == EngineKeyFunctions.UseSecondary && selectedPlayer.NetEntity != null) { - _uiManager.GetUIController().OpenVerbMenu(_entManager.GetEntity(selectedPlayer.NetEntity.Value)); + _uiManager.GetUIController().OpenVerbMenu(selectedPlayer.NetEntity.Value, true); } } diff --git a/Content.Client/Administration/UI/Tabs/PlayerTab/PlayerTab.xaml.cs b/Content.Client/Administration/UI/Tabs/PlayerTab/PlayerTab.xaml.cs index 4f77369b57..1190a4c329 100644 --- a/Content.Client/Administration/UI/Tabs/PlayerTab/PlayerTab.xaml.cs +++ b/Content.Client/Administration/UI/Tabs/PlayerTab/PlayerTab.xaml.cs @@ -14,11 +14,13 @@ namespace Content.Client.Administration.UI.Tabs.PlayerTab [GenerateTypedNameReferences] public sealed partial class PlayerTab : Control { + [Dependency] private readonly IEntityManager _entManager = default!; + [Dependency] private readonly IPlayerManager _playerMan = default!; + private const string ArrowUp = "↑"; private const string ArrowDown = "↓"; private readonly Color _altColor = Color.FromHex("#292B38"); private readonly Color _defaultColor = Color.FromHex("#2F2F3B"); - private IEntityManager _entManager; private readonly AdminSystem _adminSystem; private IReadOnlyList _players = new List(); @@ -30,7 +32,7 @@ namespace Content.Client.Administration.UI.Tabs.PlayerTab public PlayerTab() { - _entManager = IoCManager.Resolve(); + IoCManager.InjectDependencies(this); _adminSystem = _entManager.System(); RobustXamlLoader.Load(this); RefreshPlayerList(_adminSystem.PlayerList); @@ -95,13 +97,11 @@ namespace Content.Client.Administration.UI.Tabs.PlayerTab foreach (var child in PlayerList.Children.ToArray()) { if (child is PlayerTabEntry) - child.Orphan(); + child.Dispose(); } _players = players; - - var playerManager = IoCManager.Resolve(); - PlayerCount.Text = $"Players: {playerManager.PlayerCount}"; + PlayerCount.Text = $"Players: {_playerMan.PlayerCount}"; var sortedPlayers = new List(players); sortedPlayers.Sort(Compare); diff --git a/Content.Client/UserInterface/Systems/Admin/AdminUIController.cs b/Content.Client/UserInterface/Systems/Admin/AdminUIController.cs index 4a7a57e527..acb79cf301 100644 --- a/Content.Client/UserInterface/Systems/Admin/AdminUIController.cs +++ b/Content.Client/UserInterface/Systems/Admin/AdminUIController.cs @@ -187,7 +187,7 @@ public sealed class AdminUIController : UIController, IOnStateEntered CurrentVerbs = new(); /// @@ -64,8 +65,25 @@ namespace Content.Client.Verbs.UI /// public void OpenVerbMenu(EntityUid target, bool force = false, ContextMenuPopup? popup=null) { - if (_playerManager.LocalPlayer?.ControlledEntity is not {Valid: true} user || - _combatMode.IsInCombatMode(user)) + DebugTools.Assert(target.IsValid()); + OpenVerbMenu(EntityManager.GetNetEntity(target), force, popup); + } + + /// + /// Open a verb menu and fill it with verbs applicable to the given target entity. + /// + /// Entity to get verbs on. + /// Used to force showing all verbs. Only works on networked entities if the user is an admin. + /// + /// If this is not null, verbs will be placed into the given popup instead. + /// + public void OpenVerbMenu(NetEntity target, bool force = false, ContextMenuPopup? popup=null) + { + DebugTools.Assert(target.IsValid()); + if (_playerManager.LocalEntity is not {Valid: true} user) + return; + + if (!force && _combatMode.IsInCombatMode(user)) return; Close(); @@ -82,7 +100,7 @@ namespace Content.Client.Verbs.UI // Add indicator that some verbs may be missing. // I long for the day when verbs will all be predicted and this becomes unnecessary. - if (!EntityManager.IsClientSide(target)) + if (!target.IsClientSide()) { _context.AddElement(menu, new ContextMenuElement(Loc.GetString("verb-system-waiting-on-server-text"))); } @@ -248,7 +266,7 @@ namespace Content.Client.Verbs.UI private void HandleVerbsResponse(VerbsResponseEvent msg) { - if (OpenMenu == null || !OpenMenu.Visible || CurrentTarget != EntityManager.GetEntity(msg.Entity)) + if (OpenMenu == null || !OpenMenu.Visible || CurrentTarget != msg.Entity) return; AddServerVerbs(msg.Verbs, OpenMenu); diff --git a/Content.Client/Verbs/VerbSystem.cs b/Content.Client/Verbs/VerbSystem.cs index e71da351b9..34634482c6 100644 --- a/Content.Client/Verbs/VerbSystem.cs +++ b/Content.Client/Verbs/VerbSystem.cs @@ -170,28 +170,27 @@ namespace Content.Client.Verbs /// public SortedSet GetVerbs(EntityUid target, EntityUid user, Type type, bool force = false) { - return GetVerbs(target, user, new List() { type }, force); + return GetVerbs(GetNetEntity(target), user, new List() { type }, force); } /// /// Ask the server to send back a list of server-side verbs, and for now return an incomplete list of verbs /// (only those defined locally). /// - public SortedSet GetVerbs(EntityUid target, EntityUid user, List verbTypes, + public SortedSet GetVerbs(NetEntity target, EntityUid user, List verbTypes, bool force = false) { - if (!IsClientSide(target)) - { - RaiseNetworkEvent(new RequestServerVerbsEvent(GetNetEntity(target), verbTypes, adminRequest: force)); - } + if (!target.IsClientSide()) + RaiseNetworkEvent(new RequestServerVerbsEvent(target, verbTypes, adminRequest: force)); // Some admin menu interactions will try get verbs for entities that have not yet been sent to the player. - if (!Exists(target)) + if (!TryGetEntity(target, out var local)) return new(); - return GetLocalVerbs(target, user, verbTypes, force); + return GetLocalVerbs(local.Value, user, verbTypes, force); } + /// /// Execute actions associated with the given verb. /// @@ -200,8 +199,18 @@ namespace Content.Client.Verbs /// public void ExecuteVerb(EntityUid target, Verb verb) { - var user = _playerManager.LocalPlayer?.ControlledEntity; - if (user == null) + ExecuteVerb(GetNetEntity(target), verb); + } + + /// + /// Execute actions associated with the given verb. + /// + /// + /// Unless this is a client-exclusive verb, this will also tell the server to run the same verb. + /// + public void ExecuteVerb(NetEntity target, Verb verb) + { + if ( _playerManager.LocalEntity is not {} user) return; // is this verb actually valid? @@ -209,16 +218,16 @@ namespace Content.Client.Verbs { // maybe send an informative pop-up message. if (!string.IsNullOrWhiteSpace(verb.Message)) - _popupSystem.PopupEntity(verb.Message, user.Value); + _popupSystem.PopupEntity(verb.Message, user); return; } - if (verb.ClientExclusive || IsClientSide(target)) + if (verb.ClientExclusive || target.IsClientSide()) // is this a client exclusive (gui) verb? - ExecuteVerb(verb, user.Value, target); + ExecuteVerb(verb, user, GetEntity(target)); else - EntityManager.RaisePredictiveEvent(new ExecuteVerbEvent(GetNetEntity(target), verb)); + EntityManager.RaisePredictiveEvent(new ExecuteVerbEvent(target, verb)); } private void HandleVerbResponse(VerbsResponseEvent msg) diff --git a/Content.Shared/Verbs/SharedVerbSystem.cs b/Content.Shared/Verbs/SharedVerbSystem.cs index 40cb2d5002..e6286c47cc 100644 --- a/Content.Shared/Verbs/SharedVerbSystem.cs +++ b/Content.Shared/Verbs/SharedVerbSystem.cs @@ -24,16 +24,17 @@ namespace Content.Shared.Verbs if (user == null) return; - var target = GetEntity(args.Target); + if (!TryGetEntity(args.Target, out var target)) + return; // It is possible that client-side prediction can cause this event to be raised after the target entity has // been deleted. So we need to check that the entity still exists. - if (Deleted(target) || Deleted(user)) + if (Deleted(user)) return; // Get the list of verbs. This effectively also checks that the requested verb is in fact a valid verb that // the user can perform. - var verbs = GetLocalVerbs(target, user.Value, args.RequestedVerb.GetType()); + var verbs = GetLocalVerbs(target.Value, user.Value, args.RequestedVerb.GetType()); // Note that GetLocalVerbs might waste time checking & preparing unrelated verbs even though we know // precisely which one we want to run. However, MOST entities will only have 1 or 2 verbs of a given type. @@ -41,7 +42,7 @@ namespace Content.Shared.Verbs // Find the requested verb. if (verbs.TryGetValue(args.RequestedVerb, out var verb)) - ExecuteVerb(verb, user.Value, target); + ExecuteVerb(verb, user.Value, target.Value); } ///