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

@techgeeknz techgeeknz force-pushed the redundant_screenshot branch from 0d378b3 to c775f4b Jun 17, 2020
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
@techgeeknz techgeeknz force-pushed the redundant_screenshot branch from c6edb23 to 13619b1 Jun 18, 2020
@techgeeknz techgeeknz requested a review from LordAro Jun 18, 2020
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

@techgeeknz techgeeknz commented Jun 18, 2020

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

@techgeeknz techgeeknz force-pushed the redundant_screenshot branch from 9a5c097 to 5996149 Jun 18, 2020
@Eddi-z
Copy link
Contributor

@Eddi-z 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

@techgeeknz techgeeknz commented Jun 19, 2020

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 LordAro commented Jun 19, 2020

MakeScreenshotWithConfirm ;)

@techgeeknz
Copy link
Contributor Author

@techgeeknz 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

@techgeeknz techgeeknz commented Jun 26, 2020

Pulled in @abmyii's fix for #8232

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

@techgeeknz techgeeknz commented Jun 28, 2020

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
Linked issues

Successfully merging this pull request may close these issues.

4 participants