Page MenuHomeWildfire Games

Inform player that second market is too close to setup trade route and use disabled action
ClosedPublic

Authored by Silier on Sep 11 2019, 9:18 PM.

Details

Reviewers
elexis
Freagarach
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP23297: Inform player that second market is too close to setup trade route
Summary

We have no way how to tell difference between not possible action and action that cannot be currently done because conditions are not met.
Adding parameter disabled. When that set true, action is considered even when actioninfo returns possible false.

Garrison questions:
Should be garrison disabled behaviour reverted and to allow execute garrison command even when garrisonholder is full ?

Market problem:
Player is not allowed to set trade route between two markets if gain is 0.
But player is not informed why he cannot do that.

This is adding useful information for the player.

Test Plan

Build 2 markets close to each other.
Set one as origin.
Hover over second and confirm tooltip is displayed.
Confirm that after clicking to that market trade route is not set.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/unit_actions.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/unit_actions.js
| 579| 579| 
| 580| 580| 			case "set second":
| 581| 581| 				if (tradingDetails.gain.traderGain == 0) // markets too close
| 582|    |-					tooltip = translate("Market is too close to first one.\nRight-click to cancel setting of trading route.")
|    | 582|+					tooltip = translate("Market is too close to first one.\nRight-click to cancel setting of trading route.");
| 583| 583| 				else
| 584| 584| 					tooltip = translate("Right-click to set as destination trade market.") + "\n" +
| 585| 585| 						sprintf(translate("Gain: %(gain)s"), {

binaries/data/mods/public/gui/session/unit_actions.js
| 557| »   »   »   switch·(tradingDetails.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/unit_actions.js
| 582| »   »   »   »   »   tooltip·=·translate("Market·is·too·close·to·first·one.\nRight-click·to·cancel·setting·of·trading·route.")
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section cli...

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

Freagarach added inline comments.
binaries/data/mods/public/gui/session/unit_actions.js
593 ↗(On Diff #9719)

But it still returns true then?

Silier added inline comments.Sep 12 2019, 7:50 AM
binaries/data/mods/public/gui/session/unit_actions.js
593 ↗(On Diff #9719)

yes, esle it would not display.
its similar to trying set the same market twice, it will not do it but to inform player it returns true

Stan added a subscriber: Stan.Sep 12 2019, 7:53 AM
Stan added inline comments.
binaries/data/mods/public/gui/session/unit_actions.js
582 ↗(On Diff #9719)

I believe you can remove 'setting of'

Freagarach added inline comments.Sep 12 2019, 7:54 AM
binaries/data/mods/public/gui/session/unit_actions.js
593 ↗(On Diff #9719)

Okay, but what happens if you return "possible": false here? The icon/tooltip do not show up at all?

Stan added inline comments.Nov 14 2019, 10:17 AM
binaries/data/mods/public/gui/session/unit_actions.js
582 ↗(On Diff #9719)

"Market is too close to the first one.\n Right-click to cancel."

Imarok added a subscriber: Imarok.Nov 14 2019, 11:17 AM

Nice. Maybe print Market is too close to first one in red?
(not sure, if that is possible)

Silier planned changes to this revision.Nov 14 2019, 9:22 PM

@Imarok like this ?

Silier updated this revision to Diff 10327.Nov 14 2019, 9:39 PM
Silier added a reviewer: Restricted Owners Package.

fix phrasing, use red colour so it is easier recognised as something wrong by player

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/578/display/redirect

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

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/session/unit_actions.js
| 557| »   »   »   switch·(tradingDetails.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Executing section cli...

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

Silier updated this revision to Diff 10328.EditedNov 14 2019, 10:03 PM

use setStringTags instead coloredText refering D2151
comment from @Freagarach on irc 20:40 < Freagarach> Angen: D2151. (http://irclogs.wildfiregames.com/2019-11/2019-11-14-QuakeNet-%230ad-dev.log)

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/579/display/redirect

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/unit_actions.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/unit_actions.js
| 579| 579| 
| 580| 580| 			case "set second":
| 581| 581| 				if (tradingDetails.gain.traderGain == 0)
| 582|    |-					tooltip = translate(setStringTags("Market is too close to first one.", {"color": "255 0 0"}) + "\nRight-click to cancel.");
|    | 582|+					tooltip = translate(setStringTags("Market is too close to first one.", { "color": "255 0 0"}) + "\nRight-click to cancel.");
| 583| 583| 				else
| 584| 584| 					tooltip = translate("Right-click to set as destination trade market.") + "\n" +
| 585| 585| 						sprintf(translate("Gain: %(gain)s"), {
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/unit_actions.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/unit_actions.js
| 579| 579| 
| 580| 580| 			case "set second":
| 581| 581| 				if (tradingDetails.gain.traderGain == 0)
| 582|    |-					tooltip = translate(setStringTags("Market is too close to first one.", {"color": "255 0 0"}) + "\nRight-click to cancel.");
|    | 582|+					tooltip = translate(setStringTags("Market is too close to first one.", {"color": "255 0 0" }) + "\nRight-click to cancel.");
| 583| 583| 				else
| 584| 584| 					tooltip = translate("Right-click to set as destination trade market.") + "\n" +
| 585| 585| 						sprintf(translate("Gain: %(gain)s"), {

binaries/data/mods/public/gui/session/unit_actions.js
| 557| »   »   »   switch·(tradingDetails.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Executing section cli...

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

Silier updated this revision to Diff 10482.Dec 6 2019, 9:53 AM

fix spaces around { and }

Vulcan added a comment.Dec 6 2019, 9:55 AM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/687/display/redirect

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

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/session/unit_actions.js
| 557| »   »   »   switch·(tradingDetails.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Executing section cli...

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

Silier updated this revision to Diff 10483.Dec 6 2019, 10:17 AM
Silier added a subscriber: bb.

switch setStringTags and translate, thnx @bb for noticing

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/688/display/redirect

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

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/session/unit_actions.js
| 557| »   »   »   switch·(tradingDetails.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Executing section cli...

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

nani added a subscriber: nani.Dec 7 2019, 8:32 PM

The code looks good but I wonder if the player should have the possibility of trading even if the gain is zero.

Silier updated this revision to Diff 10521.Dec 7 2019, 8:40 PM

rephrasing

Vulcan added a comment.Dec 7 2019, 8:46 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/session/unit_actions.js
| 566| »   »   »   switch·(tradingDetails.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Executing section cli...

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

Vulcan added a comment.Dec 7 2019, 8:53 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/710/display/redirect

Freagarach requested changes to this revision.Dec 8 2019, 8:56 AM

Works for a trader, but I think a similar message ought to be shown when trying to place the rally point of a market (L907+).

This revision now requires changes to proceed.Dec 8 2019, 8:56 AM
Silier updated this revision to Diff 10532.Dec 8 2019, 2:23 PM
Silier retitled this revision from Inform player that second market is too close to setup trade route to Allow disabled tooltip actions and Inform player that second market is too close to setup trade route.
Silier edited the summary of this revision. (Show Details)
Vulcan added a comment.Dec 8 2019, 2:25 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/717/display/redirect

Vulcan added a comment.Dec 8 2019, 2:27 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/session/unit_actions.js
| 570| »   »   »   switch·(tradingDetails.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Executing section cli...

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

Silier updated this revision to Diff 10539.Dec 8 2019, 7:06 PM
Silier edited the summary of this revision. (Show Details)

Had some walk so I eliminated something not needed.

Vulcan added a comment.Dec 8 2019, 7:08 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/720/display/redirect

Vulcan added a comment.Dec 8 2019, 7:10 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/session/unit_actions.js
| 569| »   »   »   switch·(tradingDetails.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Executing section cli...

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

Stan added a comment.Dec 8 2019, 8:06 PM

Disabled cursor. There is a txt file with it. Not sure what the game use it for, but everyother icon has it so... Also don't forget to set EOL Native for that file.

Silier updated this revision to Diff 10794.Dec 27 2019, 11:52 AM
Silier retitled this revision from Allow disabled tooltip actions and Inform player that second market is too close to setup trade route to Inform player that second market is too close to setup trade route and use disabled action.

remove garrison change from this diff

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/892/display/redirect

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

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/session/unit_actions.js
| 568| »   »   »   switch·(tradingDetails.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Executing section cli...

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

The translated strings are good (in particular better than in the previous revisions). If it works reasonably no objections from my side.

binaries/data/mods/public/gui/session/unit_actions.js
13–14 ↗(On Diff #10794)

Color -> Tags as it may equally contain Font changes

592 ↗(On Diff #10794)

(I've seen value == 0 being changed to !value in d2470, not that it matters)

621 ↗(On Diff #10794)

(Not a fan of trailing commas, I've used them once and not seen any reward, but thats subjective)

986 ↗(On Diff #10794)

(Just wondering if there is a use case to distinguish undefined from null, I've never seen a use case where the distinction mattered, and not distinguishing falsy values allow for simpler falsy checks, or only distinguishing undefined from 0/emptystring, but doesn't matter)

Stan added inline comments.Dec 27 2019, 5:17 PM
binaries/data/mods/public/gui/session/unit_actions.js
621 ↗(On Diff #10794)
Silier added inline comments.Dec 27 2019, 5:40 PM
binaries/data/mods/public/gui/session/unit_actions.js
621 ↗(On Diff #10794)

because one does not have to edit that last line if he wants to add new propperty after that

986 ↗(On Diff #10794)

i used null because L391, havent look further

elexis added inline comments.Dec 27 2019, 6:05 PM
binaries/data/mods/public/gui/session/unit_actions.js
621 ↗(On Diff #10794)

The argument for not adding it is that a comma is a separator between two items, but the trailing comma does not separate two items, so it seems contradictory to that definition to me. (Its not the definition since the JS syntax allows it with an explicit exception.)

I didn't know we can just edit the CC, I could have won many arguments based on what the CC says if Id just edit it beforehand :p I don't mind how you write your patches with this not so relevant syntax change, but if you add something to the CC you also impose how other people should write their patches, so I suppose it necessitates consensus, no?

No issues found during testing, other than the very specific case described below.
When I have three markets, one in the center, two next besides those. And I accidentally set the middle one as the origin I cannot revert that (the outer two are too close to the middle, but far enough from eachother). The only way I found of freeing the trader is by deleting the middle market.

binaries/data/mods/public/gui/session/unit_actions.js
621 ↗(On Diff #10794)

Then L596 also ;)

Silier updated this revision to Diff 10805.Dec 27 2019, 9:14 PM

style consistent

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/901/display/redirect

Silier edited the summary of this revision. (Show Details)Dec 27 2019, 9:16 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/session/unit_actions.js
| 568| »   »   »   switch·(tradingDetails.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Executing section cli...

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

Silier updated this revision to Diff 10807.Dec 27 2019, 9:20 PM

sorry, forgot that one.

Yes, I know about that, it exists without this. I think it would be reasonable to handle it by clicking at origin market again so that looks for another patch.

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/902/display/redirect

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

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/session/unit_actions.js
| 568| »   »   »   switch·(tradingDetails.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Executing section cli...

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

Freagarach accepted this revision.Dec 28 2019, 8:29 AM
In D2285#105187, @Angen wrote:

I think it would be reasonable to handle it by clicking at origin market again so that looks for another patch.

Agreed (twice) ;)

  • Code looks good. Cannot be expanded nor cleaned
  • Works as advertised.
  • Could not break it in more ways than before this patch. (The thing I could break is out of scope for this patch.)
This revision is now accepted and ready to land.Dec 28 2019, 8:29 AM