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

OpenGL: Fix warning for hidden overladed virtual saveScreenshot() function #2924

Merged
merged 2 commits into from Apr 6, 2021

Conversation

@criezy
Copy link
Member

@criezy criezy commented Apr 6, 2021

This reverts commit 0f118d0.
Instead it attempt to fix the hiding overloaded virtual function warning in a cleaner way.

criezy added 2 commits Apr 6, 2021
This noisy warning at least occur when building for Android
(OpenGL, but not SDL).
criezy referenced this pull request Apr 6, 2021
Building for Android (OpenGL) had noisy warning for hiding overloaded virtual function

Added a dummy implementation in OpenGLGraphics manager, and also for OpenGLSdlGraphicsManager it now explicitly calls the SdlGraphicsManager implementation for the void argument signature.
@antoniou79
Copy link
Contributor

@antoniou79 antoniou79 commented Apr 6, 2021

This reverts commit 0f118d0.
Instead it attempt to fix the hiding overloaded virtual function warning in a cleaner way.

Indeed it fixes the warning and it's cleaner.
I kind of think my tentative fix was more symmetrical, but this is cleaner.

Still I don't think this addresses the mess/confusion with the two signatures and why we are keeping them separate in this manner, providing implementations for one and not the other, when one (the one with the file argument) can basically use the other (no argument signature) (or the other way around) in every (?) case.

@criezy
Copy link
Member Author

@criezy criezy commented Apr 6, 2021

when one (the one with the file argument) can basically use the other (no argument signature)

That's rather the other way around. The one without argument can call the one with argument passing the default path for a new screenshot file. The issue is that currently this "default path" is backend-dependent. In the SDL backend you have getScreenshotsPath(), but that is currently not available in non-SDL backends.

The main issue here is that the saveScreenshot that takes an argument is only really an implementation detail in the SDL backend. But it gets exposed to non-SDL backends as well. While on the other hand getScreenshotsPath() is only available in the SDL backend.

So I agree that this could probably be cleaned. But I don't see an obvious way to do it.

@antoniou79
Copy link
Contributor

@antoniou79 antoniou79 commented Apr 6, 2021

when one (the one with the file argument) can basically use the other (no argument signature)

That's rather the other way around. The one without argument can call the one with argument passing the default path for a new screenshot file. The issue is that currently this "default path" is backend-dependent. In the SDL backend you have getScreenshotsPath(), but that is currently not available in non-SDL backends.

Yes, that's correct. I've kind of rethought that sentence (hence the edit) and the typical approach is what you describe.

The main issue here is that the saveScreenshot that takes an argument is only really an implementation detail in the SDL backend. But it gets exposed to non-SDL backends as well. While on the other hand getScreenshotsPath() is only available in the SDL backend.

So I agree that this could probably be cleaned. But I don't see an obvious way to do it.

I completely understand. For now, this PR will do.

@criezy
Copy link
Member Author

@criezy criezy commented Apr 6, 2021

Thanks for confirming this fixes your warning.
I am merging this now and we can try to improve the code later.

@criezy criezy merged commit b6186cc into scummvm:master Apr 6, 2021
3 checks passed
3 checks passed
Codacy Static Code Analysis Codacy Static Code Analysis
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deepcode-ci-bot Well done, no issues found!
Details
@criezy criezy deleted the criezy:screenshot-warning branch Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants