Bugs in statesave slot code. about mupen64plus HOT 12 CLOSED

Wouldn't it just be an increasing integer or a pointer that changes each time 
we make
a save?

It's not supposed to automatically increase, like a screenshot counter would.  
The
user should be in control - the idea is to select a slot (0 through 9), then 
any time
you do a save state or load state, it uses this slot.  That way you can save at
different points in the game and be able to back up if you missed something.

Actually, it looks like auto-increment save slot is meant to be an option for 
the
user. There's even a checkbox for it in the config dialog. During the gui/nogui
merge, there was a variable called autoinc_save_slot which I ended up putting 
into
main.c, but it was used differently in the gui and nogui version, so it's 
probably
not working correctly now. I was also going to review the design of that while
working on this issue.

Ok, I ran some tests on this tonight and I found the following:

-Without statesave slot autoincrement selected, I get files of the form <rom
name>.stX where X is the statesave slot.
-In the Emulation menu, there's a submenu called "Current save state" which has 
a
"Default" entry and then "Slot 1" through "Slot 9". If the "Default" statesave 
slot
is selected, the resulting statesave files are <rom name>.st0. If "Slot 1" is
selected, the files also have the .st0 suffix, "Slot 2" has the .st1 suffix, 
and so
on. Saving to and loading from the selected save slot works as expected.
-The configuration dialog has an option under the "General" tab called "Auto
increment save slot". If this option is checked, then when you take a 
statesave, the
slot is incremented, then the save is taken. Since the slot is incremented 
before the
save state and not after, there is the good side effect that hitting load will 
load
the state that you just saved. The GTK gui selected slot does not stay in sync 
with
the incrementing slot value. Also, the incrementing slot will go all the way up 
to
<rom name>.st9 before wrapping back to zero. This is actually 10 slots when the 
gui
only shows 9, so once the autoincrement wraps back to zero, the user can't 
specify
the last slot via the gui.
-If you try to load a statesave from an empty slot, you get an error dialog. If 
you
keep hitting load, more and more dialogs will pop up with the same message.

From this analysis, I plan to make the following changes:

1. Remove "Default" slot option and replace it with Slot 0. Slot 0 will be the
default selected slot.
2. GUI needs to update the save state slot on autoincrement.
3. See if there's a way to suppress the multiple dialogs for loading from an 
empty
slot (maybe making the dialog modal). Ideally, an on-screen message would popup
saying "Slot X Empty", but we don't have on-screen message support yet.

Let me know if there are other bugs I'm missing.

I also tested using the 0-9 keyboard hotkeys and the 0-9 LIRC keys and they 
seem to
be working correctly.

Just a minor comment on r313, 0-9 should be displayed like real Gtk 
accelerators 
(not mnemonics - I know I've previously confused the two) for the save slots. 

However, I'm a bit at a loss at how to a) do this without actually declaring 
them 
to be accelerators and binding the keys b) preventing this from effecting the 
filter like having p did for Pausing. 

Anyway... if you can address this, or since it involves no core stave state 
changes 
just tell me how to, I can make the other minor GUI changes to close issue 76.

The mnemonic is just so the user can quickly select the menu item without using 
the
mouse, so those can stay. But I disagree that 0-9 should be gtk accelerators 
because
of the same problem we ran into with pause. We still have SDL configured to 
trigger
on 0-9 to set the save slot. So as long as the SDL window has focus, 0-9 will 
change
the save slot. Adding them as gtk accelerators would make it so they will also 
work
if the gtk window has focus, but I feel taking away the user's ability to type 
0-9 is
worse than not having these accelerators.

Addressed issues 1 and 2 from comment 5 in r313, cleaned up slot autoincrement 
code
and implemented controller mapping support for incrementing the savestate slot 
in
r314. For issue 3 in comment 5, changing the dialog to modal did not fix the 
problem.
As noted before, the real solution is to use the on-screen display (issue 21) to
display this error message. Marking this issue fixed. Once we have on-screen 
display,
it will be trivial to add the necessary error messages.

I think I figured out how to make 0-9 behave correctly. Gtk has focus-in, 
focus-out, and focus-grab events. If we have these focus events toggle the save 
state accelerator group, it would let uses change state with 0-9 if the filter 
entry isn't selected and send the 0-9 characters to the entry if it is. Also 
when 
the state is changed and the GUI is visable it should send a statusbar message. 
I'll see if I can get the accelerator, focus, and other GUI changes together 
over 
the weekend.

k, fair enough.