-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add the fake login page #66
Conversation
bartoval
commented
Mar 25, 2023
- Added interactive login screen
- moved the login logic to LoginScreen.cs
would fix #63 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just the one minor change here, but you'll need to make the "default settings" changes in the other code, too.
Scenes/LoginScreen.cs
Outdated
if (err == Godot.Error.Ok) | ||
{ | ||
// enable/disable authentication in dev mode | ||
activateAuthDev = (Boolean)clientConfig.GetValue("auth", "activate_auth_dev"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case where there's no config file, we need defaults baked in to the code. When we do a build of the client and distribute it, the config file doesn't get baked in. The config file really only gets used for dev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still have to test these new changes.
Since the configuration values do not change at runtime, it may be worthwhile to create a static class that can be accessed from anywhere in the game, and initialize it in the Main game. This will make it easier to manage and modify the configuration values in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're welcome to do that. The server just embeds the defaults into the places where the config file is loaded.
} | ||
|
||
url = OS.GetEnvironment("SERVER_STRING") ?? (String)clientConfig.GetValue("amqp", "server_string", "amqp://127.0.0.1:5672"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't consider the case where the required environment variables are not present, in production
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good enough for now - thank you!
merging without testing.