Misc api calls#3783
Conversation
|
Thanks for your PR. It seems there was a mishap with commit 2926589 I can't tell if there are any changes that you made in that particular commit. |
|
This wasn't straightforward but I think I could extract what you originally intended to push. I think you merged your own fork's main (
|
| @@ -92,6 +94,8 @@ CClient::CClient ( const quint16 iPortNumber, | |||
| bJitterBufferOK ( true ), | |||
| bMuteMeInPersonalMix ( bNMuteMeInPersonalMix ), | |||
| iServerSockBufNumFrames ( DEF_NET_BUF_SIZE_NUM_BL ), | |||
| bRawAudioIsSupported ( false ), | |||
| iMyChannelID ( INVALID_INDEX ) | |||
| bRawAudioIsSupported ( false ) | |||
There was a problem hiding this comment.
Just need the spurious line 99 removed. Presumably left over from a copy/paste.
There was a problem hiding this comment.
I already fixed that in 809c5f6 and it compiles fine.
@dingodoppelt your branch
Yes, I think the issue was @digable1's first three commits were branched from The technique I use to keep a work-in-progress branch up to date with
If the rebase fails, then I use
If I am later on a different machine, I can't just
A separate issue was the inclusion of (I haven't yet reviewed the actual code for this PR) |
|
@digable1 as a suggestion, the easiest way for you to get your branch up to date would be as follows: |
|
I marked this as a draft - and such I wasn't actually expecting this to looked at. Yeah I saw those merges and didn't know the codebase well enough, which is why I accepted both. Again, not expecting this to actually be reviewed because I marked it as a draft.
Sorry for the insanity. Bottom line: Are we there yet, at least as a PR (pending review)? |
From what I could tell eyeballing this particular commit, it looks good. I am a bit concerned about the message "This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository." that is displayed in that commit. I had not seen that before, and while I know it's not nefarious, the results from a search for that string suggested it (looks like it really is missing an original source for that commit?) So: In case this helps...
Which means: If there was any issues in creating branch 809c5f6, you should be able to update from 'digable1/nfs-jamulus', branch 'misc-api-calls' and it should not have the merge conflicts you ran into out of the gate (as I think I mentioned earlier, I ran into them myself - which is why I marked it a Draft, so I knew I needed to take care of that, but you beat me to it). If it helps, I can do another PR without any hiccups like merge issues or stale code due to forgetting to pull (I think that's what happened) - or fix this PR. Bottom line: I think this your work on this PR is good, with that potentially weird message notwithstanding. Code-wise: At first blush my biggest concern is do we have the right scope for some of the bookeeping attributes this API-or-GUI connect/disconnect functionality - likely due to my newbieness with the codebase. And there could be duplicate code that shouldn't be there (should have been extracted into common methods). And of course, unit tests and e2e tests would help, with a test suite (I know, I'm becoming an endless media loop on this, and we all know this would help - just finding the time and person-power for it). Just sayin... |
Add new JSON-RPC API methods, re-factor connect and disconnect to share code between UI and API
CHANGELOG: <!--
NOTE: It is suggested that some of this works gets split into its own PR, specifically connect and disconnect (which affects other common code and UI behavior). This will be done in a subsequent edit to this PR and the creation another PR to handle the other API additions.
Context: Fixes an issue?
Provides further parity between the API and the UI (more to go)
Does this change need documentation? What needs to be documented and how?
JSON-RPC.md has been updated (auto-generated from source using provided tool)
Status of this Pull Request
This is a working implementation, as far as I know. Being a new developer to the Jamulus codebase, I might have missed a side effect or other issue that I don't know about yet - or corner case(s).
What is missing until this pull request can be merged?
More testing is desirable. It has been tested, but not as deeply as desirable due to internal re-factoring to share code for actions connect and disconnect (these actions alter the UI whether triggered by the UI or API).
Checklist