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 tooltips to production statistics #12820 #13007

Merged
merged 1 commit into from Apr 18, 2017

Conversation

Projects
None yet
3 participants
@rob-v
Contributor

rob-v commented Mar 21, 2017

Tooltip with list of currently produced buildings and units.

obrazok

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Apr 2, 2017

Member

Can we please make this reuse the tooltip from the production palette (with the actor description, price, etc) on a per-icon basis?

Member

pchote commented Apr 2, 2017

Can we please make this reuse the tooltip from the production palette (with the actor description, price, etc) on a per-icon basis?

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 2, 2017

Contributor

Initially I wanted to do it per icon, but there is only 1 widget - ScrollItem for all icons so I couldn't reuse standard Widget tooltips. I need to check how it is done for prod. palette.
Isn't proposed solution better than per icon? - because without having to move mouse after a change, the tooltip is automatically updated (and implementation is simpler thanks to standard support).

Contributor

rob-v commented Apr 2, 2017

Initially I wanted to do it per icon, but there is only 1 widget - ScrollItem for all icons so I couldn't reuse standard Widget tooltips. I need to check how it is done for prod. palette.
Isn't proposed solution better than per icon? - because without having to move mouse after a change, the tooltip is automatically updated (and implementation is simpler thanks to standard support).

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Apr 2, 2017

Member

Isn't proposed solution better than per icon?

IMO it isn't, because this then behaves differently to other tooltips, which act on a single item. It also means that observers miss out on potentially useful information.

This will be even more noticable once we resurrect and merge #13023, making the production rows larger.

Member

pchote commented Apr 2, 2017

Isn't proposed solution better than per icon?

IMO it isn't, because this then behaves differently to other tooltips, which act on a single item. It also means that observers miss out on potentially useful information.

This will be even more noticable once we resurrect and merge #13023, making the production rows larger.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 3, 2017

Contributor

Updated - Production icons tooltips per icon:

obrazok

Contributor

rob-v commented Apr 3, 2017

Updated - Production icons tooltips per icon:

obrazok

@pchote

This looks good overall. Just a couple of requests:

@@ -220,6 +220,7 @@ ScrollItemWidget ProductionStats(Player player)
template.Get<ObserverProductionIconsWidget>("PRODUCTION_ICONS").GetPlayer = () => player;
template.Get<ObserverSupportPowerIconsWidget>("SUPPORT_POWER_ICONS").GetPlayer = () => player;
template.IgnoreChildMouseOver = false;

This comment has been minimized.

@pchote

pchote Apr 17, 2017

Member

Can you please set this in the yaml instead?

@pchote

pchote Apr 17, 2017

Member

Can you please set this in the yaml instead?

This comment has been minimized.

@rob-v

rob-v Apr 17, 2017

Contributor

This can't be currently set in yaml, because it is overriden in ScrollItemWidget(ScrollItemWidget other).

@rob-v

rob-v Apr 17, 2017

Contributor

This can't be currently set in yaml, because it is overriden in ScrollItemWidget(ScrollItemWidget other).

Show outdated Hide outdated OpenRA.Mods.Common/Widgets/Logic/Ingame/ProductionTooltipLogic.cs
Show outdated Hide outdated OpenRA.Mods.Common/Widgets/ObserverProductionIconsWidget.cs
Show outdated Hide outdated OpenRA.Mods.Common/Widgets/ObserverProductionIconsWidget.cs
@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 17, 2017

Contributor

Updated. Thanks for the review and all suggestions.

Contributor

rob-v commented Apr 17, 2017

Updated. Thanks for the review and all suggestions.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 17, 2017

Contributor

Good ideas, updated.

Contributor

rob-v commented Apr 17, 2017

Good ideas, updated.

@pchote

pchote approved these changes Apr 17, 2017

Looks great now, just noticed one last minor issue:

public override void Tick()
{
if (TooltipIcon != null && iconRects[lastIconIdx].Contains(Viewport.LastMousePos))
return;

This comment has been minimized.

@pchote

pchote Apr 17, 2017

Member

icons[i] might change under us, so it would be best to reassign ToolTipIcon before returning.

@pchote

pchote Apr 17, 2017

Member

icons[i] might change under us, so it would be best to reassign ToolTipIcon before returning.

This comment has been minimized.

@rob-v

rob-v Apr 17, 2017

Contributor

I think it is handled in ProductionTooltipLogic. It already checked for null Actor.

@rob-v

rob-v Apr 17, 2017

Contributor

I think it is handled in ProductionTooltipLogic. It already checked for null Actor.

This comment has been minimized.

@pchote

pchote Apr 17, 2017

Member

The issue is if in the last tick icon[lastIconIdx] was e.g. a barracks, but on this tick is now e.g. a power plant.

@pchote

pchote Apr 17, 2017

Member

The issue is if in the last tick icon[lastIconIdx] was e.g. a barracks, but on this tick is now e.g. a power plant.

This comment has been minimized.

@rob-v

rob-v Apr 17, 2017

Contributor

The TooltipIcon on position x is still the same - only Actor changes in Draw - null, barracks, power plant. ProductionTooltipLogic shows barracks, then shortly nothing, then power plant. At least my test confirms it.

@rob-v

rob-v Apr 17, 2017

Contributor

The TooltipIcon on position x is still the same - only Actor changes in Draw - null, barracks, power plant. ProductionTooltipLogic shows barracks, then shortly nothing, then power plant. At least my test confirms it.

This comment has been minimized.

@pchote

pchote Apr 17, 2017

Member

ah, you're of course right.

@pchote

pchote Apr 17, 2017

Member

ah, you're of course right.

@abcdefg30 abcdefg30 merged commit 8e6f6a3 into OpenRA:bleed Apr 18, 2017

2 checks passed

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

@rob-v rob-v deleted the rob-v:ProductionStatisticsTooltip branch Apr 18, 2017

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30
Member

abcdefg30 commented Apr 18, 2017

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