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

Cleanup: Remove redundant implementation of TakeScreenshot #8224

Merged
merged 3 commits into from Jun 27, 2020

Conversation

techgeeknz
Copy link
Contributor

@techgeeknz techgeeknz commented Jun 17, 2020

I don't think there's any reason we need two identical implementations of TakeScreenshot in toolbar_gui.cpp and screenshot_gui.cpp.

I also tried to make ScreenshotConfirmationCallback into a lambda; alas, my C++ wizardry is nowhere near that level yet.

This also includes @abmyii's commit a83bd03, to fix #8232

src/screenshot_gui.cpp Outdated Show resolved Hide resolved
src/screenshot_gui.cpp Outdated Show resolved Hide resolved
src/screenshot_gui.cpp Outdated Show resolved Hide resolved
src/screenshot.cpp Outdated Show resolved Hide resolved
src/screenshot_gui.cpp Outdated Show resolved Hide resolved
src/screenshot.cpp Outdated Show resolved Hide resolved
src/screenshot.cpp Show resolved Hide resolved
@techgeeknz
Copy link
Contributor Author

If you're happy with everything now, I'll squash it all into a single commit.

LordAro
LordAro previously approved these changes Jun 19, 2020
@Eddi-z
Copy link
Contributor

Eddi-z commented Jun 19, 2020

I'm sorry to burst in here this late, but what exactly is the difference between "MakeScreenshot" and "TakeScreenshot"?

@techgeeknz
Copy link
Contributor Author

I'm sorry to burst in here this late, but what exactly is the difference between "MakeScreenshot" and "TakeScreenshot"?

MakeScreenshot just goes ahead and does it; TakeScreenshot will ask if you're really sure about creating that 5 gigapixel screenshot before it proceeds to bring your computer to it's knees.

I didn't write them; I'm just refactoring them. If you have ideas for better names, now is the time to do it.

@LordAro
Copy link
Member

LordAro commented Jun 19, 2020

MakeScreenshotWithConfirm ;)

@techgeeknz
Copy link
Contributor Author

techgeeknz commented Jun 19, 2020

MakeScreenshotWithConfirm ;)

I'm partial to ComfirmTakeScreenshot and TakeScreenshot myself, but am open to compromise and want this merged before it causes any more trouble; so MakeScreenshotWithConfirm and MakeScreenshot it is 😉.

@techgeeknz
Copy link
Contributor Author

Pulled in @abmyii's fix for #8232

@LordAro LordAro merged commit 8a655c7 into OpenTTD:master Jun 27, 2020
@techgeeknz techgeeknz deleted the redundant_screenshot branch June 28, 2020 00:16
@techgeeknz
Copy link
Contributor Author

Thank you, @LordAro

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.

"Huge screenshot" warning incorrect for heightmap / minimap screenshots
4 participants