JSON-RPC: Extend jamulusserver/getClients method #2918

Rob-NY · 2022-10-17T15:55:56Z

Short description of changes

Expands jamulusserver/getClients method to include all elements of a user profile as well as the number of active client connections. Changes were made so they are backward compatible with the prior message format.

CHANGELOG: RPC: jamulusserver/getClients method expanded to include all elements of a user profile as well as the number of active client connections.

Context: Fixes an issue?

Addresses issue #2488 and comments in #2890

Does this change need documentation? What needs to be documented and how?

JSON-RPC documentation was updated via python tool and is included in this PR.

Status of this Pull Request

What is missing until this pull request can be merged?

Compile tested on Linux only.

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

src/serverrpc.cpp

src/util.h

src/server.cpp

src/serverdlg.cpp

src/util.h

ann0see · 2022-11-15T21:14:03Z

I'd like to have this in the next release...

Rob-NY · 2022-11-16T03:57:01Z

I'd like to have this in the next release...

Me too. I've fixed what I believe are the issues raised (as opposed to philosophical questions), squashed and pushed. Once this is merged I will resubmit the dependent PR.

hoffie

Thanks for the PR and sorry for not commenting earlier. Looks good to me in general despite two questions (see inline comments).

src/serverrpc.cpp

src/util.h

Rob-NY · 2022-11-24T02:36:38Z

I'd like to have this in the next release...

@hoffie - is there anything I still need to do to move this along? I'm asking because I'd like to resubmit the dependent PR which builds upon this basic one as a candidate as well. I believe I've addressed all the refactor requests and questions.

pljones · 2023-01-05T07:37:58Z

@Rob-NY there are a number of unanswered requests.

Rob-NY · 2023-01-05T11:18:32Z

@Rob-NY there are a number of unanswered requests.

ISO Country code standardization: I have no idea how to approach this at this time, so I asked to pass on this for the time being - no response.

In November, @hoffie asked you if we really wanted to expose the translated strings in some areas of this api - no response.

I made the changes for the "Skillset: NONE" vs. "not set" request as well as all the others raised here. If I missed something, please point it out to me and I'll address right away.

ann0see · 2023-01-05T12:26:43Z

Ok. So then there was a misunderstanding on both sides about the state of this PR.

I‘ll look where I can help if I have time.

docs/JSON-RPC.md

src/util.h

src/serverrpc.cpp

ann0see

Ok. See ann0see@f3d6bea
I hope we can agree on removing these methods as:

The country code is inconsistent between QT5 and QT6 (we already expose the country name which is not optimal but probably ok)
The skill level is translated and thus inconsistent. Alternatively, we'd find a way to only use the English translation. However, I think using just IDs is fine in this case, as the Jamulus protocol specifies the meaning. I believe that if we remove this - to stay consistent we should also remove the text translations of other translated names.

ann0see · 2023-04-26T20:39:44Z

@pljones @Rob-NY would you be ok with my proposed changes?

Rob-NY · 2023-04-28T00:12:19Z

@pljones @Rob-NY would you be ok with my proposed changes?

No issue.

ann0see · 2023-05-03T20:54:33Z

@Rob-NY @pljones pushed my commit. Waiting on further review now.

ann0see · 2023-05-04T19:45:01Z

Ok. Rebased. Will do a new local test now.

ann0see

Local testing says this is ok. still, we could remove the instrument name - as it is translated and thus not reliable. Qt doesn't seem to support any short name for countries, so the country name is probably the best we can get.

ann0see · 2023-05-09T20:46:59Z

@pljones (and maybe @dtinth) Could you please review this (especially from a C++ dev viewpoint)? I'd like to have this in. The legacy and iOS build failures are unrelated.

dtinth

Looks good to me too.

vecChanInfo in CServerDlg seems unused but that's fine I guess.

pljones · 2023-05-10T16:01:02Z

docs/JSON-RPC.md

 | result.clients | array | The list of connected clients. |
 | result.clients[*].id | number | The client’s channel id. |
 | result.clients[*].address | string | The client’s address (ip:port). |
 | result.clients[*].name | string | The client’s name. |
 | result.clients[*].jitterBufferSize | number | The client’s jitter buffer size. |
 | result.clients[*].channels | number | The number of audio channels of the client. |
+| result.clients[*].instrumentCode | number | The id of the instrument for this channel. |
+| result.clients[*].instrumentName | number | The text name of the instrument for this channel. |


instrumentName | number? That sounds strange. What is it meant to represent? The position in the enum? In which case it's redundant.

we could remove the instrument name

I think this is wrongly documented and should be a string (but I still believe it should be removed due to the translation issue.)

pljones · 2023-05-10T16:02:50Z

I'd like a new issue raised with the changes requested here but not made. Once that's done, I'm happy to approve it.

ann0see · 2023-05-11T19:16:32Z

@pljones

For fixable stuff, I'd only have the removal of translated content (= instrument name).
I believe there is more, but I think others have raised these issues:

JSON-RPC: Extend jamulusserver/getClients method #2918 (comment)
JSON-RPC: Extend jamulusserver/getClients method #2918 (comment)
JSON-RPC: Extend jamulusserver/getClients method #2918 (review)
(feel free to add your comments to this list and then open an issue - it seems that most open stuff is related to design issues in the original code you found.)

pljones · 2023-05-12T18:56:52Z

@pljones

For fixable stuff, I'd only have the removal of translated content (= instrument name). I believe there is more, but I think others have raised these issues:

Yes, it's probably mostly the use of strings where the number would be better that should be fixed (at some point):
#2918 (comment)

ann0see · 2023-05-13T19:56:00Z

Ok. I've now removed the instrument name too.
@pljones could you please go through the comments (especially your comments) and open the issue? I think you have more context.

pljones

I'll approve anyway on the basis nothing here is breaking as far as I can see. The countryName issue is complicated because of Qt 5/6 rather than anything else, as far as I remember and I wasn't involved much in the solution there.