Add JSON-RPC access to directories and server lists. #3249

riban-bw · 2024-03-22T22:55:07Z

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

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

rpc_notification jamulusclient/serverListReceived rpc_method jamulus/pollServerList

Use pollServerList instead of getServerList. (No result returned from the call.) Change "address" to "url"

ann0see

All in all good. Thanks. See my comments. Also @dtinth could you please have a look at this?

docs/JSON-RPC.md

ann0see · 2024-03-23T07:34:17Z

src/clientrpc.cpp

+            QJsonObject objServerInfo{
+                { "url", serverInfo.HostAddr.toString() },
+                { "name", serverInfo.strName },
+                { "countryI", serverInfo.eCountry },


Is that consistent how we handle countries elsewhere? Also why is it countryI? Not countryId?

countryId is how country codes are handled elsewhere, e.g. in clientList.
"countryI" is a type - I will fix.

On further consideration (and attempt to use in the field) I think this should be a string holding the country name as text. QLocale is QT specific and JSON-RPC clients may not be QT applications hence this id would be meaningless. I will change it in the PR. I think the country code used in client list should also be changed but that is a breaking change that is not directly related to this PR.

src/clientrpc.cpp

Changes method "jamulus/pollServerList" to "jamulusclient/pollServerList" (for consistency). Changes "url" to "address" in protocol and "server address" in docs. Fixes typo in countryId.

riban-bw · 2024-03-23T09:39:59Z

Pushed enhancements in response to PR feedback.

riban-bw · 2024-03-24T16:35:57Z

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.

Adds jamulusclient/connect method. Adds jamulusclient/disconnect method. Adds jamulusclient/serverInfoReceived notification. Return country string rather than (QT specific) country code. Request server info after server list received (to get each server's ping time and num clients).

ann0see · 2024-03-24T22:49:13Z

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.

Adds method jamulusclient/recorderState. Changes intrument code to instrument name (remote client may not have access to lookup table). Fixes errors in docs.

ann0see · 2024-03-25T07:56:43Z

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

riban-bw · 2024-03-25T07:58:02Z

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.

ann0see · 2024-03-25T07:58:05Z

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

ann0see · 2024-03-25T12:21:59Z

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.

softins · 2024-03-26T16:43:23Z

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.

softins · 2024-03-26T17:12:11Z

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.

riban-bw · 2024-03-27T07:59:18Z

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.

ann0see · 2024-03-27T08:04:05Z

I'd argue that the API should not change language.
I think we didn't want duplication and thus decided on the ID.

riban-bw · 2024-03-27T09:55:29Z

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.

ann0see · 2024-03-28T12:03:13Z

yes. That makes sense. @dtinth any concerns?

ann0see · 2024-03-31T16:50:49Z

@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.

ann0see · 2024-03-31T16:51:25Z

Also to make the JSON-RPC documentation check happy

Please run ./tools/generate_json_rpc_docs.py to regenerate docs/JSON-RPC.md

ann0see · 2024-03-31T17:18:00Z

docs/JSON-RPC.md

@@ -186,6 +186,22 @@ Results:
 | result.clients | array | The client list. See jamulusclient/clientListReceived for the format. |


+### jamulusclient/pollServerList
+
+Requests the server list from a specified directory.


Sorry for my forgetfulness here - you need to update the python script I mentioned below and not edit the file directly

I have run the script and pushed the changes.

ann0see · 2024-03-31T17:26:16Z

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?

ann0see · 2024-04-01T17:41:28Z

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.

src/clientrpc.cpp

ann0see · 2024-04-18T17:05:26Z

src/clientrpc.cpp

    /// @result {string} result.city - The musician’s city.
    /// @result {number} result.instrumentId - The musician’s instrument ID (see CInstPictures::GetTable).
+    /// @result {string} result.instrument - The musician’s instrument.
    /// @result {string} result.skillLevel - Your skill level (beginner, intermediate, expert, or null).
    pRpcServer->HandleMethod ( "jamulusclient/getChannelInfo", [=] ( const QJsonObject& params, QJsonObject& response ) {
        QJsonObject result{
            // TODO: We cannot include "id" here is pClient->ChannelInfo is a CChannelCoreInfo which lacks that field.


Typo, but not your issue.

Suggested change

// TODO: We cannot include "id" here is pClient->ChannelInfo is a CChannelCoreInfo which lacks that field.

// TODO: We cannot include "id" here as pClient->ChannelInfo is a CChannelCoreInfo which lacks that field.

Fixed in next commit.

ann0see · 2024-04-18T17:34:39Z

docs/JSON-RPC.md

@@ -471,3 +526,43 @@ Parameters:
 | params | object | No parameters (empty object). |


+### jamulusclient/recorderState
+
+Emitted when the client is connected to a server who's recorder state changes.


So this gets emitted only at connection time, not when the server changes its state as it is connected?

It only gets emitted when the client is connect to a server and that server's recorder state changes. I think the first part is probably redundant -- i.e. it can be assumed that a client won't be informed of state changes to servers to which it isn't connected.

Fixed in next commit.

ann0see · 2024-04-18T17:42:40Z

src/clientrpc.cpp

+    pRpcServer->HandleMethod ( "jamulusclient/disconnect", [=] ( const QJsonObject& params, QJsonObject& response ) {
+        if ( pClient->IsRunning() )
+        {
+            pClient->Stop();


I believe that this should call Disconnect() here instead

jamulus/src/clientdlg.cpp

Line 1244 in e12cb71

void CClientDlg::Disconnect()

if the UI is running.

Nothing should call Dialog methods, except internally. Ever. The Dialog UI might not exist if th RPC interface is in use.

We need to update the UI somehow. -> refactoring needed.

Yes. If a change to state happens to the Client (CClient), it should emit an event. The UI (CClientDlg) can respond to that. But the state should be in the Client itself, not the UI. That way, it's accessible to both UI and RPC-I.

src/clientrpc.cpp

ann0see

The GUI stuff is a blocker in my opinion, but methods seem to be available.
We should probably check if the gui is running and then call different code.
Otherwise it's the right direction.
Sorry for the delay.

pljones · 2024-04-19T15:44:44Z

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).

ann0see · 2024-04-19T21:54:06Z

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?

pljones · 2024-04-20T13:56:59Z

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).

ann0see · 2024-04-22T21:54:09Z

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.

riban-bw · 2024-05-07T05:18:22Z

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.

ann0see · 2024-05-07T06:54:29Z

The issue is that the UI thing is a blocker for me.

pljones · 2024-05-07T16:50:45Z

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

ann0see

I'm sorry to say that, but in my opinion we need to get the GUI stuff solved. I would suggest to revert the connection and disconnect behavior- which I know is substential out of this PR and get the other changes merged. Then open a new PR with only the connect/disconnect changes. Then we can work on the slot/signal idea there.

ann0see · 2024-06-30T13:28:28Z

Ok. Can we get everything which doesn't break the UI in?

pljones · 2024-07-01T17:32:32Z

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.

riban-bw · 2024-07-02T05:42:55Z

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.

pljones · 2024-07-02T19:14:07Z

docs/JSON-RPC.md

+
+### jamulusclient/connect
+
+Disconnect client from server


Surely this is

Suggested change

Disconnect client from server

Connect from client to server

pljones · 2024-07-02T19:21:26Z

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.

ann0see · 2024-07-02T19:39:35Z

Please get rid of the connect/disconnect features then and put them in a separate PR.

ann0see · 2024-07-15T22:01:34Z

Otherwise: do we find a compromise that the connect/disconnect methods only work headless?

ann0see · 2024-09-08T12:58:27Z

Ok. Have a look at #3364

If we can get these changes in and probably some refactoring, maybe the GUI issues can be fixed.

ann0see · 2024-12-14T21:17:22Z

@pljones do you think it makes sense extracting everything except for the connect and disconnect functionality out of this PR and then open a separate PR for just connection an disconnection? Then we do not depend on refactoring the connection/disconnection methods yet.

pljones · 2024-12-15T11:18:37Z

I'd rather do no more work on the RPC client that didn't depend on signals from CClient itself -- depending on signals from elsewhere (CProtocol::CLServerListReceived in this case) isn't something I'm keen on. CClient should be exposing a cleaner API and we should be working on that first, in my view, at this stage. The more we add workarounds using the existing APIs, the more work we create to undo in the future.

riban-bw added 5 commits March 22, 2024 09:21

Implements JSON-RPC methods to get server lists

407a290

rpc_notification jamulusclient/serverListReceived rpc_method jamulus/pollServerList

Change JSON-RPC for server list.

aa56c27

Use pollServerList instead of getServerList. (No result returned from the call.) Change "address" to "url"

Adds documentation for new JSON-RPC methods

667191d

Consistent use of countryId

0372ce1

Fixes JSON-RPC docs

cbeb68e

ann0see reviewed Mar 23, 2024

View reviewed changes

JSON-RPC fixes for feedback to PR

86ed64e

Changes method "jamulus/pollServerList" to "jamulusclient/pollServerList" (for consistency). Changes "url" to "address" in protocol and "server address" in docs. Fixes typo in countryId.

JSON-RPC enhancements

6573b9c

Adds method jamulusclient/recorderState. Changes intrument code to instrument name (remote client may not have access to lookup table). Fixes errors in docs.

ann0see requested changes Mar 31, 2024

View reviewed changes

ann0see added this to the Release 3.11.0 milestone Mar 31, 2024

riban-bw added 2 commits April 1, 2024 18:05

Fixes coding style

48f2d6b

Update docs

a23fd99

ann0see reviewed Apr 18, 2024

View reviewed changes

src/clientrpc.cpp Show resolved Hide resolved

ann0see reviewed Apr 18, 2024

View reviewed changes

src/clientrpc.cpp Show resolved Hide resolved

ann0see requested changes Apr 18, 2024

View reviewed changes

pljones assigned riban-bw May 6, 2024

Fixes some documentation

16d88df

ann0see reviewed May 11, 2024

View reviewed changes

pljones modified the milestones: Release 3.11.0, Release 3.12.0 Jun 21, 2024

pljones reviewed Jul 2, 2024

View reviewed changes

ann0see mentioned this pull request Sep 7, 2024

Connection status client and gui can get out of sync. #2519

Open

pljones added the JSON-RPC Related to the JSON-RPC API label Nov 16, 2024

	// TODO: We cannot include "id" here is pClient->ChannelInfo is a CChannelCoreInfo which lacks that field.
	// TODO: We cannot include "id" here as pClient->ChannelInfo is a CChannelCoreInfo which lacks that field.

Add JSON-RPC access to directories and server lists. #3249

Are you sure you want to change the base?

Add JSON-RPC access to directories and server lists. #3249

Conversation

riban-bw commented Mar 22, 2024 • edited by ann0see Loading

Checklist

ann0see left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riban-bw commented Mar 23, 2024

riban-bw commented Mar 24, 2024

ann0see commented Mar 24, 2024

ann0see commented Mar 25, 2024

riban-bw commented Mar 25, 2024

ann0see commented Mar 25, 2024

ann0see commented Mar 25, 2024

softins commented Mar 26, 2024

softins commented Mar 26, 2024

riban-bw commented Mar 27, 2024

ann0see commented Mar 27, 2024

riban-bw commented Mar 27, 2024

ann0see commented Mar 28, 2024

ann0see commented Mar 31, 2024

ann0see commented Mar 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ann0see commented Mar 31, 2024

ann0see commented Apr 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pljones Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pljones Apr 20, 2024 • edited Loading

Choose a reason for hiding this comment

ann0see left a comment • edited Loading

Choose a reason for hiding this comment

pljones commented Apr 19, 2024 • edited Loading

ann0see commented Apr 19, 2024

pljones commented Apr 20, 2024

ann0see commented Apr 22, 2024

riban-bw commented May 7, 2024

ann0see commented May 7, 2024

pljones commented May 7, 2024 • edited Loading

ann0see left a comment

Choose a reason for hiding this comment

ann0see commented Jun 30, 2024

pljones commented Jul 1, 2024

riban-bw commented Jul 2, 2024

Choose a reason for hiding this comment

pljones commented Jul 2, 2024

ann0see commented Jul 2, 2024

ann0see commented Jul 15, 2024

ann0see commented Sep 8, 2024

ann0see commented Dec 14, 2024

pljones commented Dec 15, 2024

riban-bw commented Mar 22, 2024 •

edited by ann0see

Loading

pljones Apr 19, 2024 •

edited

Loading

pljones Apr 20, 2024 •

edited

Loading

ann0see left a comment •

edited

Loading

pljones commented Apr 19, 2024 •

edited

Loading

pljones commented May 7, 2024 •

edited

Loading