Fix bug in AuthenticationDialog #463
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cas--
requested changes
Sep 3, 2024
Comment on lines
+257
to
+258
| self.destroy() | ||
| self.deferred.callback(response) |
There was a problem hiding this comment.
I know this was copied from the other dialog but we can make use of super to call the parent class cleanup steps
Suggested change
| self.destroy() | |
| self.deferred.callback(response) | |
| super()._on_response(widget, response) |
| @@ -316,7 +316,7 @@ def _on_connect_fail(self, reason, host_id, try_counter): | |||
|
|
|||
| def dialog_finished(response_id): | |||
| if response_id == Gtk.ResponseType.OK: | |||
| self._connect(host_id, dialog.get_username(), dialog.get_password()) | |||
There was a problem hiding this comment.
If AuthenticationDialog methods get_username and get_password are not usable then they should also be removed from the class.
cas--
reviewed
Sep 3, 2024
| @@ -23,6 +23,7 @@ Contributors (and Past Developers): | |||
| * Pedro Algarvio ('s0undt3ch') <[email protected]> | |||
| * Cristian Greco ('cgreco') <[email protected]> | |||
| * Chase Sterling ('gazpachoKing') <[email protected]> | |||
| * Justin Williams ('Jaywalker') | |||
There was a problem hiding this comment.
This list is really for authors of significant contributions to Deluge
zakkarry
suggested changes
Sep 6, 2024
There was a problem hiding this comment.
Two linting issues by the way, figured I'd point them out.
https://github.com/deluge-torrent/deluge/actions/runs/10640286820/job/29631116756
| @@ -316,7 +316,7 @@ def _on_connect_fail(self, reason, host_id, try_counter): | |||
|
|
|||
| def dialog_finished(response_id): | |||
| if response_id == Gtk.ResponseType.OK: | |||
| self._connect(host_id, dialog.get_username(), dialog.get_password()) | |||
| self._connect(host_id, dialog.account.username, dialog.account.password) | |||
There was a problem hiding this comment.
Suggested change
| self._connect(host_id, dialog.account.username, dialog.account.password) | |
| self._connect( | |
| host_id, dialog.account.username, dialog.account.password | |
| ) |
| self.account = Account( | ||
| self.username_entry.get_text(), | ||
| self.password_entry.get_text(), | ||
| "", |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Specific bug is described here: https://dev.deluge-torrent.org/ticket/3643#ticket
The issue occured because the
AuthenticationDialogwasdestroy()'d before the calls were made to get the text from theirGTK.Entry()'s. It has been fixed by using the same method asAccountDialogand storing the information in anAccountobject prior to thedestroy()call.It may be worth considering giving the user an option to save/update the
hostlist.conffile with the working credentials as well, but that would merit discussion and a separate PR. This PR simply fixes the problem and leaves existing logic flow in place.