Opened 22 years ago

Closed 21 years ago

Last modified 6 years ago

#8194 closed patch

Quit Confirmation Dialog (feature request #642721)

Reported by: Deledrius Owned by: fingolfin
Priority: normal Component: GUI
Version: Keywords:
Cc: Game:

Description

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.

Attachments (3)

confirm.diff (4.0 KB ) - added by Deledrius 22 years ago.
Patch against 2003-04-05 CVS to add Exit Confirmation
quitdialog.diff (6.2 KB ) - added by Deledrius 22 years ago.
Patch against 2003-05-03 CVS to add Exit Confirmation
quitdialog.2.diff (4.5 KB ) - added by Deledrius 21 years ago.
Patch against 2003-07-21 CVS Snapshot to add Exit Confirmation

Download all attachments as: .zip

Change History (15)

by Deledrius, 22 years ago

Attachment: confirm.diff added

Patch against 2003-04-05 CVS to add Exit Confirmation

comment:1 by Deledrius, 22 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 fingolfin, 22 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).

Thank you!

comment:3 by fingolfin, 22 years ago

Owner: set to fingolfin

comment:4 by Deledrius, 22 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 Deledrius, 22 years ago

Attachment: quitdialog.diff added

Patch against 2003-05-03 CVS to add Exit Confirmation

comment:5 by fingolfin, 22 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 fingolfin, 21 years ago

Status: newpending

comment:7 by fingolfin, 21 years ago

This patch seems to bitrot :-(

comment:8 by Deledrius, 21 years ago

Status: pendingnew

comment:9 by Deledrius, 21 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 Deledrius, 21 years ago

Attachment: quitdialog.2.diff added

Patch against 2003-07-21 CVS Snapshot to add Exit Confirmation

comment:10 by fingolfin, 21 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 fingolfin, 21 years ago

Status: newclosed

comment:12 by digitall, 6 years ago

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