JSON-RPC - Extend jamulusserver/getClients to include additional client attributes

[Rob-NY]

What is the current behaviour and why should it be changed?

The current getClients RPC method does not return skill level, country, city, or instrument for connected clients. In order to close the gap between desired functionality in (now closed) PR #2006, getClients can be extended to provide these additional properties.

Describe possible approaches

@dtinth had as specific goal of not touching server.cpp when initially authoring JSON-RPC. This limited the information available to the RPC interface. The most direct solution I see now is to modify GetConCliParam() to add an additional vector that can be populated with Channel Information (core). GetConCliParam() is already used by JSON-RPC within the getClients method.

void CServer::GetConCliParam ( CVector<CHostAddress>& vecHostAddresses,
                               CVector<QString>&      vecsName,
                               CVector<int>&          veciJitBufNumFrames,
                               CVector<int>&          veciNetwFrameSizeFact,
                               CVector<CChannelCoreInfo>& vecChanInfo )

Changing this method impacts server.cpp/h and serverdlg.cpp only. While serverdlg would not make use of this additional information, having connection attributes such as country, city, instrument, and skill would allow the serverdlg to make use of this information in the future within the UI's "connections" widget.

The modified GetConCliParam method proposed is:

void CServer::GetConCliParam ( CVector<CHostAddress>& vecHostAddresses,
                               CVector<QString>&      vecsName,
                               CVector<int>&          veciJitBufNumFrames,
                               CVector<int>&          veciNetwFrameSizeFact,
                               CVector<CChannelCoreInfo>& vecChanInfo )
{
    // init return values
    vecHostAddresses.Init ( iMaxNumChannels );
    vecsName.Init ( iMaxNumChannels );
    veciJitBufNumFrames.Init ( iMaxNumChannels );
    veciNetwFrameSizeFact.Init ( iMaxNumChannels );
    vecChanInfo.Init ( iMaxNumChannels );

    // check all possible channels
    for ( int i = 0; i < iMaxNumChannels; i++ )
    {
        if ( vecChannels[i].IsConnected() )
        {
            // get requested data
            vecHostAddresses[i]      = vecChannels[i].GetAddress();
            vecsName[i]              = vecChannels[i].GetName();
            veciJitBufNumFrames[i]   = vecChannels[i].GetSockBufNumFrames();
            veciNetwFrameSizeFact[i] = vecChannels[i].GetNetwFrameSizeFact();
            vecChanInfo[i]           = vecChannels[i].GetChanInfo();
        }
    }
}

The new channel attributes would then be added to the getClients JSON-RPC method:

<SNIP>

      CVector<int>          veciNetwFrameSizeFact;
        CVector<CChannelCoreInfo> vecChanInfo;


        pServer->GetConCliParam ( vecHostAddresses, vecsName, veciJitBufNumFrames, veciNetwFrameSizeFact, vecChanInfo );
        
        // we assume that all vectors have the same length
        const int iNumChannels = vecHostAddresses.Size();

        // fill list with connected clients
        for ( int i = 0; i < iNumChannels; i++ )
        {
            if ( vecHostAddresses[i].InetAddr == QHostAddress ( static_cast<quint32> ( 0 ) ) )
            {
                continue;
            }
            QJsonObject client{
                { "id", i },
                { "address", vecHostAddresses[i].toString ( CHostAddress::SM_IP_PORT ) },
                { "name", vecsName[i] },
                { "jitterBufferSize", veciJitBufNumFrames[i] },
                { "channels", pServer->GetClientNumAudioChannels ( i ) },
                { "instrument", vecChanInfo[i].iInstrument },
                { "city", vecChanInfo[i].strCity },
                { "skillLevel", vecChanInfo[i].eSkillLevel },
                { "countryCode", vecChanInfo[i].eCountry },
            };
            clients.append ( client );
        }
<SNIP>

The proposal here returns the enumerated values of some attributes to be consistent with the current approach. Issue #2468 proposes to change this. The solution proposed here could be refactored if that issue moves forward.

Has this feature been discussed and generally agreed? No

[dtinth]

FYI; My goal of not touching server.cpp / client.cpp only applies to my original PR #1975 😄. It was because I wanted that PR to focus only on introducing an API layer.

Now that that PR is merged, I think future work can change the client/server files as needed to support that feature/improvement. Encouraged especially if it would result in cleaner code if client/server files are adjusted.

[Rob-NY]

@dtinth - yes, I tried to say “initial RPC” to make that point. However, not reaching into those files was cited as the early reason why the functionality from my UDP PR could not (would not) be addressed as part of the initial JSON-RPC merge. I completely get that — but that fact is the basis and justification for this request so I thought it was worth pointing out.

[ann0see]

Changing this method impacts server.cpp/h and serverdlg.cpp only

Is serverdlg.cpp also available on headless?

[pljones]

Is serverdlg.cpp also available on headless?

Changing server.cpp can affect serverdlg.cpp when it's not headless.

[ann0see]

So this would only work in GUI Mode? I thought the main point or at least one of the main points of JSON-RPC is to obtain more information on headless servers

[pljones]

So this would only work in GUI Mode?

What makes you think that?

[ann0see]

I haven’t looked into serverdlg.cpp but the name implies that it’s somehow related to the UI. Probably it makes sense to try how the headless server behaves.

[hoffie]

I think there's confusion about which uses what. @Rob-NY proposed to change a method which was previously used by the UI only. JSON-RPC started using it as well. If we extend it, both callers (JSON-RPC and UI) have to be adjusted. That does not mean that JSON-RPC becomes dependent on the UI. I think what @Rob-NY suggested is a perfectly valid approach.

This is not about introducing method UI method calls from JSON-RPC.

[pljones]

Exactly. A can be dependent upon C and B can be dependent upon C without any dependency being implied between A and B or any reverse dependency from C to A or B being implied. In fact, C shouldn't know or care what's calling its methods or responding to its signals.

(And none of the above requires looking at any code. It's a general principle and doesn't just apply in Computer Science.)