Feature set cl message#3727
Conversation
| FS_MULTITHREADING = 1, | ||
| FS_RECORDER_INIT = 2, | ||
| FS_DISABLE_RECORDING = 3, | ||
| FS_IS_RECORDING = 4, |
There was a problem hiding this comment.
I don't think we need three flags:
FS_RECORDER_ENABLED- true ifGetRecorderInitialised() && !GetDisableRecording()FS_IS_RECORDING- true ifJamController.GetRecorderState() == RS_RECORDING(you had!=?)
There was a problem hiding this comment.
I don't think we need three flags:
* `FS_RECORDER_ENABLED` - true if `GetRecorderInitialised() && !GetDisableRecording()` * `FS_IS_RECORDING` - true if `JamController.GetRecorderState() == RS_RECORDING` (you had `!=`?)
From my understanding JamController.GetRecorderState() == RS_RECORDING is only true when GetRecorderInitialised() && !GetDisableRecording() is true (recorder may be actively recording or idling) while both are not transparently reflecting if the server is currently recording.
I think we need to know...
- if the recorder is initialised -> a recording directory is set with
-R - if recording will start automatically when a client joins the server -> recording is enabled by NOT using
--norecord - if the server is currently recording which is currently not known in the jamController. @pljones Is there a way for CServer to know for sure if a recording is currently taking place on a server?
There was a problem hiding this comment.
I think only two states need to be shown -- either the Server is recording or the Server could be recording. The reason why is not really relevant. So that's what I was trying to reflect.
I think you're right that I got the logic wrong, though.
(I mean, that --norecord is in use doesn't stop someone monitoring their server with JSONRPC and turning on recording automatically when a Client joins. It's just a different mechanism.)
|
Could you please also check which protocol messages could be replaced by this? |
| (standard re-registration timeout). | ||
|
|
||
|
|
||
| - PROTMESSID_CLM_SERVER_INFO: Bitmask of enabled server features |
There was a problem hiding this comment.
I initially phrased it as "capabilities". FYI.
There were old versions which didn't support panning I think. Assuming this message was already implemented at this time, We would have used this message to advertise pan support.
In think we should make it very clear what this message can and cannot encode
There was a problem hiding this comment.
It's a new message being added for this new feature.
There was a problem hiding this comment.
Yes. I know. But we should specify what this feature can and what it can't expose. I'm thinking of a specification/architectural viewpoint
There was a problem hiding this comment.
We are not exclusively checking for features available in newer versions. There are status flags like recording enabled and if there is a welcome message. Those aren't strictly capabilities so I think calling it a "feature set" makes more sense here. I'd therefore propose to call it PROTMESSID_CLM_SERVER_FEATURE_SET
|
My view is that it's exposing the operating state of the Server. If you're running with the UI or using JSONRPC, some state can change (one day, it'll all be controllable, hopefully). However, it's not for large blocks of data: client list, welcome message, etc. I see it as providing answers to questions a Client user might have about Server capabilities (I also favour using that term) - so on/off states. That's why I think it should be stuff that gets shown on the Connect Dialog (although I'm thinking of a "toggle extra connect info" option in Advanced Settings). |
|
Ok. Can it be used by the client to to enable/disable certain features such as raw audio? |
Yes, I agree. I plan to add a separate protocol request to fetch the welcome message. Would be useful for Explorer, and we could add a means within the Connect Dialog to right-click on a host and view the welcome message. |
I'm not sure that's very useful, and may add significant complication for some features, which set up things at initialisation time. I would treat this PR as just a way of exposing information, and any means of control could be investigated and proposed as a subsequent PR. |
2abdc4a to
6dd6cc4
Compare
I don't know if the style check is right this time... |
|
I'd suggest adding two flags to see if the server
any objections? |
What OS do you do your dev on? If you have |
8caf060 to
139c0fc
Compare
I use kate on arch. I just used the "format" feature of the built-in LSP-client for the first time 👍 |
|
Would be good if you can install |
Yes, that file is used by kate. I just ran |
It may depend on which version of |
Ah, I'm on |
|
The Could that become an issue when not updated or is only the comment outdated? |
Both make sense to me. I'd actually thought of the licence one myself and forgot to add it! |
| { | ||
| uint32_t iFeatures = 0; | ||
|
|
||
| iFeatures |= ( !bUseDoubleSystemFrameSize << FS_FAST_UPDATE ); |
There was a problem hiding this comment.
I'd like to have comments which describe exactly which feature is enabled - especially if it's non obvious like the recorder. Yes, we do have FS_* but that may get ambiguous for non trivial features in future.
There was a problem hiding this comment.
While writing these I came to the conclusion that the recorder flags need a makeover and I'll push the comments once I figured out how to handle recording state properly.
|
How about:
|
I don't think these are relevant to the use case.
|
Short description of changes
Adds two new connection-less protocol messages, one to request and one to return server features.
CHANGELOG: Request and receive server features by connection-less message
Context: Fixes an issue?
Fixes 3710
Does this change need documentation? What needs to be documented and how?
Yes. See below. Will also need updates to Client and Server documentation.
Status of this Pull Request
Proof of concept. Needs:
What is missing until this pull request can be merged?
See above.
Checklist