-
Notifications
You must be signed in to change notification settings - Fork 337
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
[3540][UI][core] Fix interface not being updated in thinclient #394
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -8,7 +8,6 @@ | |||||||||||||||||||||||
|
||||||||||||||||||||||||
import logging | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
from deluge.common import is_interface | ||||||||||||||||||||||||
from deluge.decorators import overrides | ||||||||||||||||||||||||
from deluge.i18n import get_languages | ||||||||||||||||||||||||
from deluge.ui.client import client | ||||||||||||||||||||||||
|
@@ -91,11 +90,19 @@ def add_config_values(self, conf_dict): | |||||||||||||||||||||||
) | ||||||||||||||||||||||||
elif ipt.name == 'listen_interface': | ||||||||||||||||||||||||
listen_interface = ipt.get_value().strip() | ||||||||||||||||||||||||
if is_interface(listen_interface) or not listen_interface: | ||||||||||||||||||||||||
if ( | ||||||||||||||||||||||||
client.is_daemon_version_equal_or_greater('2.1.1') | ||||||||||||||||||||||||
and client.core.is_valid_interface(listen_interface) | ||||||||||||||||||||||||
or not listen_interface | ||||||||||||||||||||||||
): | ||||||||||||||||||||||||
Comment on lines
+93
to
+97
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Firstly this could be more readable with an intermediary variable and secondly the fallback should be True not False since we cannot check the validity of the interface. Also I don't think the version should specify patch number since this new core function in theory should bump the minor version.
Suggested change
Oh and I just noticed that the call to client.core method will return a deferred so is always True so this will need a callback to get the actual result. That would lead us to look at the alternative option of not using the client daemon version check and instead handle the deferred fail for that method call. It might not be as explicit as the version check however... |
||||||||||||||||||||||||
conf_dict['listen_interface'] = listen_interface | ||||||||||||||||||||||||
elif ipt.name == 'outgoing_interface': | ||||||||||||||||||||||||
outgoing_interface = ipt.get_value().strip() | ||||||||||||||||||||||||
if is_interface(outgoing_interface) or not outgoing_interface: | ||||||||||||||||||||||||
if ( | ||||||||||||||||||||||||
client.is_daemon_version_equal_or_greater('2.1.1') | ||||||||||||||||||||||||
and client.core.is_valid_interface(outgoing_interface) | ||||||||||||||||||||||||
or not outgoing_interface | ||||||||||||||||||||||||
): | ||||||||||||||||||||||||
conf_dict['outgoing_interface'] = outgoing_interface | ||||||||||||||||||||||||
elif ipt.name.startswith('proxy_'): | ||||||||||||||||||||||||
if ipt.name == 'proxy_type': | ||||||||||||||||||||||||
|
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 have concerns about the naming of this method and the underlying problem we are solving.
In the first instance
valid
seems redundant but can understand clarification as a core method. Even if we were to use it I would suggest using it as a suffix rather than prefixis_interface_valid
.However I am leaning towards either
core.has_interface
orcore.has_interface_name
with perhaps more weight to the latter sincecommon.is_interface
seems to be my source of the contention. In addition the check if an IP is valid doesn't require a core method call and we don't actually check that the IP is assigned to one of the core host interfaces.Perhaps
core.has_interface
will suffice as a core method with a future check that IP is assigned to an interface.My final concern is that should we prevent the user from assigning an 'invalid' interface, e.g. interface is down briefly, and instead show a UI warning... That does increase the scope of this change so will need to revisit that