Skip to content
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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

DjLegolas
Copy link
Contributor

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 DjLegolas changed the title [UI][core] Fix interface not being updated in thinclient [3540][UI][core] Fix interface not being updated in thinclient Aug 6, 2022
@cas--
Copy link
Member

cas-- commented Feb 24, 2023

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 DjLegolas force-pushed the bug/3540/thinclient-not-updates-interface branch from d8c81ad to 5eda643 Compare June 3, 2023 07:48
@DjLegolas
Copy link
Contributor Author

depends on: #427

Copy link
Member

@cas-- cas-- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some thoughts on this change... 😄

Comment on lines +93 to +97
if (
client.is_daemon_version_equal_or_greater('2.1.1')
and client.core.is_valid_interface(listen_interface)
or not listen_interface
):
Copy link
Member

@cas-- cas-- Jun 3, 2023

Choose a reason for hiding this comment

The 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
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:
Copy link
Member

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 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants