Conversation
|
Nightly build for this pull request:
|
Metadorius
left a comment
Metadorius
left a comment
There was a problem hiding this comment.
Nice! Gave you some feedback.
Generally though while this approach definitely has it's place, I am not sure if we should do an explicit separate data source manager? My thoughts were more about something like an implicit manager class; different labels could have content URL endpoints specified right in place, and the response will be shared between them by that class (say, if the requests came to the manager in span of half a minute, then give the same data to every subscriber; if something updates the URL -- then all of the consumers of that URL will receive that).
Not fully set on the above, what are your thoughts?
| private List<string> ParseJSONPath(string json, string path) | ||
| { | ||
| try | ||
| { | ||
| var root = Newtonsoft.Json.Linq.JToken.Parse(json); | ||
| var results = new List<string>(); | ||
|
|
||
| var tokens = root.SelectTokens(path); | ||
|
|
||
| if (tokens != null) | ||
| foreach (var token in tokens) | ||
| if (token != null) | ||
| results.Add(token.ToString()); | ||
|
|
||
| return results.Count > 0 ? results : null; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Logger.Log($"JSONPath error: {ex.Message}"); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this should happen way earlier at manager stage, and the control should only receive a parsed json and do template substitution, otherwise we're duplicating work.
Also we already use System.Text.Json, I see that it has no support for JSON Path for some reason, maybe this extension to System.Text.Json would do better? https://www.nuget.org/packages/JsonPath.Net Just so we don't introduce two similar libraries.
| public string Template { get; set; } | ||
| public string LoadingText { get; set; } | ||
| public int MaxResults { get; set; } = 0; | ||
| public string FallbackText { get; set; } = "N/A"; |
ClientCore/JSONDataSourceManager.cs
Outdated
| private string ConvertIniToJson(string iniContent) | ||
| { | ||
| var iniFile = new IniFile(); | ||
| using (var reader = new StringReader(iniContent)) |
There was a problem hiding this comment.
There is a constructor in IniFile that takes a stream, no need to parse it by hand, just take a string and make a stream out of it 😄
| var sb = new StringBuilder(); | ||
| sb.Append("{"); | ||
| bool firstSection = true; |
There was a problem hiding this comment.
The aforementioned bringing of JSON parsing earlier than that will allow you to construct the programmatic representation via a few foreaches, iterating sections and then keys to set values and greatly simplifying the code 🙂
| if (string.IsNullOrEmpty(value)) | ||
| continue; | ||
|
|
||
| // format: URL,RefreshIntervalSeconds,TimeoutSeconds,Format |
There was a problem hiding this comment.
URL could have commas in it, I think instead should do something like
[DataSourceName]
URL=https://...
RefreshIntervalSeconds=Also I think individual data source parsing should be in the DataSource itself?
| private static JSONDataSourceManager _instance; | ||
| public static JSONDataSourceManager Instance => _instance ??= new JSONDataSourceManager(); |
There was a problem hiding this comment.
We have a DI container in use, I think you could use that? The codebase haven't fully migrated though, so tell me if you encounter any issues with that
| private readonly string _url; | ||
| private readonly int _refreshIntervalSeconds; | ||
| private readonly int _timeoutSeconds; | ||
| private readonly string _format; |
There was a problem hiding this comment.
I think this _format should be either an enum and/or maybe even a tiny simple interface/class with one method that converts input value (stream?) and gives JSON for arbitrary data representation.
There was a problem hiding this comment.
I agree with an interface. So we can decouple some string parsing logic from the UI logic. My recent editing experience on LAN lobby shows that it might waste a huge amount of time to maintain codes if different kinds of logics are not decoupled
| { | ||
| private readonly string _id; | ||
| private readonly string _url; | ||
| private readonly int _refreshIntervalSeconds; |
There was a problem hiding this comment.
does it run even in background or when controls using it is not active?
although I realise now that it will probably complicate the code a lot, so I don't think there's a big need in doing it exactly. but if there is a lot of players that don't go online -- maybe autorefresh isn't something we may want to do? it will strain our servers quite a bit, maybe we should update just when the control is shown for the first time? (or sth in this direction)
|
|
||
| namespace ClientGUI; | ||
|
|
||
| public class XNAClientJSONLabel : XNALabel |
There was a problem hiding this comment.
Perhaps let's name it XNAClientWebLabel, because it's not just JSON anymore?
There was a problem hiding this comment.
also maybe it could be good to have it as a descendant of XNALinkLabel? in case the users want to see details
Fix crash when no datasources specified
| public string Template { get; set; } | ||
| public string LoadingText { get; set; } | ||
| public int MaxResults { get; set; } = 0; | ||
| public string FallbackText { get; set; } = "N/A"; |
There was a problem hiding this comment.
"N/A" is short for not applicable, instead of "not available". So I don't think we should not use "N/A" here
There was a problem hiding this comment.
it's used interchangeably as either of those, so I think it's fine?
3 participants
This PR adds a label that can query JSON data from a datasource. We can use this for the online player count, for ladder ranking, or news. INI datasources are also supported.
Datasources are specified in
ClientDefinitions.iniLabels then point to a datasource and can query the JSON.
See
INISytem.mdfor more info.Excuse the crappy screenshot.
