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

Use %appdata% on windows. #1437

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

JonathanILevi
Copy link
Contributor

When installing Empty Epsilon in the system Program Files folder (like #1003 would) it requires administrator privileges to write to the local folder. Using the appdata folder also makes options.ini and such user specific, which is a good thing.

I have not tested this, if someone who already has a Windows development environment setup would do so, I would appreciate it.

@gcask
Copy link
Contributor

gcask commented May 19, 2021

This is incomplete btw - there are references to HOME in src/menus/optionsMenu.cpp as well as src/scriptDataStorage.cpp that needs to be fixed up too (and tested).

if (getenv("HOME"))
PreferencesManager::load(string(getenv("HOME")) + "/.emptyepsilon/options.ini");
else
PreferencesManager::load("options.ini");
if (getenv("APPDATA"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of additional branches, it would probably make more sense to swap HOME for APPDATA on windows.

Better yet - store the result of getenv(<home or appdata>) once, and use that throughout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, copypasta... HOME is also not a thing on Windows either... I just extended the current solution rather than re-implementing a better one.

#ifdef _WIN32
mkdir((string(getenv("APPDATA")) + "/emptyepsilon").c_str());
#else
mkdir((string(getenv("APPDATA")) + "/emptyepsilon").c_str(), S_IRWXU | S_IRWXG | S_IROTH | S_IXOTH);
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like happy-go-lucky copypasta: which platform is this supposed to handle?

I don't think APPDATA really is a thing to worry about on anything but windows :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, copypasta is what it was. A more eloquent solution would be ideal.

@JonathanILevi
Copy link
Contributor Author

This is incomplete btw - there are references to HOME in src/menus/optionsMenu.cpp as well as src/scriptDataStorage.cpp that needs to be fixed up too (and tested).

Shoot, I got lazy and didn't look, I can add those.

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.

2 participants