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

COMMON: Adding default iconspath functionality #4024

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

Conversation

Thunderforge
Copy link
Contributor

@Thunderforge Thunderforge commented Jun 20, 2022

Implements Feature Request #13538.

This PR adds a default value for the iconspath setting, allowing users to download icons without specifying one. It is generally in the "application support" folder or equivalent, as specified by the operating system guidelines.

As this is my first big change to the ScummVM codebase, feedback is welcome on how to polish this PR!

Copy link
Member

@sev- sev- left a comment

Good thing. Except I have couple notes regarding the API naming and some functionality

backends/platform/sdl/macosx/macosx.cpp Outdated Show resolved Hide resolved
backends/platform/sdl/macosx/macosx.h Show resolved Hide resolved
backends/platform/sdl/win32/win32.cpp Show resolved Hide resolved
@Thunderforge Thunderforge marked this pull request as ready for review Jun 21, 2022
@Thunderforge
Copy link
Contributor Author

@Thunderforge Thunderforge commented Jun 21, 2022

This PR sets default locations for macOS, Windows, and other POSIX platforms (i.e. Linux).

Are there others that I should include?

@Thunderforge Thunderforge requested a review from sev- Jun 21, 2022
@aquadran
Copy link
Member

@aquadran aquadran commented Jun 26, 2022

I think backends with unusually paths can be skipped to allow backend porter decide to add it or precise what path here

@lotharsm
Copy link
Member

@lotharsm lotharsm commented Jun 26, 2022

I agree to @aquadran. We should focus on "sane defaults" for the most common platforms since the other platforms are most likely so different that it should be up to the porters to decide.

@lotharsm
Copy link
Member

@lotharsm lotharsm commented Jun 26, 2022

I just tested in on Win32, works as advertised.

Copy link
Member

@lephilousophe lephilousophe left a comment

This works with my proposed suggestion. Else, it didn't prepend the HOME prefix.
This now match what is done in getDefaultLogFileName

return Common::String();
}

return iconsPath;
Copy link
Member

@lephilousophe lephilousophe Jun 26, 2022

Choose a reason for hiding this comment

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

Suggested change
return iconsPath;
return Common::String::format("%s/%s", prefix, iconsPath.c_str());

return Common::String();
}

const Common::String appSupportFolder = Common::String(prefix) + "/Library/Application Support/ScummVM";
Copy link
Member

@criezy criezy Jun 26, 2022

Choose a reason for hiding this comment

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

I would suggest to add a new function in macosx_wrapper.mm

Common::String getAppSupportPathMacOSX() {
	// See comments in getDesktopPathMacOSX() as we use the same methods
	NSArray *paths = NSSearchPathForDirectoriesInDomains(NSApplicationSupportDirectory, NSUserDomainMask, YES);
	if ([paths count] == 0)
		return Common::String();
	NSString *path = [paths objectAtIndex:0];
	if (path == nil)
		return Common::String();
	return Common::String([path fileSystemRepresentation]) + "/ScummVM";
}

Beside using the correct API to get the path, this will also be useful if we finally decide to migrate the savegames there.

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