Opened 17 years ago

Closed 16 years ago

Last modified 13 months ago

#8194 closed patch

Quit Confirmation Dialog (feature request #642721)

Reported by: Deledrius Owned by: fingolfin
Priority: normal Component: GUI
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 17 years ago.
Patch against 2003-04-05 CVS to add Exit Confirmation
quitdialog.diff (6.2 KB ) - added by Deledrius 17 years ago.
Patch against 2003-05-03 CVS to add Exit Confirmation
quitdialog.2.diff (4.5 KB ) - added by Deledrius 16 years ago.
Patch against 2003-07-21 CVS Snapshot to add Exit Confirmation

Download all attachments as: .zip

Change History (15)

by Deledrius, 17 years ago

Attachment: confirm.diff added

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

comment:1 by Deledrius, 17 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, 17 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, 17 years ago

Owner: set to fingolfin

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

Attachment: quitdialog.diff added

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

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

Status: newpending

comment:7 by fingolfin, 16 years ago

This patch seems to bitrot :-(

comment:8 by Deledrius, 16 years ago

Status: pendingnew

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

Attachment: quitdialog.2.diff added

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

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

Status: newclosed

comment:12 by digitall, 13 months ago

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