Opened 9 years ago

Closed 9 years ago

Last modified 7 months ago

#4772 closed defect (fixed)

Gob: ScummVM quits on pause

Reported by: Templier Owned by: lordhoto
Priority: normal Component: GUI
Keywords: Cc:
Game:

Description

When pressing CTRL+p, ScummVM exits with an error: "Could not load widget position for 'PauseDialog'"

Traced to the following code: PauseDialog::PauseDialog() : GUI::Dialog("PauseDialog") in gob.cpp:87
which will cause the error to be thrown in GuiObject::reflowLayout() because PauseDialog is not a valid dialog name.

This is working properly in 1.0, but not with the trunk.

Ticket imported from: #2954286. Ticket imported from: bugs/4772.

Attachments (1)

gob-construct_pause_dialog_using_coordinates_instead_of_name.patch (484 bytes) - added by Templier 9 years ago.
Use the other constructor to create the Pause Dialog

Download all attachments as: .zip

Change History (9)

comment:1 Changed 9 years ago by eriktorbjorn

That error message seems to come from the standard GuiObject::reflowLayout() function. Which strikes me as a bit odd, because it's a virtual function and the gob engine's PauseDialog class defines its own.

Apparently, when the dialog's constructor runs, and it eventually calls GuiObject's constructor, it calls reflowLayout() (that line of code is new), which is the one in GuiObject, not in PauseDialog. So apparently I either don't understand virtual functions as well as I hoped, or I'm missing something glaringly obvious.

comment:2 Changed 9 years ago by wjp

Calling virtual functions from the constructor causes this behaviour because the vtable likely isn't yet initialized fully. (And even if it was, it would call the subclass' methods before the subclass' constructor has executed, which likely isn't desired.)

Changed 9 years ago by Templier

Use the other constructor to create the Pause Dialog

comment:3 Changed 9 years ago by Templier

If the dialog name isn't used anywhere else (and it doesn't seem to be), we can just call the other constructor with "default" coordinates, the same way the Mohawk or scumm engines do.

The one line patch fixes the problem for me, but since I don't know much about the GUI subsystem yet, somebody with better knowledge might want to check for side effects.

comment:4 Changed 9 years ago by lordhoto

wjp: Actually calling the virtual method is not the problem over here. It should be perfectly fine, when just GuiObject::reflowLayout is called, due to the way our widget sizes are calculated. (And it was never meant to call the subclasses reflowLayout at that point :-).

Littleboy's patch looks perfectly fine, that's how it should be done in case Gob's dialog auto-calculates it's size like SCUMM does. If that isn't the case we also have to add auto-calculation like in SCUMM...

Assigning to DrMcCoy so he can take care of that.

comment:5 Changed 9 years ago by lordhoto

Just to explain a little more: using a "fake" dialog name is not correct, a dialog name should always match an entry in the theme file. Thus even if the Gob's code uses it's own reflowLayout to do auto calculation, it should either call the coordinates taking constructor, use a real dialog name described in the theme file or use an empty dialog name.

Though I can understand that you might want to call this "messy" ;-).

comment:6 Changed 9 years ago by lordhoto

Ok I fixed it myself now :-P.

comment:7 Changed 9 years ago by lordhoto

Owner: set to lordhoto
Resolution: fixed
Status: newclosed

comment:8 Changed 7 months ago by digitall

Component: GUI
Note: See TracTickets for help on using tickets.