#8925 closed patch
ALL: Common load dialog
| Reported by: | bluegr | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | GUI |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
Hello
This is a first attempt at creating a common load dialog for all ScummVM engines.
It adds a new virtual class to Engine(), loadGameState(), which accepts a game slot and calls the corresponding engine's load state method. I've added some test code for the SAGA engine. The igor, queen and touche engines should support this correctly without any major changes (they already have a loadGameState method). I haven't modified the other engines in this patch.
I'm unsure if a new engine feature should be added, (e.g. kSupportsLoading) - it seems that engines that support kSupportsDirectLoad should be easy to change.
Currently, I've reused the code for the load dialog from the game launcher, though this could be moved to engines/dialogs.*
It should be pretty straightforward to add a common save dialog as well (though I haven't done it in this patch).
Thoughts/opinions are welcome :)
Ticket imported from: #2122869. Ticket imported from: patches/1030.
Attachments (5)
Change History (25)
by , 16 years ago
| Attachment: | common-load.patch added |
|---|
comment:1 by , 16 years ago
Looks nice, some remarks though:
- It's really good that you just use the load menu from launcher, but maybe it would be neat to move it away from gui/launcher.cpp/.h to engine/dialogs.cpp/.h then? - I would add a new method to Engine, which can be used to query if the engine is currently capable of loading a game or not. You already encountered the problem in the SAGA implementation it seems, so it is only natural to do so. - It should be defined whether the engine itself should takes care of displaying failure to the user or if the GMM does that. If we decide for the GMM, which would look more consistent for the user IMHO, we would check if we should rather add some error define, which Engine::loadGameState returns or if we should allow the GMM to query a error string. I would go with a error state define, which would allow states for say: "Invalid savegame version" (if applies, most engines should probably support older save games anyway), "Corrupted savegame", "I/O Failure".
Minor nitpicks: - The SAGA engine implementation looks ill-formatted
comment:2 by , 16 years ago
The direction you're moving is the Right Thing(TM). Thank you for your work.
o I would object moving gui code out of gui/ directory. In fact, the current situation that some code in engines/scumm depends on gui/themes/modern.ini is ugly. Another reason is that I really looking forward to have ScummVM localizable and in this case have all UI strings in a single place is a plus.
o However, I would move saveload dialog to something like gui/saveload.cpp
o I would also prefer to see how this approach will clean up scumm/dialogs.cpp. I.e. Filippos, please include that in another version of the patch.
o There is a question how we will bind Shift+F5 to original save/load dialog. Should each engine do that or should we try to standardize that as well?
Rest of Johannes' remarks are seconded including extra indentation in SAGA-specific code.
comment:3 by , 16 years ago
Actually I thought Fingolfin wanted to clean up scumm/dialogs.cpp after we have a uniform global save/load dialog and also have the possibillity to add engine specific buttons in the GMM. That's why I think we should rather first implement that and then start to get rid of scumm/dialogs.cpp. Anyway I would personally say that's a different topic (includes the point via Shift+F5 for original save/load menu), and it might be wise to talk about that on -devel and keep comments in this patch entry focused on the load menu patch.
Moving the save/load code to gui/saveload.cpp is of course nice too, but since the GMM and everything related was in engines/ I found it more natural to move it to this place, since it heavily depends on the engine code. But of course the arugmentation that all gui code should rather be in gui/ is reasonable.
comment:4 by , 16 years ago
Hi folks, sorry, not much time, and none to review the patch itself, but some surface thoughts:
* Localization should not depend on where code sits in. If done right, it should be the same effort to localize code in gui/ as in engines/scumm/. Anyway, let's worry about that if we ever get to implement localization, not before
* The F5 / Shift-F5 hotkeys are SCUMM specific. What I envision is that we introduce a new hotkey (currently, F6, but that's only a bad temporary one) which can be used to trigger the GMM in all engines. Done in such a way that ports with few buttons can always reach it easily (e.g. by adding a new EVENT_MAINMENU or something like that). However, this is indeed, as LordHoto already stated, peripheral to this patch and should be discussed elsewhere.
* And yeah, I first want the "shared" load dialog be done, and the ability to add extra custom buttons & dialogs (save, help, ...) to the GMM before actually switching SCUMM to those. I.e. separate patches. However, they can (and probably should) be developed simultaneously, of course. After all, the new dialog would have to prove itself capable of replacing the old SCUMM load dialog ;).
* That engines/scumm/dialogs.cpp relies on code in the theme sucks indeed... but this sucks because it's an annoying limitation of our theming code. It makes it very difficult to use dialogs in engines. Maybe once we have the new GUI code we could solve this properly by making it possible to separate dialog layouts into multiple files, one for each dialog? They could still be bundled in the single theme .zip file, of course. But for now, I don't see why gui/saveload.cpp would save us any work compared to engines/saveload.cpp or engines/dialogs.cpp. I'd just add it to the latter for now. It is super-easy to move the code to gui/saveload.cpp at a later point, should we choose so; but it's outside the scope of this patch, too.
* Johannes already said it, but: - we need error handling, and it should be done by the GMM code, too, so the Engine::load method must return an error value, to be added to common/error.h - it needs a "loading possible" (and in the future, "saving possible") query method - no, not all engines which support loading from commandline will necessarily be able to support loading during runtime, so it would make sense to have separate feature flags for those -
by , 16 years ago
| Attachment: | common-load_v2.patch added |
|---|
Common load/save game dialog, version 2 (WIP)
comment:5 by , 16 years ago
New version: - Added a "save" button to the GMM (still non-functional) - Added a virtual method canLoadGameState(), which is used to toggle the load button depending on if a game can be loaded currently (implemented in the SAGA engine) - Moved the setEnabled() methods for the save and load buttons to reflowLayout() instead of the GMM dialog constructor (cause the buttons weren't toggled properly) - Added still unused methods saveGameState() and canSaveGameState() - Added a new (still unused) method audioSettingsChanged(), which will be called from the AudioSettings dialog when it closes, so that the engine is aware that the audio settings have been changed from the GMM - Added 2 more engine features: kSupportsLoading and kSupportsSaving (which enable the load and save buttons respectively) - The saveGameState and loadGameState return integers now, which will be used as error codes (though I'm still unsure which errors to process - LordHoto's proposal seems quite good and sufficient) - Fixed spacing in the SAGA specific code (oops) - Adjusted the loadGameState() and saveGameState() in the queen and igor engines
Note that this is all still WIP, ofc :) File Added: common-load_v2.patch
comment:6 by , 16 years ago
Sorry, didn't have time yet to properly look at this yet, but let me assure you that I am interested in getting this patch done and included, and that I understand it is work in progress. Anyway, some quick comments:
> - Added still unused methods saveGameState() and canSaveGameState()
Can't you at least toggle the "Save" button on the latter?
> - Added a new (still unused) method audioSettingsChanged(), which will be > called from the AudioSettings dialog when it closes, so that the engine is > aware that the audio settings have been changed from the GMM
How does this differ from Engine::syncSoundSettings() ?
> > - Added 2 more engine features: kSupportsLoading and kSupportsSaving > (which enable the load and save buttons respectively)
Please rename to kSupportsLoadingDuringRuntime ? And then rename kSupportsDirectLoad to kSupportsLoadingDuringStartup.
Also, the corresponding button should be completely hidden (not just disabled) if the respective feature flag is not set.
comment:7 by , 16 years ago
Some remarks apart Max' comments:
- In MainMenuDialog::reflowLayout you're using Engine::hasFeature with integers instead of the enum values like 'kSupportsListSaves' etc. That's IMHO a pretty ugly way. - In my opinion 'canLoadGameState'/'canSaveGameState' is a bit misleading. To me it does sound like a general save/load functionality check, it doesn't really sound like a check for being able to load/save at this very moment.
comment:8 by , 16 years ago
Many thanks for your comments and feedback! They're very helpful and much appreciated
Here we go: - The "Save" button is already toggled (i.e. enabled or disabled) depending on canSaveGameState(), it's just non-functional - audioSettingsChanged() is actually the same as syncSoundSettings(), so it's not needed - As for the renaming of the engine features, yes they will make more sense that way - I'm not yet sure how to hide the buttons instead of disabling them, as the problem is that they take their Y coordinate from the modern theme (modern.ini), and from hardcoded values for the classic theme. I'll see what I can do - I'm aware of the ugliness with the ints in MainMenuDialog::reflowLayout, a problem inherited from GSoC. It seems that there are errors when including engines/metaengine.h, I'll see what the problem was - I can't think of any better names than 'canLoadGameState'/'canSaveGameState'. Does 'canLoadGameStateCurrently'/'canSaveGameStateCurrently' sound better or do you have any other proposals?
comment:9 by , 16 years ago
Actually I'm unsure of a better name myself yet, but at laest I think it would be *nice* to have it changed in the future. But for now it's pretty ok to keep the name IMHO, there are still more important things to worry about :-).
That problem with engines/metaengine.h is of course bad :-/. I hope you can get it fixed. I can't test it myself right now, but I guess it's some include order related problem maybe?
comment:10 by , 16 years ago
I fixed the problem with hasFeature in SVN. But I don't see a new patch attached here -- thebluegr, maybe you forgot to attach it?
As for "hiding" the buttons instead of disabling them: OK, I see the problem. Well, we can just add that to the TODO list and fix it later. Maybe the new GUI code by Tanoku will make this a tad easier... or not... no idea.
comment:11 by , 16 years ago
what about LoadGameStateDisabled and SaveGameStateDisabled or *Enabled respectively?
comment:12 by , 16 years ago
'canLoadGameStateCurrently'/'canSaveGameStateCurrently' sound fine to me.
Note that the latest patch is still missing. Thebluegr?
comment:13 by , 16 years ago
Fingolfin: Hiding buttons shouldn't be *that* hard, although atm I fail to see exactly how and when do you want to hide them. I'll look into it.
The patch, either way, looks like a merge nightmare. It would be nice to get it on the trunk as soon as it's stable enough so I can start fixing it on the new GUI branch.
comment:14 by , 16 years ago
Hello, sorry for not attaching the patch to the previous message, my mistake. I was quite busy during the weekend so I didn't have time to look into this, but I did some work on it today
I'm attaching version 3 of the patch (correctly this time!). Changes: - Renamed kSupportsDirectLoad -> kSupportsLoadingDuringStartup - Renamed canLoadGameState -> canLoadGameStateCurrently - Renamed canSaveGameState -> canSaveGameStateCurrently - Removed audioSettingsChanged() (does the same thing as syncSoundSettings) - Added proper engine feature enums in common/dialogs.cpp (thanks fingolfin!) - Added a new method setVisible() to the Widget class - Save/load buttons are now hidden if the engine doesn't support the relevant features (kSupportsListSaves and kSupportsLoadingDuringRuntime/kSupportsSavingDuringRuntime) - this uses the new setVisible method - Note that the above method is problematic: when the save/load button is hidden, there is a space left in its place (i.e. the Y coordinate of the next button remains unchanged). I'll try and find out a way to set it correctly - Updated gui/theme-config.cpp, so that the two new buttons work in the classic theme as well - Swapped save/load buttons, so that the save button is above and the load below File Added: common-load_v3.patch
by , 16 years ago
| Attachment: | common-load_v3b.patch added |
|---|
Common save/load game dialog, version 3 (with non-working code to shift button positions)
comment:15 by , 16 years ago
Adding an updated version of the patch In this version, I've attempted to shift the Y coordinates of the dialog buttons depending on the visibility of the save/load buttons. For some reason, it's not working (the Y coordinates of the buttons remain unchanged), so any feedback would be welcome :) File Added: common-load_v3b.patch
comment:16 by , 16 years ago
Thanks for the update. Don't bother trying to add code to hide buttons for now; I rather prefer we do this independently of this patch, at a later time.
Also, we are about to merge the GUI branch. So, you probably should wait till after that merge before putting further effort into this, as it will certainly break your patch partially.
comment:17 by , 16 years ago
Now that the GUI branch has been merged, this patch will have to be revised.
To emphasize what I wrote before: I would prefer to not have a hacked solution to hide the buttons for now, but rather just disable them for now, and then later we implement a proper solution -- maybe with some help by LordHoto and Tanoku.
by , 16 years ago
| Attachment: | common-load_v4.patch added |
|---|
Common save/load game dialog, version 4 (working with the current GUI code)
comment:18 by , 16 years ago
Updated the patch to work with the new GUI branch File Added: common-load_v4.patch
comment:19 by , 16 years ago
| Owner: | set to |
|---|---|
| Status: | new → closed |
comment:20 by , 6 years ago
| Component: | → GUI |
|---|
Common load game dialog