-
Notifications
You must be signed in to change notification settings - Fork 704
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
CommonClient: fix bug when using Connect button without a disconnect #3609
Conversation
…onnect does to fix an issue where losing connection would make you unable to connect to a different server
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.
Maybe it could be considered as another bug (or not a bug at all), but I see that ctx.connect
also set ctx.password
. Should it also set to None
?
Anyway, given this at least fix the issue when there is no password, LGTM.
kvui.py
Outdated
@@ -599,6 +599,7 @@ def connect_button_action(self, button): | |||
self.ctx.username = None | |||
async_start(self.ctx.disconnect()) | |||
else: | |||
self.ctx.username = None |
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.
Since self.ctx.username = None
is in both branch of the if self.ctx.server
, would it be better to extract it?
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.
didn't notice this haha, fixed
invalid passwords trigger a different workflow where you can fix it, nothing (in that specific workflow) for clearing and fixing a slot, so I think that's fine |
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.
Merged in and texted the changes which fixed a bug that consistently annoyed me
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.
Change looks safe and I am gonna trust the 2 peer reviews.
The password thing is addressed in #3641
What is this fixing or adding?
makes the kivy connect button do the same username forgetting that /connect does to fix an issue where losing connection would make you unable to connect to a different server
How was this tested?
With two servers that have different valid slot names
If this makes graphical changes, please attach screenshots.