Page MenuHomeWildfire Games

Automatic radius tooltip for ranged auras
ClosedPublic

Authored by fatherbushido on May 8 2017, 8:10 PM.

Details

Summary

Many of the range strings are reported on transifex and automatic ones could be good.
That information is now less relevant as we have range visualization but it still relevant (at least for structure tree).
I'll nuke the related string in another diff.

Test Plan

entity panel
struct tree
training panel
hero panel

Check if translation is done correctly.

I was getting headache with the arrow function so I write it as such but could be written without the explicit loop.

Gui formatting is bad too I guess.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

fatherbushido created this revision.May 8 2017, 8:10 PM
fatherbushido edited the test plan for this revision. (Show Details)
fatherbushido edited the summary of this revision. (Show Details)
elexis edited edge metadata.May 8 2017, 10:03 PM

Looks good. But similarly to that old spread tooltip proposal, we run into the issue that the line (for example for Boudica) becomes so long that the line will split to 2 lines.

So we end up with:

John Doe
Hero Aura: all champions in range +3 attack damage.
Range: 30 meters
Attack: 40 pierce, Range: 72 meters

but that is already the case with languages whose translation of the string is longer than the english one.

An alternative (which might avoid repeating this issue here) might be "Hero Aura: All champions within %(auraRange)s +3 attack damage".
This has the disadvantage that the number is embedded.

So at some point I guess we want to use sprintf for all modifyable template attributes.

Like "Hero Aura: +3 Pierce, Range: 30 meters, Affects: Champion" or "Some Hero: +40 Minimum Distance, Affects: Fortress".

But the proposal works on its own and won't have to be rewritten from scratch if we would implement the latter.

So only requesting indentation, Range, auraID, " ", unless someone else has some opinion too.

binaries/data/mods/public/gui/common/tooltips.js
626 ↗(On Diff #1771)

Should use the same identifier as in the GuiInterface / Templates.js. 3x auraID would be better than 1x aura 1x name and 1x auraID

634 ↗(On Diff #1771)

Agree with this simple check, because the user won't be interested in auras with 0 meters effect range until we would start doing things like changing the aura range with a tech or aura (which we can legitimately exclude at this point).

Missing indentation 1 tab for these 8 lines

636 ↗(On Diff #1771)

tooltip += " " + sprintf

637 ↗(On Diff #1771)

Just translateWithContext("aura", "Range:") ?
(All of these "Foo:" strings are annoying each time, but surprisingly still noone complained about this and we want to colorize that colon too.)

Vulcan added a subscriber: Vulcan.May 9 2017, 7:34 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1092/ for more details.

Adress elexis remarks

elexis accepted this revision.May 9 2017, 2:34 PM

Thanks for this great patch!
Tested again and works as it should and is complete.
We should remove cunobelin, catafalque, monument range strings after this patch.

binaries/data/mods/public/gui/common/tooltips.js
636 ↗(On Diff #1787)

Was wondering whether we should end with a period, but since the other tooltips don't do that, won't do it here either I guess

637 ↗(On Diff #1787)

Didn't mean to remove the font, but that's actually ok and addresses also that issue where the label appears in the next line.

This revision is now accepted and ready to land.May 9 2017, 2:34 PM
This revision was automatically updated to reflect the committed changes.
Vulcan added a comment.May 9 2017, 3:18 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1102/ for more details.