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

Fixed joint war. #346

Closed
wants to merge 2 commits into from
Closed

Conversation

pspjuth
Copy link
Contributor

@pspjuth pspjuth commented Jun 20, 2018

This makes it possible to get joint war and research agreements in vanilla version.
This adds back the initial dialog from unmodded trade screen.
It seems the deal manager refuses incomplete deals and thus the upfront dialog is needed.
This solution should work for R&F as well with Third Party and Casus Belli choices.

Added back the initial dialog from unmodded trade screen.
@pspjuth
Copy link
Contributor Author

pspjuth commented Jun 20, 2018

This fixes the last problem in vanilla I'm aware of.

Copy link
Contributor

@ihendriks ihendriks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me aside of the indentation of the one block.

Did we remove this code earlier ourselves or was this missed in the merging during the spring patch? Because it seems like we were missing a lot of this, which is pretty weird.

I'll test this ingame as well, see how that goes.

</Grid>
</Grid>
</Container>
</Stack>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from L258 until here the code is indented too much (2 spaces)

@ihendriks
Copy link
Contributor

I tested this on R&F and it works just fine. Didn't test in vanilla but I think if it works in R&F it'll work in vanilla just as well.

Didn't get to test the research agreement though, only joint war (with CB) and joining ongoing war (also with CB). Works both in joining an ongoing war yourself as well as asking AI to join yours.

@pspjuth
Copy link
Contributor Author

pspjuth commented Jun 23, 2018

I think since the Improved Deal Screen had a very different design, this popup was removed.
And that worked up to the last patch where it seems something changed in the deal manager.

@ihendriks
Copy link
Contributor

Looks good to me now.

And yeah, that does make the most sense. Just seemed like a lot of lua was missing as well, which seems weird to me, as it's usually commented out with a note instead of just removed. But maybe it's because it's an integration, and not native CQUI.

@ComanDanteSA ComanDanteSA mentioned this pull request Jul 12, 2018
14 tasks
@Azurency
Copy link
Owner

Azurency commented Jul 27, 2018

Thank you for your help 👍. However the popup was removed, and I'd like to keep it that way as it fits the orignal spirit of the improved deal screen better.
You guested it right, the code was removed by the original creator of this mod.
I just pushed a fix for the base game (e682907) and I'll test it for R&F when everything is merged together on the expansion1 branch.

@Azurency Azurency closed this Jul 27, 2018
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.

3 participants