jamulussoftware
Add JSON-RPC access to directories and server lists.
Short description of changes
Adds JSON-RPC notification of server lists from directories. Allows polling a directory for this info.
CHANGELOG: Added jamulusclient/pollServerList methods and jamulusclient/receivedServerList notification to JSON-RPC interface.
Context: Fixes an issue?
As discussed here, it may be advantageous to increase the control and monitoring provided by the JSON-RPC interface. This provides access to server lists on directories.
Does this change need documentation? What needs to be documented and how?
This PR adds the associated documentation of the new method and notification in the JSON-RPC md page.
Status of this Pull Request
What is missing until this pull request can be merged?
Checklist
- [X] I've verified that this Pull Request follows the general code principles
- [X] I tested my code and it does what I want
- [ ] My code follows the style guide
- [X] I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
- [X] I've filled all the content above
I have changed from countryID (which is QT specific an not particulary useful to a JSON-RPC client that may not have access to the QLocale class) to country string. I have done this throughout including clientList, etc. for the same reason (it is of more use to a remote client.)
I have added connect / disconnect methods but am not triggering a signal to update the GUI. Any help here would be appreciated.
I have added a method to provide server ping time and quantity of connected clients. I poll for this from each server after receiving a serverList. I wonder if this is desirable behaviour. (It works well for my use case.)
I am aware that I added a function to the client class to expose its ConnLessProtocol. That feels wrong to me but I couldn't see an alternative method to receive CLServerListReceived signal. Maybe someone can validate this.
FYI these changes are working well when integrating Jamulus client into Zynthian.
I have changed from countryID (which is QT specific an not particulary useful to a JSON-RPC client
I think we had a discussion about that in the past. I'm not sure what the consensus was.
have added connect / disconnect methods but am not triggering a signal to update the GUI. Any help here would be appreciated.
Seems not trivial. Does the UI look like it's not connected? I believe there's a Boolean flag to show the faders. But not sure.
Ah. I think the problem was translation. Therefore we used the ID over a string for e.g instruments. Otherwise there's a chance that someone who set the client to non English gets other values than someone who uses the English version
I have changed from countryID (which is QT specific an not particulary useful to a JSON-RPC client
I think we had a discussion about that in the past. I'm not sure what the consensus was.
I could not find this discussion. In a bid to be consistent and provide the most useful information to any client I have changed countryID to country text and instrumentID to instrument text in the JSON-RPC messages. We can't expect all clients to have access to these id/string maps. A client needs to be able to show the correct information. There may be an issue with i18n/l10n in which case we should use an internation standard for country code, e.g. ISO 3166-1. For instrument names we may prefer to change back to jamulus table to allow localisation within client (with associated local tables) although that could be done with strings too, i.e. a default English instrument name with clients able to map to local language.
have added connect / disconnect methods but am not triggering a signal to update the GUI. Any help here would be appreciated.
Seems not trivial. Does the UI look like it's not connected? I believe there's a Boolean flag to show the faders. But not sure.
I think the issue here is that there isn't a signal sent from the client to the client GUI when a connect occurs. There is an assumption that connect will be done only from the connect dialog which explicitly updates the client GUI. I am not sufficiently familiar with QT and jamulus code to best know how to refactor to implement such a signal.
I have added jamulusclient/recorderState notification so that JSON-RPC connected clients can indicate the recorer status of the connected server which seems important for privacy.
Please run clang-format on your device or apply the style changes from here: https://github.com/jamulussoftware/jamulus/actions/runs/8416630554/job/23043980690#step:4:1
There may be an issue with i18n/l10n in which case we should use an internation standard for country code, e.g. ISO 3166-1. For instrument names we may prefer to change back to jamulus table to allow localisation within client (with associated local tables) although that could be done with strings too
Yes, we need to be consistent and I think the ID was the non optimal agreement. please check what the string is if you change the la gauge to verify it gives back something useful.
There may be an issue with i18n/l10n in which case we should use an internation standard for country code, e.g. ISO 3166-1. For instrument names we may prefer to change back to jamulus table to allow localisation within client (with associated local tables) although that could be done with strings too
Yes, we need to be consistent and I think the ID was the non optimal agreement. please check what the string is if you change the la gauge to verify it gives back something useful.
I can't remember the discussion; it was probably while I was inactive for a time. But as mentioned above, using the ID allows for consistency between different language settings.
We already require a server operator to look up the correspondence between ID and Country, so they can specify the correct ID. I think so long as we document the list of IDs for Countries (as per Qt5) and Instruments (as per util.cpp), then we can expect a JSON-RPC client developer to have reference to this list and include a suitable mapping in their client.
Maybe it would also be useful to output the Country name and Instrument name in any case, according to the current language that is set, but we should still include the ID as well, so the client developer can choose which to use. There's no harm having both.
Maybe it would also be useful to output the Country name and Instrument name in any case, according to the current language that is set, but we should still include the ID as well, so the client developer can choose which to use. There's no harm having both.
On a side note: Having made the above point, I checked my jamulus-php to see what I had done there. I was not including the IDs, so I've just updated it additionally to include the IDs for Country, Instrument and Skill Level.
I was away for a few days which delayed my response. I just tested and the instrument name (string) is sent in the language selected / configured in the client, e.g. changing from English to Spanish, (after a client restart) the client sends "oyente" instead of "listener". I think this works well. The JSON-RPC client will have the correct instrument name without requring a lookup table (with data redundency and associated risk of error and bloat).
The country is always in English, the same as the GUI client so this looks like it might be a compilation issue, i.e. not using the locale / lookup at runtime. So this looks like an issue with the client's core code, not with the JSON-RPC interface. I suggest this be corrected in the client core code so that users see the country name in their native / selected language.
We could send both numeric and text representation of the data (country, instrument) as suggested above but this is data redundancy (generally considered a bad thing) within a rather verbose protocol (JSON) so I would prefer not. If the decision makers here decide this is desirable I will change the PR to include the codes too. I recommend keeping the strings as this provides the JSON-RPC client with useful data without the need for manually maintained look-up tables which adds undesirable and unnecessary code.
I'd argue that the API should not change language. I think we didn't want duplication and thus decided on the ID.
If the API uses codes then the map from code to string should also be available via the API, e.g. jamulus/getCountryCodes. This would allow a client to create the correct text without needing to store a static map. The JSON-RPC client should not need to have detailed knowledge / tables of the server's internals. It should be able to control and monitor the remote device through function calls.
I could add these extra methods: jamulus/getCountryNames, jamulus/getInstrumentNames. Maybe jamulus/getCountryName with parameter countryId would return the name for the requested country.
@riban-bw could you please have a look at the code style starting here: https://github.com/jamulussoftware/jamulus/actions/runs/8416630554/job/23043980690?pr=3249#step:4:20 this needs some adjustments. You can most likely copy the patch directly from the output.
Otherwise, add the proposed messages. Usually, we'd prefer to have them in a separate PR or at least a clearly separated commit. This PR would then only focus on the initial changes you requested.
Also to make the JSON-RPC documentation check happy
Please run ./tools/generate_json_rpc_docs.py to regenerate docs/JSON-RPC.md
I have added connect / disconnect methods but am not triggering a signal to update the GUI. Any help here would be appreciated.
I'm not sure how to do that unfortunately, but I think the code has "emit" somewhere.
Otherwise, I'd probably look in the documentation (e.g. https://doc.qt.io/qt-6/signalsandslots.html). Maybe @pljones knows more about the GUI?
Thanks. There's still a spacing error (remove space before the *) in client.h and a missing one in clientrpc.cpp
-
if ( pClient->SetServerAddr ( jsonAddr.toString() ) )
is required.
Also the ID should still be there. Then in a later PR you'd change them if needed.
Also the ID should still be there. Then in a later PR you'd change them if needed.
I'm on it. Just checking compile then will push fixes.
If there's code in the Client Dialog UI that's needed by the RPC interface, it should be moved to the Client itself. There's a lot of code that's been put in the wrong place. The Dialog UI layer should be dumb - just responding to UI events and updating state in the Client (or Server for the Server Dialog UI) - or responding to Client events by updating the UI. It should work stuff out or hold state (for example, like the issue with "Mute" - that's only held in the UI).
The only thing I can think of is to factor out the actual disconnection process and introduce a GUI method to set the UI to connected and disconnected. We might also use signals?
We might also use signals?
That's the only correct way to do it with Qt, in my view.
The UI is meant to respond to signals - either from the widget set or from CClient. The response to a widget signal should be to update CClient. The response to a CClient signal should be to update the widget layer (including shutting down CClientDlg, if appropriate and not otherwise handled).
Ok. Then the way to go is to also issue a signal for updating the UI which means that the UI update code needs to be refactored into a new method.
This PR is to provide improved JSON-RPC interface. I may not be the right person to refactor the core code to improve UI/backend integration with better use of signals. I appreciate and agree with the plan but does that belong in this PR? I feel like there should be a separate bit of work to abstract the core client functionality from the UP to the client code. That may impact (improve) the remote control but feels wrong to bind it to this PR.
I am not clear what is outstanding on this PR. I think I have reviewed and fixed all suggestions (thanks) apart from refactoring the client and UI to implement better use of signals.
So we're suggesting:
- mixer UI and/or RPC interface issues signal to CClient to update fader state
- CClient updates fader state in CChannel and sends appropriate protocol message
- CClient receives protocol messages, updates CChannel state, and issues signal to indicate appropriate state change
- mixer UI and/or RPC interface listen for state change signals and respond appropriately
- CClient also provides "set fader state" and "get fader state" methods; set sends message to server, get retrieves latest held state
If this can be split into one PR per change it would probably be easier to get it moving. The changes where there are problems would then not block the changes where there aren't.
If this can be split into one PR per change it would probably be easier to get it moving. The changes where there are problems would then not block the changes where there aren't.
Many of those changes were added in response to reviewer's comments. Do you mean that each reviewer observation should be a seperate PR until the codebase is sufficiently clean to allow the actual PR? Many of the additional changes requested by reviewers relate to historical issues that I was requested to resolve so we do seem to be some way beyond the original PR. Some of these I do not have sufficient understanding of the whole codebase and I may not be the ideal canidate to resolve.
I'm not sure what this means from the auto build
I'm actually not too unhappy with getting this merged as is, given:
- fix the above error
- fix the doc for connect
I'd consider whether or not leaving the ClientUI Connect dialog open is a problem for the JSON-RPC client a separate issue, to be looked at when thinking about properly refactoring the ClientUI so it doesn't hold state.