Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hotkeys: Implement "SelectNextSlotAndSaveState" #11782

Merged
merged 2 commits into from
Sep 9, 2024
Merged

Hotkeys: Implement "SelectNextSlotAndSaveState" #11782

merged 2 commits into from
Sep 9, 2024

Conversation

smasimar
Copy link
Contributor

@smasimar smasimar commented Sep 5, 2024

Flipped the order of Hotkey action "SaveStateAndSelectNextSlot" to "SelectNextSlotAndSaveState"

Description of Changes

Flipped the order of hotkey actions from
Creating a Save State -> Selecting Next Save Slot
to
Selecting Next Save Slot -> Creating a Save State

Rationale behind Changes

The intention behind this is to streamline the quicksave/quickload process, as with this change the LoadStateFromSlot action will load the most recent Save State instead of the oldest one.

@smasimar smasimar changed the title Flipped the order of "SaveStateAndSelectNextSlot" to "SelectNextSlotA… Flipped the order of Hotkey action "SaveStateAndSelectNextSlot" to "SelectNextSlotAndSaveState" Sep 5, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for submitting a contribution to PCSX2

As this is your first pull request, please be aware of the contributing guidelines.

Additionally, as per recent changes in GitHub Actions, your pull request will need to be approved by a maintainer before GitHub Actions can run against it. You can find more information about this change here.

Please be patient until this happens. In the meantime if you'd like to confirm the builds are passing, you have the option of opening a PR on your own fork, just make sure your fork's master branch is up to date!

@smasimar smasimar changed the title Flipped the order of Hotkey action "SaveStateAndSelectNextSlot" to "SelectNextSlotAndSaveState" Hotkeys: Flipped the order of Hotkey action "SaveStateAndSelectNextSlot" to "SelectNextSlotAndSaveState" Sep 5, 2024
@RedPanda4552
Copy link
Contributor

This basically boils down to not liking how other people use the hotkey, and switching it to make it work the way they don't want it to instead.

I'd much more prefer to see this added as a second hotkey with the distinction between them being the order of the actions taken. One for "Save State and Cycle Slot", and one for "Cycle Slot and Save State".

@smasimar
Copy link
Contributor Author

smasimar commented Sep 6, 2024

Fair. Is there any other place (like UI definitions) I will need to edit to add the button, or would adding another entry in this block in Hotkeys.cpp be enough?
Either way, I suppose this PR can be closed, and I will create a new one with suggested edits.

@F0bes
Copy link
Member

F0bes commented Sep 8, 2024

Fair. Is there any other place (like UI definitions) I will need to edit to add the button, or would adding another entry in this block in Hotkeys.cpp be enough? Either way, I suppose this PR can be closed, and I will create a new one with suggested edits.

An easy way to tell is to just search the entire project for SaveStateAndSelectNextSlot and see where it is used.

I took a peek and it looks like all of the necessary work for adding a hotkey is just in Hotkeys.cpp

You don't need to close this pr. If you're confident enough with git you can make your changes,git add ..., git commit --amend, and it will apply the changes onto the previous commit (your commit). Or you can checkout your branch, git reset --hard origin/master, and make a new commit. In both instances you will need to force push back to GitHub.
If none of that makes sense and you don't want to give it a try, you can for sure close the pr and make a new one :P

@smasimar
Copy link
Contributor Author

smasimar commented Sep 9, 2024

Thank you for your help and patience. I think I managed to cleanly reset/rebase/amend it. Does this look good now?

@F0bes F0bes changed the title Hotkeys: Flipped the order of Hotkey action "SaveStateAndSelectNextSlot" to "SelectNextSlotAndSaveState" Hotkeys: Implement "SelectNextSlotAndSaveState" Sep 9, 2024
@F0bes F0bes merged commit 2886f82 into PCSX2:master Sep 9, 2024
12 checks passed
@OctopusButtons
Copy link
Contributor

OctopusButtons commented Sep 11, 2024

This basically boils down to not liking how other people use the hotkey, and switching it to make it work the way they don't want it to instead.

I'd much more prefer to see this added as a second hotkey with the distinction between them being the order of the actions taken. One for "Save State and Cycle Slot", and one for "Cycle Slot and Save State".

It doesn't look like just a "preference" issue. With the new way, a person simply loads to latest state, because the latest state is the active selected slot. In the old method, a person has to manually cycle back a slot and then load...otherwise they'd load the oldest slot. Another way to put it is: one method focuses on saving/loading progress, while the other method focuses on conscious manual selection of slot to load.

What is the justification scenario for the older method? What is the opposite rationale to what the contributor described (load recent save instead of oldest)?

@RedPanda4552
Copy link
Contributor

This basically boils down to not liking how other people use the hotkey, and switching it to make it work the way they don't want it to instead.
I'd much more prefer to see this added as a second hotkey with the distinction between them being the order of the actions taken. One for "Save State and Cycle Slot", and one for "Cycle Slot and Save State".

It doesn't look like just a "preference" issue. With the new way, a person simply loads to latest state, because the latest state is the active selected slot. In the old method, a person has to manually cycle back a slot and then load...otherwise they'd load the oldest slot. Another way to put it is: one method focuses on saving/loading progress, while the other method focuses on conscious manual selection of slot to load.

What is the justification scenario for the older method? What is the opposite rationale to what the contributor described (load recent save instead of oldest)?

The rationale is that not everyone does it the way you do. All you are doing is confirming this is a matter of preference.

Saving then switching slots makes logical sense because you start in the first slot, save, then move to the next slot to prepare another save. At any point if you want to go back, you can hit load state and pick the slot you want to go back to.

Your use case is that you do not want to have to pick which save to go back to, you want it to always go back to the last save. In which case the new hotkey will solve this for you. Not sure what the issue is with having both options, and insistence that everyone do it your way instead...

@OctopusButtons
Copy link
Contributor

OctopusButtons commented Sep 13, 2024

I was thinking parsimony unless it was trivial to implement both and commit to both.

this is a matter of preference.

Saving then switching slots makes logical sense because you start in the first slot, save, then move to the next slot to prepare another save.

Both versions of the double-function hotkey take care of slotting the sequential saves, and saving, but the newer one also prepares the likely load while the old one doesn't. The newer method lets a person do the workflow with 2 two buttons rather than 3. And the player wants to quick-load the most recent save far more likely than the oldest. For those reasons it looks incorrect to call the difference only a matter of preference, and the author was right in thinking of real usage.

It is true that it's strange, on the surface, if the newer method skips past the first slot for the first save. The future probably holds hotkeys for Save to Oldest Slot and Load From Newest Slot.

@RedPanda4552
Copy link
Contributor

That's what I suggested, was adding both. That way we aren't forcing people to do it one way and not the other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants