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

Disallow Join button without IP address input #20272

Merged
merged 1 commit into from Sep 9, 2022

Conversation

The-Zen-Cat
Copy link
Contributor

@The-Zen-Cat The-Zen-Cat commented Sep 8, 2022

Closes #20234

Trying to join a server with an empty address crashes the game. This fix disallows pressing join button without ip address field input.

Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

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

If you write Closes #20234 in PR description, the issue will be automatically closed on PR merge. You don't need to write on the issue itself

@@ -39,6 +39,8 @@ public DirectConnectLogic(Widget widget, Action onExit, Action openLobby, Connec
portField.Text = text.Substring(last + 1);
}

panel.Get<ButtonWidget>("JOIN_BUTTON").IsDisabled = () => string.IsNullOrEmpty(ipField.Text);
Copy link
Member

Choose a reason for hiding this comment

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

It's more optimal to reuse the search

Suggested change
panel.Get<ButtonWidget>("JOIN_BUTTON").IsDisabled = () => string.IsNullOrEmpty(ipField.Text);
var joinButton = panel.Get<ButtonWidget>("JOIN_BUTTON");
joinButton.IsDisabled = () => string.IsNullOrEmpty(ipField.Text);

and reuse it on OnClick

@PunkPun
Copy link
Member

PunkPun commented Sep 8, 2022

Fixes #20234 also works, just without the brackets

@abcdefg30
Copy link
Member

Can you squash the commits, please?

Disallow join button without IP address Input

Closes OpenRA#20234

Trying to join a server with an empty address crashes the game. This fix disallows pressing join button without ip address field input.  Updated to reuse search for joinButton vs 2 separate calls to search.
Copy link
Contributor

@dragunoff dragunoff left a comment

Choose a reason for hiding this comment

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

LGTM 👍

However I think that while this works it has a problem with feedback - the user can't easily tell what is preventing them from pressing the button. It's not so evident in this example but in a more complex form where validation depends on several inputs (like the save map dialog in #20274) it may be difficult to tell where the errors are.

So we should give some feedback about what is wrong - for example in the tooltip text of the button. Perhaps an even better approach is to not disable the button, let the user press it and then highlight the invalid fields. In any case this is not something we need to solve now.

@PunkPun
Copy link
Member

PunkPun commented Sep 9, 2022

However I think that while this works it has a problem with feedback - the user can't easily tell what is preventing them from pressing the button.

We could highlight the empty fields

@PunkPun PunkPun merged commit 125a7b8 into OpenRA:bleed Sep 9, 2022
@PunkPun
Copy link
Member

PunkPun commented Sep 9, 2022

Changelog

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.

Trying to join a server with an empty address crashes the game
4 participants