Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
407a290
Implements JSON-RPC methods to get server lists
riban-bw Mar 22, 2024
aa56c27
Change JSON-RPC for server list.
riban-bw Mar 22, 2024
667191d
Adds documentation for new JSON-RPC methods
riban-bw Mar 22, 2024
0372ce1
Consistent use of countryId
riban-bw Mar 22, 2024
cbeb68e
Fixes JSON-RPC docs
riban-bw Mar 22, 2024
86ed64e
JSON-RPC fixes for feedback to PR
riban-bw Mar 23, 2024
401c9f9
JSON-RPC Enhancements
riban-bw Mar 24, 2024
6573b9c
JSON-RPC enhancements
riban-bw Mar 25, 2024
48f2d6b
Fixes coding style
riban-bw Apr 1, 2024
a23fd99
Update docs
riban-bw Apr 1, 2024
65329f2
Fix coding style
riban-bw Apr 1, 2024
2941143
Add back in the country and instrument IDs
riban-bw Apr 1, 2024
8938ff9
Fixes docs and coding style
riban-bw Apr 2, 2024
6c6bfe2
Fixes docs
riban-bw Apr 2, 2024
16d88df
Fixes some documentation
riban-bw May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions docs/JSON-RPC.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,22 @@ Results:
| result.clients | array | The client list. See jamulusclient/clientListReceived for the format. |


### jamulusclient/pollServerList

Requests server list.
ann0see marked this conversation as resolved.
Show resolved Hide resolved

Parameters:

| Name | Type | Description |
| --- | --- | --- |
| params.directory | string | URL of directory, e.g. anygenre1.jamulus.io:22124 |
ann0see marked this conversation as resolved.
Show resolved Hide resolved

Results:

| Name | Type | Description |
| --- | --- | --- |
| result | string | "ok" or "error" if invalid URL |

### jamulusclient/sendChatText

Sends a chat text message.
Expand Down Expand Up @@ -449,6 +465,21 @@ Parameters:
| params.clients[*].instrumentId | number | The musician’s instrument ID (see CInstPictures::GetTable). |


### jamulusclient/serverListReceived

Emitted when the server list is received.

Parameters:

| Name | Type | Description |
| --- | --- | --- |
| params.servers | array | The server list. |
| params.servers[*].url | url | The server's URL. |
| params.servers[*].name | string | The server’s name. |
| params.servers[*].countryId | number | The servers’s country ID (see QLocale::Country). |
| params.servers[*].city | string | The server’s city. |


### jamulusclient/connected

Emitted when the client is connected to the server.
Expand Down
4 changes: 4 additions & 0 deletions src/client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,10 @@ class CClient : public QObject
Channel.GetBufErrorRates ( vecErrRates, dLimit, dMaxUpLimit );
}

CProtocol * getConnLessProtocol() {
return &ConnLessProtocol;
}

// settings
CChannelCoreInfo ChannelInfo;
QString strClientName;
Expand Down
56 changes: 56 additions & 0 deletions src/clientrpc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,67 @@ CClientRpc::CClientRpc ( CClient* pClient, CRpcServer* pRpcServer, QObject* pare
} );
} );

/// @rpc_notification jamulusclient/serverListReceived
/// @brief Emitted when the server list is received.
/// @param {array} params.servers - The server list.
/// @param {string} params.servers[*].url - IP Address
/// @param {string} params.servers[*].name - Server name
/// @param {number} params.servers[*].countryId - Server country code
/// @param {string} params.servers[*].city - Server city
connect ( pClient->getConnLessProtocol(), &CProtocol::CLServerListReceived, [=] ( CHostAddress /* unused */, CVector<CServerInfo> vecServerInfo ) {
QJsonArray arrServerInfo;
for ( const auto& serverInfo : vecServerInfo )
{
QJsonObject objServerInfo{
{ "url", serverInfo.HostAddr.toString() },
{ "name", serverInfo.strName },
{ "countryI", serverInfo.eCountry },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{ "city", serverInfo.strCity },
};
arrServerInfo.append ( objServerInfo );
}
pRpcServer->BroadcastNotification ( "jamulusclient/serverListReceived",
QJsonObject{
{ "servers", arrServerInfo },
} );
} );

/// @rpc_notification jamulusclient/disconnected
/// @brief Emitted when the client is disconnected from the server.
/// @param {object} params - No parameters (empty object).
connect ( pClient, &CClient::Disconnected, [=]() { pRpcServer->BroadcastNotification ( "jamulusclient/disconnected", QJsonObject{} ); } );

/// @rpc_method jamulus/pollServerList
/// @brief Request list of servers in a directory
/// @param {string} params.directory - URL of directory to query, e.g. anygenre1.jamulus.io:22124.
/// @result {string} result - "ok" or "error" if bad arguments.
pRpcServer->HandleMethod ( "jamulus/pollServerList", [=] ( const QJsonObject& params, QJsonObject& response ) {
auto jsonDirectoryIp = params["directory"];
if ( !jsonDirectoryIp.isString() )
{
response["error"] = CRpcServer::CreateJsonRpcError ( CRpcServer::iErrInvalidParams, "Invalid params: directory is not a string" );
riban-bw marked this conversation as resolved.
Show resolved Hide resolved
return;
}

CHostAddress haDirectoryAddress;
if ( NetworkUtil().ParseNetworkAddress (
jsonDirectoryIp.toString(),
haDirectoryAddress,
false ) )
{
// send the request for the server list
pClient->CreateCLReqServerListMes( haDirectoryAddress );
response["result"] = "ok";
}
else
{
response["error"] = CRpcServer::CreateJsonRpcError ( CRpcServer::iErrInvalidParams, "Invalid params: directory is not a valid url" );
}

response["result"] = "ok";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this always ok and not an error if this is called on an already disconnected client?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was only following similar code elsewhere in the interface. A common question is whether we want an error when we ask for an action and the action gives the desired effect, e.g. disconnect from a device to which we are not connected results in being disconnected. Is this erroneous? I would guess other places this same, alway "ok" result is used here allows such an action that will never fail to give an acknowledgement that the request was recevied and actioned.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Then we can leave it as is.

Q_UNUSED ( params );
} );

/// @rpc_method jamulus/getMode
/// @brief Returns the current mode, i.e. whether Jamulus is running as a server or client.
/// @param {object} params - No parameters (empty object).
Expand Down
Loading