Opened 20 years ago
Closed 20 years ago
Last modified 4 years ago
#8194 closed patch
Quit Confirmation Dialog (feature request #642721)
|Reported by:||Deledrius||Owned by:||fingolfin|
Here's a patch to add an optional exit confirmation dialog (off by default). This is my first time using this system, so I hope I performed and upload the diff correctly.
Ticket imported from: #715991. Ticket imported from: patches/299.
Change History (15)
by , 20 years ago
comment:1 by , 20 years ago
Ignore the first one. It had a minor typo in it, and I don't seem to have permission to remove my own submission.
comment:2 by , 20 years ago
Please submit patches in th unified diff format ("-u" option).
This patch will have to be resubmitted, in theunnified format and against latest CVS. The problem is that it won't apply cleanly; worse since it contains no context information, patch will write it into semi- random places instead of erroring out.
I don't think a command line option ("-i" in the patch) makes much sense here. IMHO it's sufficient if this is in the config file (and in the future in the in-game prefs dialog).
comment:3 by , 20 years ago
comment:4 by , 20 years ago
Okay, here it is with -u against today's CVS. The version of WinCVS I was using didn't have the option before, but I've since updated.
I've also removed the -i command line code. I wasn't sure if it was needed before, and only provided it in the name of completeness. Given the current shortage of letters, I agree it's not very useful.
I hope this diff is properly usable now. I also cleaned up a place or two in the code I submitted before which deviated slightly from the coding standards.
by , 20 years ago
Patch against 2003-05-03 CVS to add Exit Confirmation
comment:5 by , 20 years ago
Some problem with that patch:
* there is no need for your custom getResult methon in the dialog. We already implement such a feature, look at Diaog::setResult() and the return value of runModal()
* for consistency with the rest of the code, _confirm_on_exit should be renamed to something like _confirmExit
* don't call shutDown. Nor are TODO's refering to shutDown appropriate.
* you modified the SDL backend... and you map the Mac Cmd-Q to Alt-X. Well, OK, but the code only seems to handle Ctrl-Z for quit, so that doesn't do anything. My suggestion: handle Alt-X not Ctrl-z in scummvm.cpp; and also modify the code in sdl-common.cpp to always "fake" an alt-x keyboard even, even for Ctrl-z. Of course the proper solution on the long run might be a new event like "quit requested event"
comment:6 by , 20 years ago
|Status:||new → pending|
comment:7 by , 20 years ago
This patch seems to bitrot :-(
comment:8 by , 20 years ago
|Status:||pending → new|
comment:9 by , 20 years ago
Here ya go. Hope it's all done right this time. Sorry about the delay.
* Fixed this. Didn't see the return value from runDialog and runModal.
* Fixed this. There seemed to be a few inconsistencies that confused the matter when I was deciding what to use.
* Didn't see what was wrong with this, but consider it fixed.
Since there's now a quit event and shutdown works, it's moot. That makes things easier, too...
* Didn't even need to touch this now. Since all quit-request key-combos return a quit event to the game loop, I no longer had to fudge one in. :-)
BTW, I never had a problem with ctrl-z not working on mine.
Maybe I missed something with the version I diffed. Oh well, I know *this* one works. Let me know if you have any more comments on this.
by , 20 years ago
Patch against 2003-07-21 CVS Snapshot to add Exit Confirmation
comment:10 by , 20 years ago
Yes, this looks much much better. Not sure if I can get this in before 0.5.0, but once that is release we can get it in.
Some things that I still think could be tweaked (but which I can also do once it's checked in):
* use 0 and 1 for setResult, not 'y' and 'n' -> simplifies code a little, and is "language agnostic (i.e. maybe one day we want to localize that dialog to non-english languages)
* why not use a 'real' dialog with a "yes" and a "no" button". Buttons can have keyboard shortcuts, after all ('y' and 'n' in this case). The setResult call then would be inside a handleCommand method
Again, those are more cosmetical changes, and can be done later :-)
comment:11 by , 20 years ago
|Status:||new → closed|
comment:12 by , 4 years ago
Patch against 2003-04-05 CVS to add Exit Confirmation