Repository navigation
Xml documentation comments imported into View Variables #5430
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
base: master
Are you sure you want to change the base?
Changes from all commits
1343168
5e745f2
457671d
26a7115
a5a4fdc
349c431
2aa841d
732305a
3637c6e
a19b647
0fda1a7
7d4cd92
cd2e65f
e23bbbd
2a80d50
887f63f
7a62c0a
64226ce
63fecf8
dc6b30b
e87debc
0c4376e
727ecf9
967d33e
77d6a0d
d076244
dd940cc
4c746b7
ac39f18
32a241e
a346ec0
fb95556
90790c4
780483a
5acd325
eff08b7
09d86d1
60548f7
a9a719f
e0b8f03
f06dcd4
8e21e9e
7f5f768
8521bfa
0a76011
da51c88
029de73
e7c4f12
29df604
a059e3d
2801d4c
3ebc0a8
ed75c57
e3b47ad
746714a
8e669fa
c9bf109
f9b390e
da7ce79
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| <!-- | ||
| This is the MSBuild file that drives the creation of xml docs for use by View Variables. | ||
|
|
||
| Normally this is invoked from `Robust.Properties.targets` when a csproj defines `<ViewVariables>` | ||
| in its `<PropertyGroup>`. | ||
|
|
||
| The workflow is: | ||
|
|
||
| 0. Ensure the XmlDocTool is built before any project that uses this system | ||
| 1. Make sure the doc folder exists: | ||
| - `..\Resources\ViewVariables\` for client and shared code, which is shipped to clients. | ||
| - `..\Resources\Server\ViewVariables\` for server code, which only the server can see. | ||
| The server hands these docs to clients that are allowed to VV, on demand. | ||
| 2. Get MSBuild to generate the xml doc at the path from step 1. | ||
| 3. Run XmlDocTool on the generated doc | ||
|
|
||
| Step two avoids using `<DocumentationFile>` due to a weird quirk of how that facility | ||
| works. DocumentationFile always causes two copies of the generated xml file to be | ||
| created, one at the path in DocumentationFile and one next to the DLL. This script | ||
| uses the somewhat arcane facility `<DocFileItem>` to side-step that problem and create | ||
| a single copy of the xml docs in the folder we want. | ||
|
|
||
| Project files (.csproj), inside or outside Robust Toolbox, opt in with: | ||
|
|
||
| ``` | ||
| <PropertyGroup> | ||
| <ViewVariables>true</ViewVariables> | ||
| </PropertyGroup> | ||
| ``` | ||
|
|
||
| Projects whose name ends in `.Server` are treated as server-only automatically. | ||
| Override that with `<ViewVariablesServerOnly>true|false</ViewVariablesServerOnly>`. | ||
|
|
||
| see also | ||
| github.com/dotnet/msbuild/issues/1559 | ||
| --> | ||
| <Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | ||
| <PropertyGroup> | ||
| <!-- Robust Toolbox root, found relative to this file so it works from content projects too --> | ||
| <VVRobustDir>$([MSBuild]::NormalizeDirectory('$(MSBuildThisFileDirectory)', '..'))</VVRobustDir> | ||
| <!-- Doc generation is only enabled to feed VV, we don't require every public member to be documented --> | ||
| <NoWarn>$(NoWarn);CS1591;CS1573</NoWarn> | ||
| <!-- Server code docs must not ship to clients, they go in the server-only resources folder --> | ||
| <ViewVariablesServerOnly Condition="'$(ViewVariablesServerOnly)' == '' And $(MSBuildProjectName.EndsWith('.Server'))">true</ViewVariablesServerOnly> | ||
| <VVDocDir Condition="'$(ViewVariablesServerOnly)' == 'true'">..\Resources\Server\ViewVariables\</VVDocDir> | ||
| <VVDocDir Condition="'$(ViewVariablesServerOnly)' != 'true'">..\Resources\ViewVariables\</VVDocDir> | ||
| <VVDocPath>$(VVDocDir)$(MSBuildProjectName).xml</VVDocPath> | ||
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <!-- Add a reference to XmlDocTool so it always gets built before any project that needs it --> | ||
| <ProjectReference Include="$(VVRobustDir)Robust.XmlDocTool\Robust.XmlDocTool.csproj"> | ||
| <!-- Don't copy the build artifacts from this project to the referencing project's output folder --> | ||
| <Private>false</Private> | ||
| <!-- Indicate that the referencing project isn't actually meant to reference this project's assembly directly --> | ||
| <ReferenceOutputAssembly>false</ReferenceOutputAssembly> | ||
| <!-- Build-time tools run on the host, not the target runtime --> | ||
| <GlobalPropertiesToRemove>RuntimeIdentifier;RestoreRuntimeIdentifier</GlobalPropertiesToRemove> | ||
| </ProjectReference> | ||
| </ItemGroup> | ||
| <Target Name="GenerateXmlDoc" BeforeTargets="CoreCompile"> | ||
| <!-- Ensure the destination folder for the docs exists --> | ||
| <MakeDir Directories="$(VVDocDir)"/> | ||
| <ItemGroup> | ||
| <!-- Slightly arcane invocation of the MSBuild doc generation facility that bypasses the issue explained above --> | ||
| <DocFileItem Include="$(VVDocPath)" /> | ||
|
Mervill marked this conversation as resolved.
Resolved
|
||
| </ItemGroup> | ||
| </Target> | ||
| <Target Name="TrimXmlDoc" AfterTargets="PostBuildEvent"> | ||
| <!-- Run XmlDocTool on the generated doc file --> | ||
| <!-- Run the dll through the dotnet host rather than relying on a platform-specific apphost existing --> | ||
| <Exec Command=""$(DOTNET_HOST_PATH)" "$([MSBuild]::NormalizePath('$(VVRobustDir)', 'bin', 'XmlDocTool', 'Robust.XmlDocTool.dll'))" "$(VVDocPath)"" /> | ||
| </Target> | ||
| </Project> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| /Resources/Server/ | ||
|
|
||
| Files in this folder are not shipped to the client, and are thus visible only to the server. | ||
|
|
||
| See `RobustClientPackaging.ClientIgnoredResources` for refrence. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,194 @@ | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Threading.Tasks; | ||
| using Robust.Shared.Network; | ||
| using Robust.Shared.Network.Messages; | ||
| using Robust.Shared.ViewVariables; | ||
|
|
||
| namespace Robust.Client.ViewVariables | ||
| { | ||
| internal sealed partial class ClientViewVariablesManager | ||
| { | ||
| // The client only ever sees the shared docs, server docs are requested from the server. | ||
| private ViewVariablesDocStrings? _docStrings = null; | ||
|
|
||
| // DocStrings we already asked the server for. null means the server didn't have one. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or it means that user asked for them without having proper permission |
||
| // Cleared on disconnect because a different server may have different code. | ||
| private readonly Dictionary<(ViewVariablesDocStringKind, string), string?> _remoteDocStrings = new(); | ||
|
|
||
| private readonly Dictionary<uint, TaskCompletionSource<string?[]>> _requestedDocStrings = new(); | ||
|
|
||
| public void LoadDocStrings() | ||
| { | ||
| _docStrings = new(Sawmill, ViewVariablesDocStrings.SharedRoot); | ||
| _docStrings.EnsureLoaded(_resManager); | ||
| } | ||
|
|
||
| public string GetDocStringForType(Type type) | ||
| { | ||
| return GetDocStringForType(DocStringKeyForType(type)); | ||
| } | ||
|
|
||
| public string GetDocStringForType(string key) | ||
| { | ||
| return GetLocalDocString(ViewVariablesDocStringKind.Type, key); | ||
| } | ||
|
|
||
| public string GetDocStringForFieldOrProperty(string key) | ||
| { | ||
| return GetLocalDocString(ViewVariablesDocStringKind.FieldOrProperty, key); | ||
| } | ||
|
|
||
| public async Task<string[]> GetDocStringsAsync(ViewVariablesDocStringKind kind, IReadOnlyList<string> keys) | ||
| { | ||
| LoadDocStrings(); | ||
|
|
||
| var result = new string[keys.Count]; | ||
| var missingKeys = new List<string>(); | ||
| var missingIdx = new List<int>(); | ||
|
Comment on lines
+47
to
+48
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could it be better to use HashSet for those? |
||
|
|
||
| for (var i = 0; i < keys.Count; i++) | ||
| { | ||
| var key = keys[i]; | ||
|
|
||
| if (_docStrings!.TryGet(kind, key, out var local)) | ||
| { | ||
| result[i] = local; | ||
| } | ||
| else if (_remoteDocStrings.TryGetValue((kind, key), out var cached)) | ||
| { | ||
| result[i] = cached ?? ViewVariablesDocStrings.Missing(key); | ||
| } | ||
| else | ||
| { | ||
| missingKeys.Add(key); | ||
| missingIdx.Add(i); | ||
| } | ||
| } | ||
|
|
||
| if (missingKeys.Count == 0 || !_netManager.IsConnected) | ||
| { | ||
| foreach (var i in missingIdx) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if missingKeys.Count == 0 , then missingIdx is empty, so that loop would effectively is doing nothing (probably separate early return is better?). And i am a bit confused how _netManager.IsConnected flag is working here... why would we need to do any of this if we are not connected? |
||
| { | ||
| result[i] = ViewVariablesDocStrings.Missing(keys[i]); | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| for (var start = 0; start < missingKeys.Count; start += MsgViewVariablesReqDocStrings.MaxKeys) | ||
| { | ||
| // We may have disconnected while waiting on the previous chunk. | ||
| if (!_netManager.IsConnected) | ||
| { | ||
| for (var k = start; k < missingKeys.Count; k++) | ||
| { | ||
| result[missingIdx[k]] = ViewVariablesDocStrings.Missing(missingKeys[k]); | ||
| } | ||
| break; | ||
| } | ||
|
|
||
| var count = Math.Min(MsgViewVariablesReqDocStrings.MaxKeys, missingKeys.Count - start); | ||
| var chunk = missingKeys.GetRange(start, count); | ||
| var remote = await RequestDocStrings(kind, chunk); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would propose naming it
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having no parallelism here is a bit odd. Should there be? From what i see that should not really be a problem, and doing parallelism here would be a dirty thing in itself, not trivial. Considering the only thing that currently would ask for batches is server-side components... should not be a problem i presume? |
||
|
|
||
| for (var j = 0; j < chunk.Count; j++) | ||
| { | ||
| // A short answer means we never heard back for this key (e.g. we disconnected and the | ||
| // request was finished with an empty array). Don't cache it, or the miss would stick | ||
| // around after the cache was cleared and carry over to the next server. | ||
| if (j >= remote.Length) | ||
| { | ||
| result[missingIdx[start + j]] = ViewVariablesDocStrings.Missing(chunk[j]); | ||
| continue; | ||
| } | ||
|
|
||
| var docString = remote[j]; | ||
| _remoteDocStrings[(kind, chunk[j])] = docString; | ||
| result[missingIdx[start + j]] = docString ?? ViewVariablesDocStrings.Missing(chunk[j]); | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| public async void ApplyDocStringsAsync(ViewVariablesDocStringKind kind, IReadOnlyList<(string Key, Action<string> Apply)> targets) | ||
| { | ||
| if (targets.Count == 0) | ||
| return; | ||
|
|
||
| var keys = new string[targets.Count]; | ||
| for (var i = 0; i < targets.Count; i++) | ||
| { | ||
| keys[i] = targets[i].Key; | ||
| } | ||
|
|
||
| string[] docStrings; | ||
| try | ||
| { | ||
| docStrings = await GetDocStringsAsync(kind, keys); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Sawmill.Error($"Failed to get DocStrings: {e}"); | ||
| return; | ||
| } | ||
|
|
||
| for (var i = 0; i < targets.Count; i++) | ||
| { | ||
| targets[i].Apply(docStrings[i]); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As this can contain basically anything - i would propose a bit of redundancy - wrapping whole thing in try-catch and logging any suspicious stuff. Otherwise async void method will fall silently and you will never know whats going on. |
||
| } | ||
| } | ||
|
|
||
| public static string DocStringKeyForType(Type type) | ||
| { | ||
| return type.AssemblyQualifiedName!.Split(", ")[0]; | ||
| } | ||
|
|
||
| private string GetLocalDocString(ViewVariablesDocStringKind kind, string key) | ||
| { | ||
| LoadDocStrings(); | ||
|
|
||
| return _docStrings!.TryGet(kind, key, out var docString) | ||
| ? docString | ||
| : ViewVariablesDocStrings.Missing(key); | ||
| } | ||
|
|
||
| private Task<string?[]> RequestDocStrings(ViewVariablesDocStringKind kind, List<string> keys) | ||
| { | ||
| var msg = new MsgViewVariablesReqDocStrings | ||
| { | ||
| RequestId = _nextReqId++, | ||
| Kind = kind, | ||
| Keys = keys, | ||
| }; | ||
|
|
||
| var tcs = new TaskCompletionSource<string?[]>(); | ||
| _requestedDocStrings.Add(msg.RequestId, tcs); | ||
| _netManager.ClientSendMessage(msg); | ||
| return tcs.Task; | ||
| } | ||
|
|
||
| private void _netMessageDocStrings(MsgViewVariablesDocStrings message) | ||
| { | ||
| if (!_requestedDocStrings.Remove(message.RequestId, out var tcs)) | ||
| { | ||
| Sawmill.Error($"Server sent us DocStrings we never asked for, request ID {message.RequestId}."); | ||
| return; | ||
| } | ||
|
|
||
| tcs.SetResult(message.DocStrings); | ||
| } | ||
|
|
||
| private void OnDisconnectDocStrings(object? sender, NetDisconnectedArgs e) | ||
| { | ||
| _remoteDocStrings.Clear(); | ||
|
|
||
| // Nobody is going to answer these anymore, finish them with empty results. | ||
| foreach (var tcs in _requestedDocStrings.Values) | ||
| { | ||
| tcs.TrySetResult(Array.Empty<string?>()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That will flood _remoteDocStrings with values after you cleared it. Empty ones, but it will not be any less confusing. Considering async process tho, i am unsure how to properly fix this... Maybe it is just |
||
| } | ||
| _requestedDocStrings.Clear(); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| using Robust.Client.UserInterface.Controls; | ||
| using Robust.Client.UserInterface.CustomControls; | ||
| using Robust.Client.ViewVariables.Instances; | ||
| using Robust.Shared.ContentPack; | ||
| using Robust.Shared.GameObjects; | ||
| using Robust.Shared.IoC; | ||
| using Robust.Shared.Localization; | ||
|
|
@@ -25,6 +26,7 @@ internal sealed partial class ClientViewVariablesManager : ViewVariablesManager, | |
| [Dependency] private IRobustSerializer _robustSerializer = default!; | ||
| [Dependency] private IEntityManager _entityManager = default!; | ||
| [Dependency] private IViewVariableControlFactory _controlFactory = default!; | ||
| [Dependency] private IResourceManager _resManager = default!; | ||
|
|
||
| private uint _nextReqId = 1; | ||
| private readonly Vector2i _defaultWindowSize = (640, 420); | ||
|
|
@@ -45,6 +47,9 @@ public override void Initialize() | |
| { | ||
| base.Initialize(); | ||
| InitializeDomains(); | ||
| _netManager.RegisterNetMessage<MsgViewVariablesDocStrings>(_netMessageDocStrings); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same about order |
||
| _netManager.RegisterNetMessage<MsgViewVariablesReqDocStrings>(); | ||
| _netManager.Disconnect += OnDisconnectDocStrings; | ||
| _netManager.RegisterNetMessage<MsgViewVariablesOpenSession>(_netMessageOpenSession); | ||
| _netManager.RegisterNetMessage<MsgViewVariablesRemoteData>(_netMessageRemoteData); | ||
| _netManager.RegisterNetMessage<MsgViewVariablesCloseSession>(_netMessageCloseSession); | ||
|
|
@@ -274,6 +279,7 @@ protected override bool TryGetSession(Guid guid, [NotNullWhen(true)] out ICommon | |
| session = null; | ||
| return false; | ||
| } | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not needed i think |
||
| } | ||
|
|
||
| [Virtual] | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.