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

STARK: Enable rough confirmation dialogs #1468

Merged
merged 9 commits into from Jul 1, 2018

Conversation

Projects
None yet
3 participants
@DouglasLiuGamer
Copy link
Collaborator

DouglasLiuGamer commented Jun 30, 2018

In the original TLJ, some confirmation dialogs will pop up in various points. It is suspected that they use the Window's dialog box, so right now I decided to just use ResidualVM's MessageDialog as a temporary replacement. Future development may consider improving this.

I only implement dialogs in the following scenarios:

  • Quit the game in the main menu
  • Quit the game through the "Quit" button in the game screen's top menu
  • Quit the game in the diary index menu
  • Load a save when the game is running
  • Save on an occupied slot

There are two things that I don't find a reasonable way to implement in ResidualVM:

  • Pop up a dialog when clicking the X of the application window. This requires changing files coming from ScummVM.
  • Show the saving screenshot when choosing whether to overwrite the old save. Just wired to do this in the current structure.

Also, although the "language.ini", which contains all the texts of messages with localization provided, is loaded, it is not used since it may contain non-Latin characters, which is not supported by ResidualVM's message dialog. I keep all the codes there to remain a room for future development.

@DouglasLiuGamer DouglasLiuGamer requested a review from bgK Jun 30, 2018

while (!tmp.eos() && !tmp.err()) {
line = tmp.readLine();
if (line.size() > 0 && line[0] == '-' && line[1] == '-') {
break;

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jun 30, 2018

Author Collaborator

The language.ini has a line "-- Norwegian --:", which is an unsupported format in the reading in INIFile, so it needs to be skipped here.

@Faalagorn

This comment has been minimized.

Copy link
Contributor

Faalagorn commented Jun 30, 2018

I remember the game to also having a windows-like menu bar you could access when clicking ALT (IIRC), that had some options to change there and a few debugging-like options as well. Does this belong to the same big or is it a different matter, and should it be implemented in the first place?

I can't find a screenshot of this and I'd have to install the game on Windows to test it though, so let me know if you want me to give more info!

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jun 30, 2018

@Faalagorn I tried my Steam version and nothing happened when I clicked ALT. There is a manual attached in the game which contains all the keyboard bindings and I don't see features related to what you described. Yes, can you give us more info if you can be more precise on this?

BTW, the keyboard binding is the next task planned.

@Faalagorn

This comment has been minimized.

Copy link
Contributor

Faalagorn commented Jun 30, 2018

@DouglasLiuGamer Yup, I'm very happy with keyboard binding being next, as I was missing them especially within dialogues :). I wanted to even make an issue before it was included in the Project Page, after noting the keybindings listed in manual do not work.

About the alt and the menu, I guess I'll have to dust off my laptop with Windows XP and test it out. I think it worked in Win7+ with the Steam version as well, but I honestly don't remember how to activate it – U thought of Alt, because that's the usual hotkey to bring down the menus, but maybe it was not the case. You can try launching the game with windowed mode in the meantime, in case it's easier to find it in windowed mode until I check it myself.

@bgK
Copy link
Member

bgK left a comment

This is looking pretty good! I've left a few minor comments.

Common::String line;
while (!tmp.eos() && !tmp.err()) {
line = tmp.readLine();
if (line.size() > 0 && line[0] == '-' && line[1] == '-') {

This comment has been minimized.

@bgK

bgK Jun 30, 2018

Member

This could read outside of the array bounds.

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jul 1, 2018

Author Collaborator

Oops. I remember I was thinking to type 2 there, what happened to me...


// Pre-process some of the texts
if (_texts.contains(kYes)) {
_texts[kYes].deleteChar(0);

This comment has been minimized.

@bgK

bgK Jun 30, 2018

Member

Maybe use Common::replace to remove the & characters in those strings to make it more clear what that pre-processing is, and not to make assumptions on where the button hotkey marker is in the string in the localized versions.


/** Acquire a message text by a given key */
Common::String getTextByKey(TextKey key) {
// TODO: Use texts read from language.ini

This comment has been minimized.

@bgK

bgK Jun 30, 2018

Member

Looks like not much is missing to complete this TODO.

};

/** Show a message dialog, return true when the left button is pressed, and false for the right */
bool alert(const Common::String &msg, TextKey leftBtnMsg = kYes, TextKey rightBtnMsg = kNo);

This comment has been minimized.

@bgK

bgK Jun 30, 2018

Member

The name alert is usually used for single button dialogs. This should probably be named confirm.


bool GameMessage::alert(const Common::String &msg, TextKey leftBtnMsg, TextKey rightBtnMsg) {
// TODO: Build the original game's confirmation dialog
GUI::MessageDialog alert(msg, getTextByKey(leftBtnMsg).c_str(), getTextByKey(rightBtnMsg).c_str());

This comment has been minimized.

@bgK

bgK Jun 30, 2018

Member

Having the display code here breaks the convention where Service classes are for data access / backend things. Perhaps it would be better if the display methods were in UserInterface.

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jul 1, 2018

One small thing: Will the game pop up the dialog when exiting the Book of Secrets? I don't have the Book of Secrets in my Steam version so I can't test it.

@DouglasLiuGamer DouglasLiuGamer force-pushed the DouglasLiuGamer:branch-box branch from 0c12d9c to 7df34a6 Jul 1, 2018

@bgK

This comment has been minimized.

Copy link
Member

bgK commented Jul 1, 2018

One small thing: Will the game pop up the dialog when exiting the Book of Secrets?

Yes, everything works exactly like when in the game levels (it's possible to save, ...)

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jul 1, 2018

@bgK Actually I mean exiting through clicking the right hand side of the book. Will that also pop up the dialog?

@bgK

This comment has been minimized.

Copy link
Member

bgK commented Jul 1, 2018

Actually I mean exiting through clicking the right hand side of the book. Will that also pop up the dialog?

Nope. It exits directly to the main menu.

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jul 1, 2018

@bgK If there's nothing left I think we can do the merging.

@bgK bgK merged commit 2c7b101 into residualvm:master Jul 1, 2018

1 check passed

default Build finished.
Details
@bgK

This comment has been minimized.

Copy link
Member

bgK commented Jul 1, 2018

All good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.