Add a web label#883
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.
There was a problem hiding this comment.
JSON is now parsed to JToken in the manager and the label only does template substitution, so the duplication is sorted. Regarding JsonPath.NET, I couldn't get this working in an acceptable manner.
| 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
There was a problem hiding this comment.
Done in the latest commit - IDataSourceFormat.
| { | ||
| 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)
Fix crash when no datasources specified
Update datasources format in ClientDefintions Add localisation Change to XNAClientWebLabel
…er already handles them. Replace _format string with IDataSourceFormat interface. IniDataSourceFormat now builds a JObject directly
4 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.
