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

New connection settings Connection Timeout field #2443

Merged
merged 8 commits into from
Jan 6, 2025

Conversation

sebjulliand
Copy link
Collaborator

@sebjulliand sebjulliand commented Jan 2, 2025

Changes

Resolves #2442 and #2440.

This PR adds a new readyTimeout field to the connection settings (i.e. to the ConnectionData type). This field is described here.

A new number input has been added to the Connection/Login Settings to manage this field:
image

The CustomUI class has been enhanced to support the number input type as well as its min and max attributes.
The SSH port field has been turned into a number input.

How to test this PR

  1. Create a new connection with wrong credentials
  2. Change the Connection Timeout field to 1000 (i.e. 1 second)
  3. Try to connect: the connection attempt must fail after 1 second
  4. Change the Connection Timeout field to 5000
  5. Try to connect: the connection attempt must fail after 5 seconds

Checklist

  • have tested my change

@sebjulliand sebjulliand added the enhancement New feature or request label Jan 2, 2025
@sebjulliand sebjulliand self-assigned this Jan 2, 2025
Copy link
Contributor

github-actions bot commented Jan 2, 2025

👋 A new build is available for this PR based on a2c3496.

@julesyan
Copy link
Collaborator

julesyan commented Jan 3, 2025

There is some odd behaviour that I have been able to inconsistently recreate. I set the value to 2000 (or 20,000), double checked the value. Connected to the system successfully. Disconnected, then checked the value again and it seems to have changed the timeout value to 1999 (or was 19,998). I will see if i can consistently recreate it, but so far its sporadic behaviour.
image

@julesyan
Copy link
Collaborator

julesyan commented Jan 3, 2025

I can set the readyTimeout field to 0, even though it has min set to 1. It shows as red, but the save is still allowed. I checked the settings file and it does save as 0. It does not treat the timeout as 0ms, it seems to just ignore the timeout when it is 0.
image

In regards to the other issue I mentioned with the timeout value changing, I have been unable to recreate it more than 2 more times after 200+ tries. So it seems to be a possible one off of the string to number conversion. I wouldn't be concerned about it.

@sebjulliand
Copy link
Collaborator Author

Thanks @julesyan !
I fixed the numeric value not being entirely checked - setting a value lesser than 1 will not allow you to save the settings.

As for the odd behavior, I think I found what the "issue" is. Spoiler alert: it's not a bug, it's a feature. 😅
It turns out that if a numeric field has the focus and your mouse pointer it on it, the value will increment/decrement if you use your mouse scrollwheel!

To try to prevent this from happening, I moved the field to the bottom of the form. Since it used to be in the middle, it was natural to use the wheel to scroll down to the Save button, with the risk of changing the value while doing so.

Give it a try and let me know how it goes!

@sebjulliand
Copy link
Collaborator Author

sebjulliand commented Jan 5, 2025

I changed the type of the SSH port number field to a "number" input, so there's a better check on the value.

Copy link
Collaborator

@julesyan julesyan left a comment

Choose a reason for hiding this comment

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

Everything looks good aside from if i have an invalid value for the timeout, then use the scroll or up/down arrows on the field, it does not revalidate that data, it stays as an error

@sebjulliand sebjulliand requested a review from julesyan January 6, 2025 13:36
@sebjulliand
Copy link
Collaborator Author

@julesyan it should be OK now. I used a more global event to catch changes in any input field. Let me know how it goes!

Copy link
Collaborator

@julesyan julesyan left a comment

Choose a reason for hiding this comment

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

Looks good!

@sebjulliand
Copy link
Collaborator Author

Much obliged @julesyan 🙏🏻

@sebjulliand sebjulliand merged commit ae2c38f into master Jan 6, 2025
3 checks passed
@sebjulliand sebjulliand deleted the feature/SSHreadyTimeout branch January 6, 2025 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a connection setting to change SSH ready timeout value
2 participants