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

Add stance filter to TooltipDescription; YAML descriptions #15144

Merged
merged 1 commit into from May 20, 2018

Conversation

Projects
None yet
4 participants
@matjaeck
Copy link
Contributor

matjaeck commented May 17, 2018

Provides a stance filter for TooltipDescription (see #14890) to allow multiple instances of the trait to show different descriptions. Intends to close #14453. Descriptions are inspired by @Smittytron suggestions.

tooltipstances

@matjaeck matjaeck changed the title Tooltipstances Add stance filter to TooltipDescription; YAML descriptions May 17, 2018

@Smittytron

This comment has been minimized.

Copy link
Contributor

Smittytron commented May 17, 2018

Can you squash fix-up commits?

public TooltipDescription(TooltipDescriptionInfo info)
: base(info) { }
readonly Actor self;
readonly TooltipDescriptionInfo info;

This comment has been minimized.

@GraionDilach

GraionDilach May 17, 2018

Contributor

Pretty sure you don't need this variable - ConditionalTrait already has the Info variable defined.

public TooltipDescription(TooltipDescriptionInfo info)
: base(info) { }
readonly Actor self;

This comment has been minimized.

@GraionDilach

GraionDilach May 17, 2018

Contributor

Too many empty lines - Travis will prolly spit this out aswell.

This comment has been minimized.

@GraionDilach

GraionDilach May 17, 2018

Contributor

This got fixed since.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented May 17, 2018

I have a feeling #10892 will crop up here as well though but demanding to fix that here would be really unfair.

@matjaeck

This comment has been minimized.

Copy link
Contributor Author

matjaeck commented May 17, 2018

Have problems to squash the commits. Would be helpful if someone could help out with bash commands.

Edit: Following the steps described here finally squashed the commits.

@Smittytron

This comment has been minimized.

Copy link
Contributor

Smittytron commented May 17, 2018

Assuming you have git extensions:
git rebase upstream/bleed -i
Change the word in front of the commits you want to squash from 'pick' to 'f' and save.
Then forcepush your branch with git push origin tooltipstances -f


public bool IsTooltipVisible(Player forPlayer) { return !IsTraitDisabled; }
public bool IsTooltipVisible(Player forplayer)

This comment has been minimized.

@abcdefg30

abcdefg30 May 17, 2018

Member

This wants to stay as forPlayer.

This comment has been minimized.

@matjaeck

matjaeck May 17, 2018

Author Contributor

Yes, typo. Updating soon.

if (!Info.ValidStances.HasStance(stance))
return false;

if (IsTraitDisabled)

This comment has been minimized.

@abcdefg30

abcdefg30 May 17, 2018

Member

I think it'd be better to move this check to the first line in the function, so we save the stance lookup if the trait is disabled.

This comment has been minimized.

@matjaeck

matjaeck May 17, 2018

Author Contributor

Will correct when I'm home.

This comment has been minimized.

@matjaeck

matjaeck May 18, 2018

Author Contributor

Should var also be moved?

@matjaeck

This comment has been minimized.

Copy link
Contributor Author

matjaeck commented May 18, 2018

Updated.

Edit: Sorry btw for so many fixups, I will try to improve code quality.


public bool IsTooltipVisible(Player forPlayer)
{
var stance = forPlayer.Stances[self.Owner];

This comment has been minimized.

@GraionDilach

GraionDilach May 18, 2018

Contributor

Yeah, move this after the TraitDisabled check as well.

This comment has been minimized.

@matjaeck

matjaeck May 18, 2018

Author Contributor

Updated.

@matjaeck

This comment has been minimized.

Copy link
Contributor Author

matjaeck commented May 18, 2018

Updated.

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

👍

@Smittytron
Copy link
Contributor

Smittytron left a comment

Works as advertised and LGTM.

@matjaeck

This comment has been minimized.

Copy link
Contributor Author

matjaeck commented May 20, 2018

Fixed formatting.

@abcdefg30 abcdefg30 merged commit 32c7869 into OpenRA:bleed May 20, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Member

abcdefg30 commented May 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.