-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
ofToDataPath simplification #7368
base: master
Are you sure you want to change the base?
Changes from 8 commits
c96c60f
c199076
a8e677a
ce61301
9159896
130008a
c3914c2
27b6370
51b141d
e5e5ff2
9dc6022
679f6dd
d86685a
59db08d
0a11ede
90f344d
1a69450
026663d
b9e6299
7badb5f
ed78257
e403845
7249c12
c87c967
e6f7815
642bd80
bef7d34
f722555
15d7d91
fdba45b
e2dffe5
b5e8112
d0d04e2
d73a5bb
75e93af
da630d2
4126b05
155894e
14a4711
919656c
902f0ca
76e84bc
d2585db
36090e6
8561228
ad097db
ac6eeda
b0f6e88
2a4ac9d
c7e933f
fd7d7cc
42df8d5
ac54e24
c0aeaeb
5f038a8
b73badf
f507a92
8c1eca8
d742529
83a0d3f
9a8e995
9192592
e27babd
cb01825
7b52456
c646e2b
c6b18ee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,24 +26,23 @@ namespace{ | |
bool enableDataPath = true; | ||
|
||
//-------------------------------------------------- | ||
// MARK: - near future | ||
// of::filesystem::path defaultDataPath(){ | ||
std::string defaultDataPath(){ | ||
#if defined TARGET_OSX | ||
try{ | ||
return of::filesystem::canonical(ofFilePath::getCurrentExeDir() / of::filesystem::path("../../../data/")).string(); | ||
}catch(...){ | ||
return (ofFilePath::getCurrentExeDir() / of::filesystem::path("../../../data/")).string(); | ||
} | ||
#elif defined TARGET_ANDROID | ||
return string("sdcard/"); | ||
#else | ||
try{ | ||
return of::filesystem::canonical(ofFilePath::join(ofFilePath::getCurrentExeDir(), "data/")).make_preferred().string(); | ||
}catch(...){ | ||
return (ofFilePath::getCurrentExeDir() / of::filesystem::path("data/")).string(); | ||
} | ||
#endif | ||
of::filesystem::path defaultDataPath(){ | ||
#if defined TARGET_OSX | ||
|
||
// three levels up because is a directory with path App.app/Contents/MacOS/App | ||
of::filesystem::path data = ofFilePath::getCurrentExeDir().parent_path().parent_path().parent_path() / "data"; | ||
if (!of::filesystem::is_directory(data)) { | ||
// bundle data folder inside app | ||
// one level up : MacOS/../Resources/data | ||
data = ofFilePath::getCurrentExeDir().parent_path() / "Resources/data"; | ||
} | ||
return data; | ||
|
||
#elif defined TARGET_ANDROID | ||
return { "sdcard/" }; | ||
#else | ||
return (ofFilePath::getCurrentExeDir() / "data"); | ||
#endif | ||
} | ||
|
||
//-------------------------------------------------- | ||
|
@@ -1679,8 +1678,7 @@ string ofFilePath::addLeadingSlash(const of::filesystem::path& _path){ | |
} | ||
|
||
//------------------------------------------------------------------------------------------------------------ | ||
// MARK: - near future | ||
//of::filesystem::path ofFilePath::addTrailingSlash(const of::filesystem::path & _path){ | ||
// MARK: - Remove this function after FS transition | ||
std::string ofFilePath::addTrailingSlash(const of::filesystem::path & _path){ | ||
#if OF_USING_STD_FS && !OF_USE_EXPERIMENTAL_FS | ||
if(_path.string().empty()) return ""; | ||
|
@@ -1776,7 +1774,9 @@ std::string ofFilePath::getEnclosingDirectory(const of::filesystem::path & _file | |
if(bRelativeToData){ | ||
fp = ofToDataPath(fp); | ||
} | ||
return addTrailingSlash(fp.parent_path()); | ||
|
||
// return addTrailingSlash(fp.parent_path()); | ||
return fp.parent_path().string(); | ||
} | ||
|
||
//------------------------------------------------------------------------------------------------------------ | ||
|
@@ -1854,10 +1854,9 @@ string ofFilePath::getCurrentExePath(){ | |
} | ||
|
||
//------------------------------------------------------------------------------------------------------------ | ||
// MARK: - near future | ||
//of::filesystem::path ofFilePath::getCurrentExeDir(){ | ||
std::string ofFilePath::getCurrentExeDir(){ | ||
return ofFilePath::getEnclosingDirectory(ofFilePath::getCurrentExePath(), false); | ||
of::filesystem::path ofFilePath::getCurrentExeDir(){ | ||
auto exePath = of::filesystem::path(getCurrentExePath()); | ||
return exePath.parent_path(); | ||
} | ||
|
||
//------------------------------------------------------------------------------------------------------------ | ||
|
@@ -1927,22 +1926,9 @@ void ofSetDataPathRoot(const of::filesystem::path& newRoot){ | |
} | ||
|
||
//-------------------------------------------------- | ||
// MARK: - near future | ||
//of::filesystem::path ofToDataPath(const of::filesystem::path & path, bool makeAbsolute){ | ||
std::string ofToDataPath(const of::filesystem::path & path, bool makeAbsolute){ | ||
dimitre marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (makeAbsolute && path.is_absolute()) { | ||
// return path; | ||
return path.string(); | ||
} | ||
|
||
if (!enableDataPath) { | ||
// return path; | ||
return path.string(); | ||
} | ||
|
||
bool hasTrailingSlash = !path.empty() && path.generic_string().back()=='/'; | ||
|
||
// if our Current Working Directory has changed (e.g. file open dialog) | ||
// if our Current Working Directory has changed (e.g. file open dialog) | ||
#ifdef TARGET_WIN32 | ||
if (defaultWorkingDirectory() != of::filesystem::current_path()) { | ||
// change our cwd back to where it was on app load | ||
|
@@ -1952,72 +1938,18 @@ std::string ofToDataPath(const of::filesystem::path & path, bool makeAbsolute){ | |
} | ||
} | ||
#endif | ||
|
||
// this could be performed here, or wherever we might think we accidentally change the cwd, e.g. after file dialogs on windows | ||
// FIXME: change to direct returns when return type is fs::path | ||
of::filesystem::path outPath; | ||
const auto & dataPath = dataPathRoot(); | ||
of::filesystem::path inputPath(path); | ||
of::filesystem::path outputPath; | ||
|
||
// if path is already absolute, just return it | ||
if (inputPath.is_absolute()) { | ||
try { | ||
auto outpath = of::filesystem::canonical(inputPath).make_preferred(); | ||
if(of::filesystem::is_directory(outpath) && hasTrailingSlash){ | ||
return ofFilePath::addTrailingSlash(outpath); | ||
}else{ | ||
return outpath.string(); | ||
// return outpath; | ||
} | ||
} | ||
catch (...) { | ||
return inputPath.string(); | ||
// return inputPath; | ||
} | ||
} | ||
|
||
// here we check whether path already refers to the data folder by looking for common elements | ||
// if the path begins with the full contents of dataPathRoot then the data path has already been added | ||
// we compare inputPath.toString() rather that the input var path to ensure common formatting against dataPath.toString() | ||
auto dirDataPath = dataPath; | ||
// also, we strip the trailing slash from dataPath since `path` may be input as a file formatted path even if it is a folder (i.e. missing trailing slash) | ||
dirDataPath = ofFilePath::addTrailingSlash(dataPath); | ||
|
||
auto relativeDirDataPath = ofFilePath::makeRelative(of::filesystem::current_path(), dataPath); | ||
relativeDirDataPath = ofFilePath::addTrailingSlash(relativeDirDataPath); | ||
|
||
// FIXME: this can be simplified without using string conversion | ||
// if (inputPath.string().find(dirDataPath.string()) != 0 && inputPath.string().find(relativeDirDataPath.string())!=0) { | ||
if (inputPath.string().find(dirDataPath.string()) != 0 && inputPath.string().find(relativeDirDataPath)!=0) { | ||
// inputPath doesn't contain data path already, so we build the output path as the inputPath relative to the dataPath | ||
if(makeAbsolute){ | ||
outputPath = dirDataPath / inputPath; | ||
}else{ | ||
outputPath = relativeDirDataPath / inputPath; | ||
} | ||
if (makeAbsolute) { | ||
outPath = dataPath / path; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be an edge case here if you pass in an absolute path and does this edge case exist in the original code? looks like it might? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great, I'll be investigating this better now |
||
} else { | ||
// inputPath already contains data path, so no need to change | ||
outputPath = inputPath; | ||
} | ||
|
||
// finally, if we do want an absolute path and we don't already have one | ||
if(makeAbsolute){ | ||
// then we return the absolute form of the path | ||
try { | ||
auto outpath = of::filesystem::canonical(of::filesystem::absolute(outputPath)).make_preferred(); | ||
if(of::filesystem::is_directory(outpath) && hasTrailingSlash){ | ||
return ofFilePath::addTrailingSlash(outpath); | ||
}else{ | ||
// return outpath; | ||
return outpath.string(); | ||
} | ||
if (!path.is_absolute()) { | ||
auto exeDir = ofFilePath::getCurrentExeDir(); | ||
outPath = of::filesystem::relative(dataPath / path, ofFilePath::getCurrentExeDir()); | ||
} else { | ||
outPath = path; | ||
} | ||
catch (std::exception &) { | ||
return of::filesystem::absolute(outputPath).string(); | ||
// return of::filesystem::absolute(outputPath); | ||
} | ||
}else{ | ||
// or output the relative path | ||
// return outputPath; | ||
return outputPath.string(); | ||
} | ||
return outPath.string(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these should prob be in another PR but fine for this one.
Maybe we should start using
auto
instead ofof::filesystem::path
for local vars going forward as it will make the code more adaptable in the future.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes +1 for auto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this specific one should not be auto, because data->settings.cacheDirectory is a string, so I'm building an of::filesystem::path from that.