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

Fix empty lobby faction tooltips. #13602

Merged
merged 1 commit into from Jul 8, 2017

Conversation

Projects
None yet
4 participants
@pchote
Member

pchote commented Jul 7, 2017

Fixes #13599.
Fixes #13567.

@pchote pchote added this to the Playtest featuring updated HitShapes milestone Jul 7, 2017

@@ -118,11 +118,17 @@ public SlotDropDownOption(string title, string order, Func<bool> selected)
flag.GetImageCollection = () => "flags";
flag.GetImageName = () => factionId;
var factionName = faction.Description.SubstringBefore("\\n", StringComparison.Ordinal);

This comment has been minimized.

@pchote

pchote Jul 8, 2017

Member

Note that SubstringBefore and SubstringAfter actually come from Stylecop, which is why we crash when trying to load the lobby (#13567).

@pchote

pchote Jul 8, 2017

Member

Note that SubstringBefore and SubstringAfter actually come from Stylecop, which is why we crash when trying to load the lobby (#13567).

@RoosterDragon

Maybe consider a helper method for extracting the faction name and description?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jul 8, 2017

Member

Fixed.

Member

pchote commented Jul 8, 2017

Fixed.

@rob-v

It works properly, but I suggest small improvements for possible future reusing of this function.

/// <summary>Splits a string into two parts on the first instance of a given token.</summary>
static Pair<string, string> SplitOnFirstToken(string input, string token = "\\n")
{
if (string.IsNullOrEmpty(input))

This comment has been minimized.

@rob-v

rob-v Jul 8, 2017

Contributor

IMO it would be more correct in general to return Pair("", null) for "" input. e.g for "A" we return Pair("A", null).

@rob-v

rob-v Jul 8, 2017

Contributor

IMO it would be more correct in general to return Pair("", null) for "" input. e.g for "A" we return Pair("A", null).

This comment has been minimized.

@pchote

pchote Jul 8, 2017

Member

This would break our tooltip display: we use null to represent "no tooltip" while "" is "tooltip with no text"

@pchote

pchote Jul 8, 2017

Member

This would break our tooltip display: we use null to represent "no tooltip" while "" is "tooltip with no text"

This comment has been minimized.

@rob-v

rob-v Jul 8, 2017

Contributor

Ok, though not clear to me. I guess not specifying Description means null. It isn't possible to specify "" in yaml so it would have an effect only when used from code. anyway ok for me to keep it, is surely simpler.

@rob-v

rob-v Jul 8, 2017

Contributor

Ok, though not clear to me. I guess not specifying Description means null. It isn't possible to specify "" in yaml so it would have an effect only when used from code. anyway ok for me to keep it, is surely simpler.

var split = input.IndexOf(token, StringComparison.Ordinal);
var first = split > 0 ? input.Substring(0, split) : input;
var second = split > 0 ? input.Substring(split + token.Length) : null;

This comment has been minimized.

@rob-v

rob-v Jul 8, 2017

Contributor

Not important currently but it works better with split >= 0.

@rob-v

rob-v Jul 8, 2017

Contributor

Not important currently but it works better with split >= 0.

@reaperrr reaperrr merged commit 6e145f9 into OpenRA:bleed Jul 8, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pchote pchote deleted the pchote:fix-empty-faction-tooltip branch Oct 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment