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

GUI: add optional 'alignment' parameter to 'MessageDialog', and SCI: GK2: if subtitles patch is missing, explain how to install it #2039

Merged
merged 2 commits into from Feb 9, 2020

Conversation

@ZvikaZ
Copy link
Contributor

ZvikaZ commented Feb 3, 2020

No description provided.

@@ -18922,6 +18939,8 @@ void ScriptPatcher::processScript(uint16 scriptNr, SciSpan<byte> scriptData) {
// Enable subtitle compatibility if a sync resource is present
if (g_sci->getResMan()->testResource(ResourceId(kResourceTypeSync, 10))) {
enablePatch(signatureTable, "subtitle patch compatibility");
} else {
suggestDownloadGK2SubTitlesPatch();

This comment has been minimized.

Copy link
@bluegr

bluegr Feb 4, 2020

Member

This should not be enforced for all users. The dialog should only be shown if the user has chosen to display "speech and subtitles" or "subtitles" in ScummVM's audio options

This comment has been minimized.

Copy link
@ZvikaZ

ZvikaZ Feb 4, 2020

Author Contributor

OK.
Fixed that.
But before committing and pushing - a question.

Currently, the "edit game" menu for GK2 has "speech and subtitles"/"subtitles"/"speech" choosing disabled (probably because in the past it wasn't relevant).
I have changed that in my global "options" menu to verify my fix.

I'd like to enable that option in the GK2 "edit game" menu.

Is it better to be done on a separate PR?
Or in this PR as a separate commit?
Or even do it all on one commit?

This comment has been minimized.

Copy link
@bluegr

bluegr Feb 4, 2020

Member

You need to check the "Override global audio settings" checkbox in order to override the defaults and change the audio settings for an individual game

This comment has been minimized.

Copy link
@ZvikaZ

ZvikaZ Feb 4, 2020

Author Contributor

yeah, I know...

I already fixed that.
It was because of GUIO_NOSUBTITLES in GK2 detection table.
I've removed it, and it's fine now.

The question is technical-aesthetical, whether it's preffered to have the GUIO_NOSUBTITLES removal as a different commit, or even as a different PR, or send now one commit with everything.

This comment has been minimized.

Copy link
@bluegr

bluegr Feb 4, 2020

Member

It would be best to end up with two commits, one in the common code, and one in the SCI engine

@ZvikaZ ZvikaZ requested a review from bluegr Feb 4, 2020
@ZvikaZ ZvikaZ force-pushed the ZvikaZ:gk2sub branch from e057485 to b50a511 Feb 5, 2020
@ZvikaZ

This comment has been minimized.

Copy link
Contributor Author

ZvikaZ commented Feb 5, 2020

Done.
Thanks.

@ZvikaZ ZvikaZ force-pushed the ZvikaZ:gk2sub branch from 7ebeefb to 0a00558 Feb 9, 2020
@ZvikaZ

This comment has been minimized.

Copy link
Contributor Author

ZvikaZ commented Feb 9, 2020

Removed yet another redundant line, and squashed commits

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Feb 9, 2020

Great work, thanks! Merging

@bluegr bluegr merged commit bf3a8df into scummvm:master Feb 9, 2020
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.