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

When loading save file, state, and screenshot directories, attempt to… #16421

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Nargash
Copy link
Contributor

@Nargash Nargash commented Apr 7, 2024

… create them if they do not exist.

Guidelines

  1. Rebase before opening a pull request
  2. If you are sending several unrelated fixes or features, use a branch and a separate pull request for each
  3. If possible try squashing everything in a single commit. This is particularly beneficial in the case of feature merges since it allows easy bisecting when a problem arises

Description

In configuration.c, when checking the directory of the save state, save file, or screenshot paths either at initial program load or when applying overrides, we just fall back to a default if the directory is invalid or does not exist. This change makes it so we first attempt to create the directory with path_mkdir first. If that fails then we continue to log a warning and fall back to defaults. From my testing, path_mkdir respects filesystem permissions so we will not create a unwritable directory (it will fail and we go back to the default just like current behavior).

This will make the directory behavior more consistent since currently we do automatically create a Core or Content Directory parent folder when using the options to sort by content directory or core name.

I also updated the warning messages to tell the user where we are falling back to.

Related Issues

Could not find any

Related Pull Requests

N/A

Reviewers

[If possible @mention all the people that should review your pull request]

@Nargash Nargash marked this pull request as draft April 7, 2024 20:01
@Nargash Nargash marked this pull request as ready for review April 7, 2024 22:05
@LibretroAdmin
Copy link
Contributor

LibretroAdmin commented Apr 8, 2024

I'm not a fan of implicitly creating directories like this though. I think that should never be forced like this in this way, or at least not with mkdir function invocations littered all over the place.

@Nargash
Copy link
Contributor Author

Nargash commented Apr 9, 2024

Thank you for the feedback. Would you prefer I back out the mkdir changes and just submit the warning message changes, or is this whole PR not needed at all?

@RobLoach
Copy link
Member

Yeah, just the warnings should be good. It's likely the installer that should set up those directories/configurations rather than the application itself.

configuration.c Outdated Show resolved Hide resolved
configuration.c Outdated Show resolved Hide resolved
configuration.c Outdated Show resolved Hide resolved
configuration.c Outdated Show resolved Hide resolved
configuration.c Outdated Show resolved Hide resolved
Copy link
Member

@RobLoach RobLoach left a comment

Choose a reason for hiding this comment

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

Made inline suggestions for just the message updates.

Just notify user of the fall back directory.

Co-authored-by: Rob Loach <robloach@gmail.com>
@Nargash
Copy link
Contributor Author

Nargash commented Apr 29, 2024

Yeah, just the warnings should be good. It's likely the installer that should set up those directories/configurations rather than the application itself.

Thank you! I have committed your changes (I hope, this is the first time I used the Github suggestions feature which is pretty cool). I agree it makes sense for the installer to handle directory creation, but users can change it later in the configs which is why I initially wanted to try to create the path if it does not exist. To give a little more context on why I initially wanted to make a folder if it didn't exist, it was due to me noticing that if you use setup to sort screenshots (or saves) by content directory, you need to go and create each folder yourself. Like if you put your screenshots in ~/Emulators/screenshots/, you need to create yourself. I wound up writing a python script to do that by scanning subfolders in my ROMs directory, but it'd be nice to avoid that step!

@Nargash Nargash requested a review from RobLoach May 5, 2024 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants