Add JSON-RPC access to directories and server lists (without connect/disconnect methods) #3479

ann0see · 2025-02-13T10:25:02Z

Short description of changes

Modified version of: #3249

Removed disconnect and connect methods since calling them would break the UI
Rebased to main

Quoting the initial PR:

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

To be tested.

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

Squash from: jamulussoftware#3249 rpc_notification jamulusclient/serverListReceived rpc_method jamulus/pollServerList Change JSON-RPC for server list. Use pollServerList instead of getServerList. (No result returned from the call.) Change "address" to "url" Adds documentation for new JSON-RPC methods Consistent use of countryId Fixes JSON-RPC docs JSON-RPC fixes for feedback to PR 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 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). JSON-RPC enhancements Adds method jamulusclient/recorderState. Changes intrument code to instrument name (remote client may not have access to lookup table). Fixes errors in docs. Fixes coding style Update docs Fix coding style

These methods unfortunately break the GUI if called

ann0see · 2025-02-13T10:32:07Z

I'd like to process with the content of #3249 without the disconnect and connect methods.
Second review will follow.

@riban-bw since you wrote the initial code, I'd also like to hear your opinion once reviews happen here.

ann0see · 2025-02-13T10:33:59Z

docs/JSON-RPC.md

@@ -146,9 +163,9 @@ Results:
 | result.id | number | The channel ID. |
 | result.name | string | The musician’s name. |
 | result.skillLevel | string | The musician’s skill level (beginner, intermediate, expert, or null). |
-| result.countryId | number | The musician’s country ID (see QLocale::Country). |
+| result.country | string | The musician’s country. |


#3249 (comment)

I'd suggest to add the id back...

Definitely. No problem with adding fields to the JSON, which can be used or ignored, but we should not remove existing fields that a JSON-RPC client might potentially be using already. So retain countryId and instrumentId, and add country and instrument as new fields.

ann0see · 2025-02-13T10:34:23Z

docs/JSON-RPC.md

@@ -444,9 +461,9 @@ Parameters:
 | params.clients[*].id | number | The channel ID. |
 | params.clients[*].name | string | The musician’s name. |
 | params.clients[*].skillLevel | string | The musician’s skill level (beginner, intermediate, expert, or null). |
-| params.clients[*].countryId | number | The musician’s country ID (see QLocale::Country). |
+| params.clients[*].country | string | The musician’s country. |


softins

Added a number of suggestions.

softins · 2025-02-13T16:26:19Z

docs/JSON-RPC.md

+| params.servers | array | The server list. |
+| params.servers[*].address | string | Socket address (ip_address:port) |
+| params.servers[*].name | string | Server name |
+| params.servers[*].country | string | Server country |


Also include countryId for consistency with jamulusclient/clientListReceived suggestion above.

softins · 2025-02-13T16:29:12Z

src/clientrpc.cpp

