Page MenuHomeWildfire Games
Differential D2893

[gui] fix barter message string
ClosedPublic
Actions

Authored by Nescio on Jul 20 2020, 10:33 AM.

Details

Reviewers
Freagarach
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP23944: Fix the barter string which were in reversed order.
Summary

As pointed out by @psypherium in https://wildfiregames.com/forum/index.php?/topic/28522-barter-x-for-y-wording-is-reversed/

Test Plan

Check for completeness and correctness.

Diff Detail

Event Timeline

Nescio created this revision.Jul 20 2020, 10:33 AM
Owners added a subscriber: Restricted Owners Package.Jul 20 2020, 10:33 AM
Vulcan added a comment.Jul 20 2020, 10:38 AM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (dot-notation):
|    | ["sell"] is better written in dot notation.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Barter.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Barter.js
|  95|  95| 	amountsToSubtract[resourceToSell] = amount;
|  96|  96| 	if (cmpPlayer.TrySubtractResources(amountsToSubtract))
|  97|  97| 	{
|  98|    |-		var amountToAdd = Math.round(prices["sell"][resourceToSell] / prices["buy"][resourceToBuy] * amount);
|    |  98|+		var amountToAdd = Math.round(prices.sell[resourceToSell] / prices["buy"][resourceToBuy] * amount);
|  99|  99| 		cmpPlayer.AddResource(resourceToBuy, amountToAdd);
| 100| 100| 
| 101| 101| 		// Display chat message to observers.
|    | [NORMAL] ESLintBear (dot-notation):
|    | ["buy"] is better written in dot notation.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Barter.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Barter.js
|  95|  95| 	amountsToSubtract[resourceToSell] = amount;
|  96|  96| 	if (cmpPlayer.TrySubtractResources(amountsToSubtract))
|  97|  97| 	{
|  98|    |-		var amountToAdd = Math.round(prices["sell"][resourceToSell] / prices["buy"][resourceToBuy] * amount);
|    |  98|+		var amountToAdd = Math.round(prices["sell"][resourceToSell] / prices.buy[resourceToBuy] * amount);
|  99|  99| 		cmpPlayer.AddResource(resourceToBuy, amountToAdd);
| 100| 100| 
| 101| 101| 		// Display chat message to observers.

binaries/data/mods/public/simulation/components/Barter.js
|  98| »   »   var·amountToAdd·=·Math.round(prices["sell"][resourceToSell]·/·prices["buy"][resourceToBuy]·*·amount);
|    | [NORMAL] JSHintBear:
|    | ['sell'] is better written in dot notation.

binaries/data/mods/public/simulation/components/Barter.js
|  98| »   »   var·amountToAdd·=·Math.round(prices["sell"][resourceToSell]·/·prices["buy"][resourceToBuy]·*·amount);
|    | [NORMAL] JSHintBear:
|    | ['buy'] is better written in dot notation.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/messages.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/messages.js
| 339| 339| 	let notificationText =
| 340| 340| 		notification.instructions.reduce((instructions, item) =>
| 341| 341| 			instructions + (typeof item == "string" ? translate(item) : colorizeHotkey(translate(item.text), item.hotkey)),
| 342|    |-			"");
|    | 342|+		"");
| 343| 343| 
| 344| 344| 	Engine.GetGUIObjectByName("tutorialText").caption = g_TutorialMessages.concat(setStringTags(notificationText, g_TutorialNewMessageTags)).join("\n");
| 345| 345| 	g_TutorialMessages.push(notificationText);
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2712/display/redirect

Freagarach added a subscriber: Freagarach.Jul 20 2020, 10:56 AM
Freagarach added inline comments.
binaries/data/mods/public/gui/session/chat/ChatMessageFormatSimulation.js
38

You can just switch them here and keep the names, right?

Nescio added inline comments.Jul 20 2020, 11:00 AM
binaries/data/mods/public/gui/session/chat/ChatMessageFormatSimulation.js
38

Yes, I know, but while at it I decided to do a grep and fix the code names as well, because I prefer proper English.

Freagarach added inline comments.Jul 20 2020, 11:02 AM
binaries/data/mods/public/gui/session/chat/ChatMessageFormatSimulation.js
38

Understandable, however now this code is out of sync with the msg.

Harbormaster completed remote builds in B12507: Diff 12799.Jul 20 2020, 11:24 AM
Nescio added inline comments.Jul 20 2020, 11:25 AM
binaries/data/mods/public/gui/session/chat/ChatMessageFormatSimulation.js
38

Is it?

Freagarach added inline comments.Jul 20 2020, 11:33 AM
binaries/data/mods/public/gui/session/chat/ChatMessageFormatSimulation.js
38

Well, the message has: msg.resourceSold and the variable was amountsSold. Now the variable name is amountGiven and the message still is msg.resourceSold.

It is no big issue, IMHO. I just liked the sold and bought more ^^' However, no need to change if you think that is inferior.

Alternatively one could use resource(s)Sold/resource(s)Bought (or given/gained).

Nescio added inline comments.Jul 20 2020, 11:51 AM
binaries/data/mods/public/gui/session/chat/ChatMessageFormatSimulation.js
38

Isn't msg.resourceSold the resource type?

The problem is that bartering (anything for anything) is fundamentally different from buying and selling (to and from a single currency).
This is also why the string is currently wrong, it has the purchase (B for A), not the exchange (A for B) word order.
I renamed them to reduce ambiguity in the code.

I suppose I could also replace resourceSold and resourceBought with resourceGiven and resourceGained, if you think that's better.

Silier added a subscriber: Silier.Jul 20 2020, 11:58 AM

I think a/b would not be clear

Freagarach added a comment.Jul 20 2020, 12:05 PM
In D2893#124928, @Angen wrote:

I think a/b would not be clear

I did not mean to use the / in the string ^^ Sorry if that was unclear.

binaries/data/mods/public/gui/session/chat/ChatMessageFormatSimulation.js
38

The problem is that bartering (anything for anything) is fundamentally different from buying and selling (to and from a single currency).

Good to know, thanks :)

