Added tooltips for victory conditions and map types.
Details
Details
- Open game and go to host a game
- Test that tooltips are appeared
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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.
| binaries/data/mods/public/gui/common/settings.js | ||
|---|---|---|
| 189 ↗ | (On Diff #1206) | "A map with a predefined landscape." |
Comment Actions
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 ↗ | (On Diff #1206) | "A map with a predefined landscape and number of players. Freely select the other gamesettings"? |
| 195 ↗ | (On Diff #1206) | 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 ↗ | (On Diff #1206) | (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 | ||
| 56 ↗ | (On Diff #1206) | 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 |
| 276 ↗ | (On Diff #1206) | 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) |
Comment Actions
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.
Comment Actions
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.
Comment Actions
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 ↗ | (On Diff #1662) | idx is an unlucky variable name, since that is used for the selected player in g_PlayerDropdowns. 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. |
| 591 ↗ | (On Diff #1662) | Perhaps we even want that for the civs ("The Celts of the British Isles." in g_CivData)? |
| source/gui/CList.cpp | ||
| 35 ↗ | (On Diff #1662) | Either not change all these lines, or replace the tabs with exactly one space. |
| 508 ↗ | (On Diff #1662) | Thanks, looks much better without the duplication! |
| 521 ↗ | (On Diff #1662) | is the conversion needed? |
| 523 ↗ | (On Diff #1662) | same amount of tabs as the if ( then spaces. |
| source/gui/CList.h | ||
| 1 ↗ | (On Diff #1662) | *cough* |
Comment Actions
Removed spaces (noticed by @elexis), removed unnecessary cast, add a check for tooltips.
Comment Actions
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.
| binaries/data/mods/public/gui/common/settings.js | ||
|---|---|---|
| 200 ↗ | (On Diff #1206) | As you can see we can't change any setting with scenario maps |
| binaries/data/mods/public/gui/gamesetup/gamesetup.js | ||
| 464 ↗ | (On Diff #1668) | Start -> Initial Don't see the point of having this tooltip. We can see the number already, no need to duplicate it to the tooltip. |
| 468 ↗ | (On Diff #1668) | 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. |
| 363 ↗ | (On Diff #1662) | The OOB check was implemented, but the rename wasn't. hoveredIdx is still be preferable for the given reasons IMO |
| binaries/data/mods/public/gui/gamesetup/gamesetup.js | ||
|---|---|---|
| 353 ↗ | (On Diff #1668) | This comment should mention that the tooltip can differ with the hovered index |
| 386 ↗ | (On Diff #1668) | All functions must get the same arguments for consistency, even if unused |
| 462 ↗ | (On Diff #1668) | return not needed, but looks better with it indeed |
| 464 ↗ | (On Diff #1668) | The || check does not work here since there might be/is an item with 0 resources |
| 468 ↗ | (On Diff #1668) | I confused g_StartingResources.Resources with g_StartingResources.Title, showing that info is actually useful. |
| 496 ↗ | (On Diff #1668) | The || check works here too |
| 981 ↗ | (On Diff #1668) | 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 ↗ | (On Diff #1668) | This must be a separate commit, so that an error when changing the orderwouldnt have to be a bug in the hover patch |
| 211 ↗ | (On Diff #1668) | This code has the exact same code flow, so the split and early break is correct |
| 223 ↗ | (On Diff #1668) | This sends a GUIM_SETTINGS_UPDATED message upon each mouse move. But we only need one if the setting changed actually. 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. |
| 522 ↗ | (On Diff #1668) | scroll-bar -> scrollbar |
| 523 ↗ | (On Diff #1662) | (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 ↗ | (On Diff #1668) | virtual |
Comment Actions
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.
Comment Actions
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.
Comment Actions
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).