Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't trade resources from other trades or city-states #5252

Merged
merged 5 commits into from
Sep 22, 2021

Conversation

SimonCeder
Copy link
Collaborator

Relates to #4602.
This PR disables "trading on" resources that you got from other trades, or from city states. They still show up in the trade list (so you don't forget that you have them), but with a quantity of 0 (unless you also have other sources).
Screenshot_2021-09-17_23-29-56

@yairm210
Copy link
Owner

This is really unclear to the user.
I think we should add "1 untradable from [cityStateName]" below the "Gems (0)" line"

@SimonCeder
Copy link
Collaborator Author

Something like this
Screenshot_2021-09-19_13-49-42
?

@xlenstra
Copy link
Collaborator

xlenstra commented Sep 19, 2021

Maybe "+1 from Trade" to make more clear this is an additional one?
Or possibly "+1 untradable copy" or something to make more clear that it can't be traded?
Just some ideas

@yairm210
Copy link
Owner

"untradable" is a good word to include, gets right to the point

@SimonCeder
Copy link
Collaborator Author

New buttons:
Screenshot_2021-09-19_20-50-29

@SomeTroglodyte
Copy link
Collaborator

NullPointerException at com.unciv.testing.TranslationTests.allConditionalsAreContainedInConditionalOrderTranslation

Somehow, this seems to be unrelated to resources, trades and city states...

@SimonCeder
Copy link
Collaborator Author

It's an error in master.
Provides [stats] [cityFilter] = Ger [amount] [resource] and equivalent in all translations.
Perhaps because I changed the strings around in #5237 ? And something went very wrong in generating new translation strings?

@SomeTroglodyte
Copy link
Collaborator

SomeTroglodyte commented Sep 19, 2021

Go ahead and debug it, I'm not going to, sorry, toying elsewhere. You know how to debug tests - if not, search through my scratchpad.

Actually, now see #5272

@xlenstra
Copy link
Collaborator

xlenstra commented Sep 19, 2021

It's an error in master.
Provides [stats] [cityFilter] = Ger [amount] [resource] and equivalent in all translations.
Perhaps because I changed the strings around in #5237 ? And something went very wrong in generating new translation strings?

I've fixed the problem in #5272.
Main problem was that "Provides [stats] [cityFilter]" and "Provides [amount] [resource]" had the same placeholder text and were thus indistinguishable to the TranslationFileWriter, confusing it. I've fixed it for now by readding "Provides [stats] [cityFilter] per turn", but feel free to implement a better solution for this.
Additionally, generating translation files somehow messed up the existing translations for conditionals making the other test fail, which is the other error here

@SomeTroglodyte
Copy link
Collaborator

SomeTroglodyte commented Sep 19, 2021

Well, "general solution" would mean a Unique would be identifiable out of context without relying on placehoder text being a primary key... Actually, TranslationFileWriter knows or could know some context, so having each Unique - in the enum perhaps - know the scope of application (not the scope of effect or scope of conditionals) - could enable TranslationFileWriter to distinguish such "uniques". Meaning another test should whine when two Uniques with identical placeholder text have overlapping scopes.

enum class Scope { BaseUnit, Building, Policy, Nation, Terrain,....... }
enum class UniqueTypeOrHowIsItCalledNow(val placeholderText: String, val scope: EnumSet<Scope> = EnumSet.noneOf(Scope::class.java)) {
    ProvidesStatsCity("Provides [] []", EnumSet.of(Scope.Building, Scope.Nation)),
    ProvidesResources("Provides [] []", EnumSet.of(Scope.Terrain, Scope.Whatever)),
    ....

@yairm210 yairm210 closed this Sep 20, 2021
@yairm210 yairm210 reopened this Sep 20, 2021
@ajustsomebody ajustsomebody mentioned this pull request Sep 22, 2021
26 tasks
@SimonCeder
Copy link
Collaborator Author

To be clear, I'm not working on any improvement to the translation writer.

@yairm210 yairm210 merged commit 0bb565f into yairm210:master Sep 22, 2021
@SimonCeder SimonCeder deleted the citystatetrade branch October 12, 2021 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants