Page MenuHomeWildfire Games

Display tooltips for victory conditions
ClosedPublic

Authored by elexis on Apr 10 2017, 9:10 PM.

Details

Summary

Added tooltips for victory conditions and map types.

Test Plan
  1. Open game and go to host a game
  2. Test that tooltips are appeared

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 1461
Build 2310: Vulcan BuildJenkins

Event Timeline

vladislavbelov created this revision.Apr 10 2017, 9:10 PM
Vulcan added a subscriber: Vulcan.Apr 10 2017, 11:34 PM

Build is green

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

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

Sandarac added inline comments.
binaries/data/mods/public/gui/common/settings.js
189

"A map with a predefined landscape."

elexis requested changes to this revision.May 3 2017, 11:01 AM

The patch will look nicer after the rebase. g_Dropdowns will need a new optional item, for example "item_tooltips" below "labels" (should have JSdoc) and the function needs to be set in initGUIDropdown or updateGUIDropdown.

mapSize and mapFilter should have descriptions for the other dropdowns too, I can write them once this patch is in.

binaries/data/mods/public/gui/common/settings.js
189

"A map with a predefined landscape and number of players. Freely select the other gamesettings"?

195

This could be slightly more elaborate and can come up with some kind of advertizement.

"Create a unique map with a different resource distribution each time. Freely chose the number of players and teams." ?

200

(Could be more vivid, but then again our scenario maps aren't what they ought to be (specific stories) and are very similar to skimish maps.)

source/gui/CList.cpp
41

Tabs should only occur before the first non-whitespace character, I suggest to keep those 2 as, add L53 with tab and L57 without tab

252

Almost the entire block was copy & pasted from GUIM_MOUSE_PRESS_LEFT, better make a private helper member that returns the currently hovered item (moving stuff above the switch might work too but had the disadvantage of exposing variables to the other cases that are not used by those cases)

This revision now requires changes to proceed.May 3 2017, 11:01 AM
vladislavbelov edited edge metadata.
vladislavbelov marked 5 inline comments as done.

Removed duplications, added tooltips for resources.

Added early return.

Vulcan added a comment.May 5 2017, 2:45 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/1016/ for more details.

Vulcan added a comment.May 5 2017, 4:19 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/1018/ for more details.

elexis requested changes to this revision.May 5 2017, 9:24 PM

Patch is broken when hovering a player dropdown.

We should add more tooltips, for example we can have mapsize suggestions (more than normal will lag, smaller than that has too few space to expand for many players, etc.) and
population limit (that more than 150*8 will lag most likely).
But that can and probably should be done in a separate patch.

I've always been wondering whether it is a good choice to have that tooltip in the bottom left where noone looks.
It seems more useful to me to have them right below the cursor, no?
Removing all these tooltip_style="onscreenToolTip" in the XML suffices, but that should be done in a separate patch, if accepted.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
363

idx is an unlucky variable name, since that is used for the selected player in g_PlayerDropdowns.
So if we wanted to add this function to the playerdropdowns too, it would collide.
So hoveredIdx would be a better name IMO.

Why is there an out-of-bounds ceck for only one side? Not the case for the mapType, but the elements could change dynamically for other dropdowns.
Testing for g_MapTypes.Description[idx] checks for both boundaries and even yeilds shorter code, it could become g_MapTypes.Description[hoveredIdx] || translate("...")

591

Perhaps we even want that for the civs ("The Celts of the British Isles." in g_CivData)?

source/gui/CList.cpp
35–38

Either not change all these lines, or replace the tabs with exactly one space.
We don't want to align things because of the same issue you run into:
When adding one more item that is longer than the rest, every line has to be aligned differently again.

509

Thanks, looks much better without the duplication!

522

is the conversion needed?

524

same amount of tabs as the if ( then spaces.

source/gui/CList.h
1

*cough*

This revision now requires changes to proceed.May 5 2017, 9:24 PM
vladislavbelov edited edge metadata.
vladislavbelov marked 6 inline comments as done.

Removed spaces (noticed by @elexis), removed unnecessary cast, add a check for tooltips.

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/1022/ for more details.

elexis added inline comments.May 10 2017, 4:18 PM
binaries/data/mods/public/gui/common/settings.js
200

As you can see we can't change any setting with scenario maps

binaries/data/mods/public/gui/gamesetup/gamesetup.js
363

The OOB check was implemented, but the rename wasn't.

hoveredIdx is still be preferable for the given reasons IMO

464

Start -> Initial
value -> amount

Don't see the point of having this tooltip. We can see the number already, no need to duplicate it to the tooltip.

468

This would be the first time we have a colon on a separate line and the parenthesis isn't closed on a separate line either, do a grep -R sprintf` -A6 *`

The tooltip is pointless, it is self evident that the "Starting resources" selects the games starting resources (not sure about that apostroph), but since that tooltip exists already, we could keep it.

Better take that one information that isn't self evident from the two tooltips (the fact that this number applies to each resource) and move it to the tooltips property (independent of hoverIdx):

Selects the initial amount of each resource?

Or just reverting this hunk altogether.

elexis commandeered this revision.May 15 2017, 2:30 AM
elexis edited reviewers, added: vladislavbelov; removed: elexis.
elexis added inline comments.
binaries/data/mods/public/gui/gamesetup/gamesetup.js
353

This comment should mention that the tooltip can differ with the hovered index

386

All functions must get the same arguments for consistency, even if unused

462

return not needed, but looks better with it indeed

464

The || check does not work here since there might be/is an item with 0 resources

468

I confused g_StartingResources.Resources with g_StartingResources.Title, showing that info is actually useful.
(Still think noone reads the tooltips if they're buried in the bottom left corner)

496

The || check works here too

981

It's nice to avoid if statements, but in this case we do a function call for no reason if we don't have such a function. Better avoid that call IMO

source/gui/CList.cpp
35–38

This must be a separate commit, so that an error when changing the orderwouldnt have to be a bug in the hover patch

205

This code has the exact same code flow, so the split and early break is correct

213

This sends a GUIM_SETTINGS_UPDATED message upon each mouse move. But we only need one if the setting changed actually.
Guess that early return could be added to SetSetting in the first place, but that might have side-effects.

I'm adding early returns so that the setting is changed only upon actual change. The message still arrives twice, but it's not our code doing so.

523

scroll-bar -> scrollbar

524

(The way the for-loop does it, not the way this if statement does it, according to other people at least)

source/gui/CList.h
108

virtual

elexis updated this revision to Diff 1938.May 15 2017, 2:30 AM

see above

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/1207/ for more details.

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/1208/ for more details.

vladislavbelov accepted this revision.May 15 2017, 10:29 PM
This revision is now accepted and ready to land.May 15 2017, 10:29 PM

Notice a Setting can be settable by definition, a function would be better, but we don't support those. We have a similar issue with other COList patches that want to read something from JS without being able to set it.
Anyway, supporting to change the hoveredIdx setting (and thus highlighting a different element) could be added (so the current code is not wrong), but not useful anytime soon (so implementing it is not important).

This revision was automatically updated to reflect the committed changes.