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

Fix selling improvements for multiple cities at once #2484

Merged
merged 7 commits into from
Jan 5, 2025

Conversation

blabber
Copy link
Collaborator

@blabber blabber commented Jan 5, 2025

Closes #2483

Reported by clangd.
@blabber
Copy link
Collaborator Author

blabber commented Jan 5, 2025

I will keep this as a draft for now. I aim to factor out the selling of improvements into it's own function/method before merging it.

@blabber blabber force-pushed the feature/sell_multiple branch from f198525 to 07350f3 Compare January 5, 2025 11:42
Copy link
Contributor

@lmoureaux lmoureaux left a comment

Choose a reason for hiding this comment

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

I think it would make sense to unify this with eco_report::sell_buildings in view_economics.{h,cpp}.

client/views/view_cities.cpp Outdated Show resolved Hide resolved
client/views/view_cities.cpp Outdated Show resolved Hide resolved
The closure already captures the environment by value.
@blabber
Copy link
Collaborator Author

blabber commented Jan 5, 2025

I think it would make sense to unify this with eco_report::sell_buildings in view_economics.{h,cpp}.

Unifying the behavior? Showing a sell results dialog after selling the improvements from the cities view seems like a good idea, and I can do that. Unifying the code on the other hand...

@lmoureaux
Copy link
Contributor

Unifying code is probably better. The function sell_all_improvements in repodlgs_common.cpp could take a city list parameter (and be renamed accordingly), and maybe boilerplate for the "sell results" dialog could be shared as well. Not sure about the confirmation dialog since the message is slightly different: sell every Temple vs sell the Temple.

If you don't see how to do it we can open a refactoring issue.

@blabber
Copy link
Collaborator Author

blabber commented Jan 5, 2025

I followed your advice and unified the code used by both views to sell city improvements.

We could also generate the question and result dialog in this central function, but I am not sure if this is considered good practice, so I left it out for now. Let me know what you think.

Copy link
Contributor

@lmoureaux lmoureaux left a comment

Choose a reason for hiding this comment

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

Thanks! I think we can remove the draft flag (and then I'll want to check the functionality one last time).

@blabber blabber marked this pull request as ready for review January 5, 2025 21:32
@lmoureaux lmoureaux self-requested a review January 5, 2025 21:45
Copy link
Contributor

@lmoureaux lmoureaux left a comment

Choose a reason for hiding this comment

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

Small nitpick on the working, else ok!

client/views/view_cities.cpp Outdated Show resolved Hide resolved
@lmoureaux lmoureaux enabled auto-merge (rebase) January 5, 2025 22:05
The improvement will not be sold from every city the player owns, but
from every selected city.
auto-merge was automatically disabled January 5, 2025 22:15

Head branch was pushed to by a user without write access

@blabber blabber force-pushed the feature/sell_multiple branch from 3b63647 to c315ae8 Compare January 5, 2025 22:15
@lmoureaux lmoureaux enabled auto-merge (rebase) January 5, 2025 22:17
@lmoureaux lmoureaux merged commit 7a68485 into longturn:master Jan 5, 2025
20 checks passed
@blabber blabber deleted the feature/sell_multiple branch January 6, 2025 20:02
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.

Selling improvements from several cities at once does not work
2 participants