Skip to content

Add jamulusclient/setInstrumentCode JSONRPC method #3772

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

Open
dingodoppelt wants to merge 1 commit into
base: main
Choose a base branch
from
+43 −0
Open

Add jamulusclient/setInstrumentCode JSONRPC method #3772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 17 additions & 0 deletions docs/JSON-RPC.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,23 @@ Results:
| result | string | Always "ok". |


### jamulusclient/setInstrumentCode

Sets your instrument code.

Parameters:

| Name | Type | Description |
| --- | --- | --- |
| params.instrCode | number | The new instrument code. |

Results:

| Name | Type | Description |
| --- | --- | --- |
| result | string | Always "ok". |


### jamulusclient/setMidiSettings

Sets one or more MIDI controller settings.
Expand Down
26 changes: 26 additions & 0 deletions src/clientrpc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,32 @@ CClientRpc::CClientRpc ( CClient* pClient, CClientSettings* pSettings, CRpcServe
response["result"] = "ok";
} );

/// @rpc_method jamulusclient/setInstrumentCode
/// @brief Sets your instrument code.
/// @param {number} params.instrCode - The new instrument code.
/// @result {string} result - Always "ok".
pRpcServer->HandleMethod ( "jamulusclient/setInstrumentCode", [=] ( const QJsonObject& params, QJsonObject& response ) {

@ann0see ann0see Jul 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should have one call for instrument, name, skill level, imo.

If it was REST, we'd have PUT client/profile {name: ..., instrument: ..., skill level: ..., city:... } etc.

@dingodoppelt dingodoppelt Jul 14, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Do you mean to add setCity and setCountryCode as those are still missing? Skill and name are already settable, instrument will be with this merged.

@ann0see ann0see Jul 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes. However I think an alternative design would be one method to set all metadata for the client

@pljones pljones Jul 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For the JSONRPC, it's setter/getter mapping to the CClient entrypoints - or, here, CClientSettings (although I'd rather CClient orchestrated that...).

@dingodoppelt dingodoppelt Jul 14, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes. However I think an alternative design would be one method to set all metadata for the client

True. We submit our changes with pClient->SetRemoteInfo() so this is already happening (kind of).
Should the name be reused as well or should it rather be setChannelInfo analogous to getChannelInfo?
The method should still let users set single fields, i.e. only those supplied in the request, and leave the others untouched. There might be valid and invalid fields in the request which need to be checked for.

@ann0see ann0see Jul 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think setChannelInfo would be good.

@dingodoppelt dingodoppelt Jul 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For the JSONRPC, it's setter/getter mapping to the CClient entrypoints - or, here, CClientSettings (although I'd rather CClient orchestrated that...).

At the moment we have one getter getChannelInfo but multiple setters. This looks not too elegant but on the other hand I don't see see the need for more than one getter.
Implementing a single setter would be trickier than having multiple setters for we have to check more input fields for validity at the same time and need to define behaviour for every possible combination of invalid and valid fields.
Any ideas on how to proceed here? @ann0see @pljones

@ann0see ann0see Jul 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can't we parse the json and validate it against a json schema?

This should make optional fields trivial

@dingodoppelt dingodoppelt Jul 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, invalid json should fail right away.
After successful parsing we check which fields are valid. (name, instrument code, shoesize, ...)
We then check each field's value for validity (type, length) and set the valid ones.
We could return the number of set fields or maybe a more verbose message?

auto jsonInstrCode = params["instrCode"];

if ( !jsonInstrCode.isDouble() )
{
response["error"] = CRpcServer::CreateJsonRpcError ( CRpcServer::iErrInvalidParams, "Invalid params: instrCode is not a number" );
return;
}

const int _iInstrument = jsonInstrCode.toInt();

if ( CInstPictures::GetName ( _iInstrument ).isEmpty() )
{
response["error"] = CRpcServer::CreateJsonRpcError ( CRpcServer::iErrInvalidParams, "Invalid params: unknown instrCode" );
return;
}

pClient->ChannelInfo.iInstrument = _iInstrument;
pClient->SetRemoteInfo();
response["result"] = "ok";
} );

/// @rpc_method jamulusclient/sendChatText
/// @brief Sends a chat text message.
/// @param {string} params.chatText - The chat text message.
Expand Down
Loading