[3540][UI][core] Fix interface not being updated in thinclient #394
Conversation
DjLegolas
changed the title
|
Ah yes I encountered this issue! Will need to handle backward compatibility in the client for daemons that don't have that method exported |
For new UI features to be added, one should make sure the backend daemon is supported and add fallback in case it doesn't. Here we add the ability to get the daemon version from the `Client` class and also check the version against a desired version.
When changing the interface in both `GTK` and `ConsoleUI`, we call directly to `deluge.common.is_interface`, which checks the interface on the local machine and not on the daemon's machine. The `WebUI` does not seem to have any validation on input. closes: https://dev.deluge-torrent.org/ticket/3540
DjLegolas
force-pushed
the
bug/3540/thinclient-not-updates-interface
branch
from
d8c81ad to
5eda643
Compare
|
depends on: #427 |
| if ( | ||
| client.is_daemon_version_equal_or_greater('2.1.1') | ||
| and client.core.is_valid_interface(listen_interface) | ||
| or not listen_interface | ||
| ): |
There was a problem hiding this comment.
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.
| if ( | |
| client.is_daemon_version_equal_or_greater('2.1.1') | |
| and client.core.is_valid_interface(listen_interface) | |
| or not listen_interface | |
| ): | |
| listen_interface_valid=( | |
| client.core.is_valid_interface(listen_interface) | |
| if client.is_daemon_version_equal_or_greater('2.2') | |
| else True | |
| ) | |
| if listen_interface_valid or not listen_interface: |
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...
| @@ -817,6 +817,11 @@ def set_config(self, config: Dict[str, Any]): | |||
| continue | |||
| self.config[key] = config[key] | |||
|
|
|||
| @export | |||
| def is_valid_interface(self, interface: str) -> bool: | |||
There was a problem hiding this comment.
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 prefix is_interface_valid.
However I am leaning towards either core.has_interface or core.has_interface_name with perhaps more weight to the latter since common.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
When changing the interface in both
GTKandConsoleUI, we calldirectly to
deluge.common.is_interface, which checks the interface onthe local machine and not on the daemon's machine.
The
WebUIdoes not seem to have any validation on input.closes: https://dev.deluge-torrent.org/ticket/3540