@@ -53,9 +53,9 @@ CClientRpc::CClientRpc ( CClient* pClient, CRpcServer* pRpcServer, QObject* pare
    /// @param {number} params.clients[*].id - The channel ID.
    /// @param {string} params.clients[*].name - The musician’s name.
    /// @param {string} params.clients[*].skillLevel - The musician’s skill level (beginner, intermediate, expert, or null).
-    /// @param {number} params.clients[*].countryId - The musician’s country ID (see QLocale::Country).
+    /// @param {string} params.clients[*].country - The musician’s country.


Suggested change

/// @param {string} params.clients[*].country - The musician’s country.

/// @param {number} params.clients[*].countryId - The musician’s country ID (see QLocale::Country).

/// @param {string} params.clients[*].country - The musician’s country.

softins · 2025-02-13T16:29:53Z

src/clientrpc.cpp

    /// @param {string} params.clients[*].city - The musician’s city.
-    /// @param {number} params.clients[*].instrumentId - The musician’s instrument ID (see CInstPictures::GetTable).
+    /// @param {string} params.clients[*].instrument - The musician’s instrument.


Suggested change

/// @param {string} params.clients[*].instrument - The musician’s instrument.

/// @param {number} params.clients[*].instrumentId - The musician’s instrument ID (see CInstPictures::GetTable).

/// @param {string} params.clients[*].instrument - The musician’s instrument.

softins · 2025-02-13T16:31:16Z

docs/JSON-RPC.md

@@ -146,9 +163,9 @@ Results:
 | result.id | number | The channel ID. |
 | result.name | string | The musician’s name. |
 | result.skillLevel | string | The musician’s skill level (beginner, intermediate, expert, or null). |
-| result.countryId | number | The musician’s country ID (see QLocale::Country). |
+| result.country | string | The musician’s country. |


softins · 2025-02-13T16:32:00Z

docs/JSON-RPC.md

 | result.city | string | The musician’s city. |
-| result.instrumentId | number | The musician’s instrument ID (see CInstPictures::GetTable). |
+| result.instrument | number | The musician’s instrument. |


softins · 2025-02-13T16:40:23Z

src/clientrpc.cpp

+                      QJsonObject objServerInfo{
+                          { "address", serverInfo.HostAddr.toString() },
+                          { "name", serverInfo.strName },
+                          { "country", QLocale::countryToString ( serverInfo.eCountry ) },


Suggested change

{ "country", QLocale::countryToString ( serverInfo.eCountry ) },

{ "countryId", serverInfo.eCountry },

{ "country", QLocale::countryToString ( serverInfo.eCountry ) },

softins · 2025-02-13T16:41:38Z

src/clientrpc.cpp

@@ -125,17 +199,17 @@ CClientRpc::CClientRpc ( CClient* pClient, CRpcServer* pRpcServer, QObject* pare
    /// @result {number} result.id - The channel ID.
    /// @result {string} result.name - The musician’s name.
    /// @result {string} result.skillLevel - The musician’s skill level (beginner, intermediate, expert, or null).
-    /// @result {number} result.countryId - The musician’s country ID (see QLocale::Country).
+    /// @result {string} result.country - The musician’s country.


Suggested change

/// @result {string} result.country - The musician’s country.

/// @result {number} result.countryId - The musician’s country ID (see QLocale::Country).

/// @result {string} result.country - The musician’s country.

softins · 2025-02-13T16:42:11Z

src/clientrpc.cpp

    /// @result {string} result.city - The musician’s city.
-    /// @result {number} result.instrumentId - The musician’s instrument ID (see CInstPictures::GetTable).
+    /// @result {number} result.instrument - The musician’s instrument.


Suggested change

/// @result {number} result.instrument - The musician’s instrument.

/// @result {number} result.instrumentId - The musician’s instrument ID (see CInstPictures::GetTable).

/// @result {number} result.instrument - The musician’s instrument.

softins · 2025-02-13T16:42:51Z

src/clientrpc.cpp

    /// @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.
            { "name", pClient->ChannelInfo.strName },
-            { "countryId", pClient->ChannelInfo.eCountry },
+            { "country", QLocale::countryToString ( pClient->ChannelInfo.eCountry ) },


Suggested change

{ "country", QLocale::countryToString ( pClient->ChannelInfo.eCountry ) },

{ "countryId", pClient->ChannelInfo.eCountry },

{ "country", QLocale::countryToString ( pClient->ChannelInfo.eCountry ) },

softins · 2025-02-13T16:43:36Z

src/clientrpc.cpp

            { "city", pClient->ChannelInfo.strCity },
-            { "instrumentId", pClient->ChannelInfo.iInstrument },
+            { "instrument", CInstPictures::GetName ( pClient->ChannelInfo.iInstrument ) },


Suggested change

{ "instrument", CInstPictures::GetName ( pClient->ChannelInfo.iInstrument ) },

{ "instrumentId", pClient->ChannelInfo.iInstrument },

{ "instrument", CInstPictures::GetName ( pClient->ChannelInfo.iInstrument ) },

ann0see force-pushed the jsonrpc/addDirectoryMethods branch from d830548 to 324c588 Compare February 13, 2025 10:26

ann0see added this to the Release 3.12.0 milestone Feb 13, 2025

Remove connect/disconnect methods

3c89fe5

These methods unfortunately break the GUI if called

ann0see force-pushed the jsonrpc/addDirectoryMethods branch from 324c588 to 3c89fe5 Compare February 13, 2025 10:28

ann0see requested review from pljones and softins February 13, 2025 10:30

ann0see commented Feb 13, 2025

View reviewed changes

softins reviewed Feb 13, 2025

View reviewed changes

riban-bw mentioned this pull request Feb 14, 2025

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

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JSON-RPC access to directories and server lists (without connect/disconnect methods) #3479

Add JSON-RPC access to directories and server lists (without connect/disconnect methods) #3479

ann0see commented Feb 13, 2025 •

edited

Loading

ann0see commented Feb 13, 2025

ann0see Feb 13, 2025

softins Feb 13, 2025

softins Feb 13, 2025

ann0see Feb 13, 2025

softins Feb 13, 2025

softins left a comment

softins Feb 13, 2025

softins Feb 13, 2025

softins Feb 13, 2025

softins Feb 13, 2025

softins Feb 13, 2025

softins Feb 13, 2025

softins Feb 13, 2025

softins Feb 13, 2025

softins Feb 13, 2025

softins Feb 13, 2025

softins Feb 13, 2025

	\| result.country \| string \| The musician’s country. \|
	\| result.countryId \| number \| The musician’s country ID (see QLocale::Country). \|
	\| result.country \| string \| The musician’s country. \|

	\| params.clients[*].country \| string \| The musician’s country. \|
	\| params.clients[*].countryId \| number \| The musician’s country ID (see QLocale::Country). \|
	\| params.clients[*].country \| string \| The musician’s country. \|

	\| params.servers[*].country \| string \| Server country \|
	\| params.servers[*].countryId \| number \| Server country ID (see QLocale::Country). \|
	\| params.servers[*].country \| string \| Server country \|

	/// @param {string} params.clients[*].country - The musician’s country.
	/// @param {number} params.clients[*].countryId - The musician’s country ID (see QLocale::Country).
	/// @param {string} params.clients[*].country - The musician’s country.

	/// @param {string} params.clients[*].instrument - The musician’s instrument.
	/// @param {number} params.clients[*].instrumentId - The musician’s instrument ID (see CInstPictures::GetTable).
	/// @param {string} params.clients[*].instrument - The musician’s instrument.

	\| result.instrument \| number \| The musician’s instrument. \|
	\| result.instrumentId \| number \| The musician’s instrument ID (see CInstPictures::GetTable). \|
	\| result.instrument \| string \| The musician’s instrument. \|

	{ "country", QLocale::countryToString ( serverInfo.eCountry ) },
	{ "countryId", serverInfo.eCountry },
	{ "country", QLocale::countryToString ( serverInfo.eCountry ) },

	/// @result {string} result.country - The musician’s country.
	/// @result {number} result.countryId - The musician’s country ID (see QLocale::Country).
	/// @result {string} result.country - The musician’s country.

	/// @result {number} result.instrument - The musician’s instrument.
	/// @result {number} result.instrumentId - The musician’s instrument ID (see CInstPictures::GetTable).
	/// @result {number} result.instrument - The musician’s instrument.

	{ "country", QLocale::countryToString ( pClient->ChannelInfo.eCountry ) },
	{ "countryId", pClient->ChannelInfo.eCountry },
	{ "country", QLocale::countryToString ( pClient->ChannelInfo.eCountry ) },

	{ "instrument", CInstPictures::GetName ( pClient->ChannelInfo.iInstrument ) },
	{ "instrumentId", pClient->ChannelInfo.iInstrument },
	{ "instrument", CInstPictures::GetName ( pClient->ChannelInfo.iInstrument ) },

Add JSON-RPC access to directories and server lists (without connect/disconnect methods) #3479

Are you sure you want to change the base?

Add JSON-RPC access to directories and server lists (without connect/disconnect methods) #3479

Conversation

ann0see commented Feb 13, 2025 • edited Loading

Checklist

ann0see commented Feb 13, 2025

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

softins 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ann0see commented Feb 13, 2025 •

edited

Loading