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

BACKENDS: Use Common::U32String for OSystem::setWindowCaption #2588

Merged
merged 1 commit into from Nov 22, 2020

Conversation

@ccawley2011
Copy link
Member

@ccawley2011 ccawley2011 commented Nov 1, 2020

This is in draft mode until PR #2586 is merged.

@criezy
criezy approved these changes Nov 7, 2020
Copy link
Member

@criezy criezy left a comment

This looks like a good change to me.
I have a small doubt about the change in the sword2 engine though and added a comment about that.

Note: I also didn't look at the changes in the Android backend.

base/main.cpp Outdated Show resolved Hide resolved
@@ -674,7 +674,7 @@ void Sword2Engine::initializeFontResourceFlags() {
else
textLine = (char *)fetchTextLine(textFile, 54) + 2;

_system->setWindowCaption(textLine);
_system->setWindowCaption(Common::U32String(textLine));

This comment has been minimized.

@criezy

criezy Nov 7, 2020
Member

I am wondering if this is correct there. Do we know what encoding might be used in the resource? Assuming ASCII may not be right and maybe we need proper conversion?

By the way here is the title I get for the French version: Les Boucliers de Quetzalcoatl
So all the titles listed in the code, as well as this one, only contain ASCII characters. So maybe it does not matter.

This comment has been minimized.

@ccawley2011

ccawley2011 Nov 8, 2020
Author Member

As far as I can tell, setWindowCaption was specified to use ASCII in 188cdf9, and was changed to use ISO-8859-1 in bb28ed7, while the relevant code in the Sword2 engine predates both of those commits. As such, I would assume that the caption used here is always ASCII. Having said that though, it might be worth removing the use of setWindowCaption both here and in the icb engine, since those are the only two engines to use a custom window caption, and there doesn't seem to be much of an advantage to this compared to using the standard caption.

This comment has been minimized.

@criezy

criezy Nov 10, 2020
Member

Yes, it is probably fine to assume ASCII here since that is what was initially assumed.
Or indeed we could just remove this code and use the standard caption.

@ccawley2011 ccawley2011 force-pushed the ccawley2011:u32-caption branch from 2da6819 to fc5aa14 Nov 8, 2020
@ccawley2011 ccawley2011 force-pushed the ccawley2011:u32-caption branch from fc5aa14 to 4aabecc Nov 15, 2020
@ccawley2011 ccawley2011 marked this pull request as ready for review Nov 15, 2020
@criezy
criezy approved these changes Nov 22, 2020
Copy link
Member

@criezy criezy left a comment

Thank you. This looks good.

@criezy criezy merged commit 01b4432 into scummvm:master Nov 22, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deepcode-ci-bot Well done, no issues found!
Details
@ccawley2011 ccawley2011 deleted the ccawley2011:u32-caption branch Nov 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.