I suppose I could also replace resourceSold and resourceBought with resourceGiven and resourceGained, if you think that's better.

That seems to remove any ambiguity?

Nescio updated this revision to Diff 12811.Jul 20 2020, 12:40 PM
Vulcan added a comment.Jul 20 2020, 12:45 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (dot-notation):
|    | ["sell"] is better written in dot notation.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Barter.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Barter.js
|  95|  95| 	amountsToSubtract[resourceToSell] = amount;
|  96|  96| 	if (cmpPlayer.TrySubtractResources(amountsToSubtract))
|  97|  97| 	{
|  98|    |-		var amountToAdd = Math.round(prices["sell"][resourceToSell] / prices["buy"][resourceToBuy] * amount);
|    |  98|+		var amountToAdd = Math.round(prices.sell[resourceToSell] / prices["buy"][resourceToBuy] * amount);
|  99|  99| 		cmpPlayer.AddResource(resourceToBuy, amountToAdd);
| 100| 100| 
| 101| 101| 		// Display chat message to observers.
|    | [NORMAL] ESLintBear (dot-notation):
|    | ["buy"] is better written in dot notation.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Barter.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Barter.js
|  95|  95| 	amountsToSubtract[resourceToSell] = amount;
|  96|  96| 	if (cmpPlayer.TrySubtractResources(amountsToSubtract))
|  97|  97| 	{
|  98|    |-		var amountToAdd = Math.round(prices["sell"][resourceToSell] / prices["buy"][resourceToBuy] * amount);
|    |  98|+		var amountToAdd = Math.round(prices["sell"][resourceToSell] / prices.buy[resourceToBuy] * amount);
|  99|  99| 		cmpPlayer.AddResource(resourceToBuy, amountToAdd);
| 100| 100| 
| 101| 101| 		// Display chat message to observers.

binaries/data/mods/public/simulation/components/Barter.js
|  98| »   »   var·amountToAdd·=·Math.round(prices["sell"][resourceToSell]·/·prices["buy"][resourceToBuy]·*·amount);
|    | [NORMAL] JSHintBear:
|    | ['sell'] is better written in dot notation.

binaries/data/mods/public/simulation/components/Barter.js
|  98| »   »   var·amountToAdd·=·Math.round(prices["sell"][resourceToSell]·/·prices["buy"][resourceToBuy]·*·amount);
|    | [NORMAL] JSHintBear:
|    | ['buy'] is better written in dot notation.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/messages.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/messages.js
| 339| 339| 	let notificationText =
| 340| 340| 		notification.instructions.reduce((instructions, item) =>
| 341| 341| 			instructions + (typeof item == "string" ? translate(item) : colorizeHotkey(translate(item.text), item.hotkey)),
| 342|    |-			"");
|    | 342|+		"");
| 343| 343| 
| 344| 344| 	Engine.GetGUIObjectByName("tutorialText").caption = g_TutorialMessages.concat(setStringTags(notificationText, g_TutorialNewMessageTags)).join("\n");
| 345| 345| 	g_TutorialMessages.push(notificationText);
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2722/display/redirect

Harbormaster completed remote builds in B12519: Diff 12811.Jul 20 2020, 2:38 PM
Imarok added a subscriber: Imarok.Jul 22 2020, 11:31 AM
Imarok added inline comments.
binaries/data/mods/public/gui/session/chat/ChatMessageFormatSimulation.js
38

wasn't the idea of the whole diff to use a more clear wording?
Like %(player)s gave %(amountGiven)s for %(amountGained)s. or such?

Nescio added inline comments.Jul 22 2020, 12:39 PM
binaries/data/mods/public/gui/session/chat/ChatMessageFormatSimulation.js
38

@psypherium pointed out that the current wording is in the wrong order. The ambiguity is that the string uses the “barter” verb but the “purchase” word order. This patch addresses that.
It should not cause confusion for people with a proper understanding of English. And for those whose English is faulty, well, there are translations.

38

You're right e.g. %(player)s converted %(amountGiven)s into %(amountGained)s. or %(player)s gave %(amountGiven)s and gained %(amountGained)s. might be clearer in this particular string.
However, from a game design perspective it's important to use the same terminology for the same thing. E.g. refer to structures as structures, not buildings (D2429/rP23366) and refer to health as health, not health points, hit points, or HP (D2842/rP23863). That's also why we have a style guide.
What's happening here is barter, therefore we should use that verb; cf. template_structure_economic_market.xml has a Barter class and a <Tooltip> starting with "Barter resources."
While using give and gain in this string is linguistically correct, it would also be correct in e.g. tribute strings, which is a fundamentally different concept.

Imarok added inline comments.Jul 22 2020, 1:25 PM
binaries/data/mods/public/gui/session/chat/ChatMessageFormatSimulation.js
38

Yeah, true. So let's stay with barter. Using the correct word order makes it definitely better :)

Freagarach accepted this revision.Aug 6 2020, 7:31 AM
  • Correct.
  • Complete.
  • Works in-game.
This revision is now accepted and ready to land.Aug 6 2020, 7:31 AM
Freagarach mentioned this in rP23944: Fix the barter string which were in reversed order..Aug 7 2020, 9:35 AM
Freagarach added a commit: rP23944: Fix the barter string which were in reversed order..
Freagarach added a comment.EditedAug 7 2020, 9:39 AM

Sorry @Nescio, I forgot half the commit message ^^'
Could you please close?

Nescio closed this revision.Aug 10 2020, 10:46 AM

Revision Contents
Changeset List

PathSize
binaries/
data/
mods/
public/
gui/
session/
chat/
14 lines
8 lines
simulation/
components/
10 lines

Diff 12811

View Options

binaries/data/mods/public/gui/session/chat/ChatMessageFormatSimulation.js

Loading...
View Options

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

Loading...
View Options

binaries/data/mods/public/simulation/components/Barter.js

Loading...
Wildfire Games · Phabricator