Skip to content
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.

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

Allow central server to confirm registration #87

Merged

Conversation

pljones
Copy link
Collaborator

@pljones pljones commented Apr 17, 2020

Also display this state in the server UI.

Rebased and retested with the latest master.

Also display this state in the server UI.
| 1 byte status |
+---------------+

- "status":
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understood, you are transmitting the enum now and not a single bit, right? Then you should update the documentation here, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will merge your changes now. Please fix this in another pull request later on.

@corrados corrados merged commit 9161853 into jamulussoftware:master Apr 18, 2020
@corrados
Copy link
Contributor

Thank you for your contribution!

@corrados
Copy link
Contributor

The central server code is now updated. If you like you can test it now with the official Central Server.

@pljones pljones deleted the feature/55-server-registration-status branch April 18, 2020 10:43
@pljones
Copy link
Collaborator Author

pljones commented Apr 18, 2020

Looks good. The styling on the status messages makes them more obvious. The only thing is it appears to have lost the "and will re-try" after a timeout - unless that's no longer happening (which could also make sense).

@corrados
Copy link
Contributor

Are you referring to this line which I have changed? https://github.com/corrados/jamulus/blob/master/src/serverdlg.cpp#L565

I did this because the original text was too technical in my opinion.

@pljones
Copy link
Collaborator Author

pljones commented Apr 18, 2020

Yeah, it was a bit clumsy - I'm happy they got changed. But it lost information, too - it mentioned retries, now it doesn't.

Looking at it, this one may well be wrong:
https://github.com/corrados/jamulus/blob/master/src/serverdlg.cpp#L569
I think, because it got a response - just like success and full states - it won't retry.

@corrados
Copy link
Contributor

Yes, but I think if it does not succeed and the error is not "server list full", something fatal does not work and the user has to do something to solve it anyway. I do not think the "retry" will give useful additional information to the user. Do you agree?

@corrados
Copy link
Contributor

Looking at it, this one may well be wrong:

What would you write instead?

@pljones
Copy link
Collaborator Author

pljones commented Apr 18, 2020

What it means is that the server response was successfully received but not understood.

The retries are just the "standard ping" ones, so "retrying" is misleading, really.

Basically, the slave server is too old to understand and needs to be upgraded. Dropping the "retrying" should do.

corrados added a commit that referenced this pull request Apr 18, 2020
@corrados
Copy link
Contributor

done

@pljones
Copy link
Collaborator Author

pljones commented Apr 18, 2020

Based off #91 I added this patch:
#99

  • Moves the stringifying code out
  • Passes "nogui" down to server side components
  • Logs connection state to central server

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants