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

Aliases for Target Hosts #334

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Aliases for Target Hosts #334

wants to merge 17 commits into from

Conversation

johnny-keker
Copy link
Contributor

Resolves #332

Reworked Target Hosts edition dialog (see image). Now host and port are separate fields (latter will be set to default 9339 when new host is added), and also we have optional Alias field. If set to non-empty, this host will be listed by its alias in dropdown.

User still can manually enter new host to the hosts dropdown in format ip:port. User input is carefully handled - if there is such host, but wit alias, alias will be used. If user just enters known alias, corresponding host will be used.

The last, but not least, this PR contains a work on refactoring the logic of storing and using current target host. Before, there were a lot of duplication of essentially same info (string ip:port, string ip and ushort port, HostItem, ServerConnectionOptions...), now it is standardized and much easier to understand and extend (f.e. add an alias). Also, RemoteMachine and Port are gone from project config, since this info is already stored in TargetHosts, which works like MRU collection.

image

@kikimych
Copy link
Contributor

kikimych commented Nov 1, 2022

Could you please use standart library classes IPEndPoint and IPAddress? I think it's better then writing own serialization and deserialization methods

@johnny-keker
Copy link
Contributor Author

Could you please use standart library classes IPEndPoint and IPAddress? I think it's better then writing own serialization and deserialization methods

I believe, what we need here, is simple and maintainable structure which represents remote host (ip and port in our case). I don't see the case for using more complicated data structures than that. Besides, do you suggest serialization of such classes to json-config, or creating yet another conversion to a custom format for that case?

I can consider using library classes, but can you please describe how exactly that approach would make current serialization/deserialization easier?

Thanks:)

@johnny-keker johnny-keker changed the title Aliases for Target Hosts Aliases for Target Hosts [WIP] Nov 8, 2022
@johnny-keker
Copy link
Contributor Author

johnny-keker commented Nov 8, 2022

⚠️ Found out that old configs are not converted to a new format by default for now, so user gets error and proceeds with default config. This is definitely show stopper and I will fix that ASAP ⚠️

Marked as WIP This PR is currently work in progress until fix

@johnny-keker johnny-keker added the WIP This PR is currently work in progress label Nov 8, 2022
@johnny-keker johnny-keker changed the title Aliases for Target Hosts [WIP] Aliases for Target Hosts Nov 8, 2022
@johnny-keker johnny-keker removed the WIP This PR is currently work in progress label Nov 9, 2022
@johnny-keker
Copy link
Contributor Author

warning Found out that old configs are not converted to a new format by default for now, so user gets error and proceeds with default config. This is definitely show stopper and I will fix that ASAP warning

Marked as WIP This PR is currently work in progress until fix

Fixed in 8084564

Remove WIP This PR is currently work in progress , ready to review

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.

Add IP and Port aliases for Remote host dropdown
2 participants