Page MenuHomeWildfire Games

Colorize Wonder Victory TimeNotification
ClosedPublic

Authored by elexis on Jan 4 2017, 2:14 PM.

Details

Reviewers
Imarok
Summary

The wonder victory message in the middle of the screen (a TimeNotification) displays %(player)s will have won in %(time)s.
It requires the player to check the diplomacy screen and check which player color that is, if the player assignments aren't memorized.
It would also be consistent with the rest of the GUI which colorizes usernames.

Test Plan
  1. Start a game with wondermode.
  2. Use the "gift from the gods" cheat for fast building and resources.
  3. Build a wonder.
  4. Use the developer overlay (Alt+D) to change the perspective to another player to see the string.
  5. Ensure that you're not getting spammed out with sprintf errors and seeing a colorized, translated string.

Second test:
Start a match with an allied AI player and send an attack request via the diplomacy dialog to get an answer from the bot, thus seeing whether it uses the new function correctly.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 379
Build 610: Vulcan BuildJenkins
Build 609: arc lint + arc unit

Event Timeline

elexis updated this revision to Diff 125.Jan 4 2017, 2:14 PM
elexis retitled this revision from to Colorize Wonder Victory TimeNotification.
elexis updated this object.
elexis edited the test plan for this revision. (Show Details)
elexis added a comment.Jan 4 2017, 2:15 PM

One might argue that doing the colorization in the simulation is weird, but it's not out of sync.

Vulcan added a subscriber: Vulcan.Jan 4 2017, 4:22 PM

Build is green

Updating workspaces.
Build (release)...
../../../source/gui/CChart.cpp:38:41: warning: unused parameter ‘Message’ [-Wunused-parameter]
 void CChart::HandleMessage(SGUIMessage& Message)
                                         ^
../../../source/lib/tex/tex_png.cpp: In member function ‘virtual Status TexCodecPng::encode(Tex*, DynArray*) const’:
../../../source/lib/tex/tex_png.cpp:309:9: warning: variable ‘ret’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]
  Status ret = ERR::FAIL;
         ^
Build (debug)...
../../../source/gui/CChart.cpp:38:41: warning: unused parameter ‘Message’ [-Wunused-parameter]
 void CChart::HandleMessage(SGUIMessage& Message)
                                         ^
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

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

Was wondeirng how the AI sent colorized chat messages. It is an exception in messages.js:

			// special case for formatting of player names which are transmitted as _player_num
			for (let param in message.parameters)
			{
				if (!param.startsWith("_player_"))
					continue;

				message.parameters[param] = colorizePlayernameByID(message.parameters[param]);
			}

We could do the same with time notifications, but I'm dubious. Wouldn't it be cleaner to add a colorizePlayernameByID() function to the AI and simulation context, so that every message could decide on it's own? There could be multiple playernames in the string as well.

In D36#1155, @elexis wrote:

We could do the same with time notifications, but I'm dubious. Wouldn't it be cleaner to add a colorizePlayernameByID() function to the AI and simulation context, so that every message could decide on it's own? There could be multiple playernames in the string as well.

Why not have the GUI colorize player names? (Looking for "player" or "player1" or the "_player_" parameters, that way the simulation doesn't need to know anything about GUI formatting.)

elexis planned changes to this revision.Jan 19 2017, 4:10 PM

Advantages of colorizing in the GUI / Thesis:

  • Formatting is the department of the GUI, not of the simulation.
  • because clients should be able to change their GUI without affecting the simulation

Disadvantage of colorizing in the GUI / Antithesis:

  • Hardcodes meaning in the GUI, while solely the simulation connotes the meaning of the passed notification.
  • Thus only players are colorized:
    • In some messages, we might not want to colorize the player.
    • In some messages, we might want to colorize multiple players.
    • In some messages, we might want to colorize something arbitrary.

Actual Use Cases:
Can't imagine anything else but playernames to be colorized, especially since that's already the case for chat messages.
Thus the second set of disadvantages doesn't come into play and we can feasibly colorize in the GUI (while paying with the first disadvantage).

elexis updated this revision to Diff 329.Jan 24 2017, 7:53 PM

Colorize all playernames in TimeNotifications, i.e. the parameters starting with _player_ in the GUI (only used for the wonder victory notification currently).

elexis edited the test plan for this revision. (Show Details)Jan 24 2017, 7:58 PM

(Notice this doesn't cause an Out-Of-Sync error, as the colorization method in the GUI only changes a CloneValueFromOtherContext of the serialized data returned from `GuiInterfaceCall.)

Build is green

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

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

Imarok requested changes to this revision.Jan 28 2017, 1:31 PM

else committable

binaries/data/mods/public/gui/session/messages.js
299

missing semicolon

This revision now requires changes to proceed.Jan 28 2017, 1:31 PM
elexis updated this revision to Diff 370.Jan 28 2017, 3:38 PM
elexis edited edge metadata.

Add missing semicolon.

Build is green

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

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

Imarok accepted this revision.Jan 28 2017, 8:50 PM
This revision is now accepted and ready to land.Jan 28 2017, 8:50 PM
elexis closed this revision.Jan 28 2017, 9:33 PM
elexis marked an inline comment as done.

Fixed by rP19181, thanks for the reviews!

elexis changed the visibility from "All Users" to "Public (No Login Required)".Jan 29 2017, 6:51 AM