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.

Sign up for GitHub

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

Jump to bottom

Some fixes for search paths: #17435

Merged
merged 3 commits into from
Mar 6, 2017
Merged

Some fixes for search paths: #17435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
961e195
Some fixes for search paths:
Mar 3, 2017
035e52f
Checks whether the parameter ‘searchResolutionOrder’ passed in is the…
Mar 6, 2017
9334eec
Don’t break the compatibility, add a new method called ‘FileUtils::ge…
Mar 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
56 changes: 48 additions & 8 deletions cocos/platform/CCFileUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -859,7 +859,13 @@ std::string FileUtils::fullPathFromRelativeFile(const std::string &filename, con

void FileUtils::setSearchResolutionsOrder(const std::vector<std::string>& searchResolutionsOrder)
{
if (_searchResolutionsOrderArray == searchResolutionsOrder)
{
return;
}

bool existDefault = false;

_fullPathCache.clear();
_searchResolutionsOrderArray.clear();
for(const auto& iter : searchResolutionsOrder)
Expand Down Expand Up @@ -907,41 +913,64 @@ const std::vector<std::string>& FileUtils::getSearchPaths() const
return _searchPathArray;
}

const std::vector<std::string>& FileUtils::getOriginalSearchPaths() const
{
return _originalSearchPaths;
}

void FileUtils::setWritablePath(const std::string& writablePath)
{
_writablePath = writablePath;
}

const std::string& FileUtils::getDefaultResourceRootPath() const
{
return _defaultResRootPath;
}

void FileUtils::setDefaultResourceRootPath(const std::string& path)
{
_defaultResRootPath = path;
if (_defaultResRootPath != path)
{
_fullPathCache.clear();
_defaultResRootPath = path;
if (!_defaultResRootPath.empty() && _defaultResRootPath[_defaultResRootPath.length()-1] != '/')
{
_defaultResRootPath += '/';
}

// Updates search paths
setSearchPaths(_originalSearchPaths);
}
}

void FileUtils::setSearchPaths(const std::vector<std::string>& searchPaths)
{
bool existDefaultRootPath = false;
_originalSearchPaths = searchPaths;

_fullPathCache.clear();
_searchPathArray.clear();
for (const auto& iter : searchPaths)

for (const auto& path : _originalSearchPaths)
{
std::string prefix;
std::string path;
std::string fullPath;

if (!isAbsolutePath(iter))
if (!isAbsolutePath(path))
{ // Not an absolute path
prefix = _defaultResRootPath;
}
path = prefix + (iter);
fullPath = prefix + path;
if (!path.empty() && path[path.length()-1] != '/')
{
path += "/";
fullPath += "/";
}
if (!existDefaultRootPath && path == _defaultResRootPath)
{
existDefaultRootPath = true;
}
_searchPathArray.push_back(path);
_searchPathArray.push_back(fullPath);
}

if (!existDefaultRootPath)
Expand All @@ -962,10 +991,21 @@ void FileUtils::addSearchPath(const std::string &searchpath,const bool front)
{
path += "/";
}

if (front) {
_originalSearchPaths.insert(_originalSearchPaths.begin(), searchpath);
_searchPathArray.insert(_searchPathArray.begin(), path);
} else {
_searchPathArray.push_back(path);
_originalSearchPaths.push_back(searchpath);

if (!_searchPathArray.empty() && _searchPathArray[_searchPathArray.size()-1] == _defaultResRootPath)
{
_searchPathArray.insert(_searchPathArray.begin() + _searchPathArray.size() -1, path);
Copy link
Author

@dumganhar dumganhar Oct 27, 2017 •
edited

Choose a reason for hiding this comment

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

@sbrednikhin, @balmerdx
So the key point is that this line push the default resource path to the end, right?
Yes, I know I broke the compatibility here since I think the search path of default resource should always be at the end. I don't know what kinds of situation you will do in your game. Perhaps, more code snippets and more detail are welcome.

Anyway, if you think I was wrong, feel free to submit a new or better pull request to correct my mistake. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't care where default resource path is, actually. My only point is that if you breaking regular code behavior, causing people to fix their code (which must rely on some order of paths), you should have some good reason for that. Not just thinking that it is better.
Since at least some people don't see any obvious reason for that change, you could point on it in comment or something.
This project is open source common code - not your own branch.

Copy link
Author

Choose a reason for hiding this comment

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

OK，you're totally right, thanks for reminding me. I will revert that line. I was wrong, I used it as 'my own my private branch', good luck guys, thank you guys.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you, for understanding. We are very appreciate your work.

}
else
{
_searchPathArray.push_back(path);
}
}
}

Expand Down
21 changes: 20 additions & 1 deletion cocos/platform/CCFileUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,11 @@ class CC_DLL FileUtils
*/
virtual void setSearchPaths(const std::vector<std::string>& searchPaths);

/**
* Get default resource root path.
*/
const std::string& getDefaultResourceRootPath() const;

/**
* Set default resource root path.
*/
Expand All @@ -457,12 +462,21 @@ class CC_DLL FileUtils
/**
* Gets the array of search paths.
*
* @return The array of search paths.
* @return The array of search paths which may contain the prefix of default resource root path.
* @note In best practise, getter function should return the value of setter function passes in.
* But since we should not break the compatibility, we keep using the old logic.
* Therefore, If you want to get the original search paths, please call 'getOriginalSearchPaths()' instead.
* @see fullPathForFilename(const char*).
* @lua NA
*/
virtual const std::vector<std::string>& getSearchPaths() const;

/**
* Gets the original search path array set by 'setSearchPaths' or 'addSearchPath'.
* @return The array of the original search paths
*/
virtual const std::vector<std::string>& getOriginalSearchPaths() const;

/**
* Gets the writable path.
* @return The path that can be write/read a file in
Expand Down Expand Up @@ -873,6 +887,11 @@ class CC_DLL FileUtils
*/
std::vector<std::string> _searchPathArray;

/**
* The search paths which was set by 'setSearchPaths' / 'addSearchPath'.
*/
std::vector<std::string> _originalSearchPaths;
Copy link
Contributor

Choose a reason for hiding this comment

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

why need _originalSearchPaths

Copy link
Author

Choose a reason for hiding this comment

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

FileUtils uses _searchPathArray for searching full path internally.
setSearchPaths will modify _searchPathArray variable, it will be connected by _defaultResourceRootPath.
_originalSearchPaths saves the original parameter setSearchPaths passes in, and getSearchPaths returns _originalSearchPath will be more reasonable than returning _searchPathArray. It's the rule of getter returns what user has set.

If getSearchPaths returns _searchPathArray, you could suppose the following code will always not work.

If current search path is bbb/, ccc/

auto oldDefaultResourceRoot = fileUtils->getDefaultResourceRootPath(); // Suppose it return "" which is an empty string.
auto oldSearchPaths = fileUtils->getSearchPaths(); // oldSearchPaths is : ["bbb/", "ccc/"]
fileUtils->setDefaultResouceRootPath("aaa"); // oldSearchPath is : ["aaa/bbb/", "aaa/ccc"]
// oldSearchPaths will be modified by adding a prefix `aaa` since `_searchPathArray` was modified. 

... ...

// reset resource root
fileUtils->setDefaultResourceRootPath(oldDefaultResourceRoot); 
auto newSearchPath = fileUtils->getSearchPaths(); // newSearchPath is incorrect: ["aaa/bbb", "aaa/ccc"] since it returns `_searchPathArray` which was added by prefix 'aaa'


/**
* The default root path of resources.
* If the default root path of resources needs to be changed, do it in the `init` method of FileUtils's subclass.
Expand Down
21 changes: 20 additions & 1 deletion tests/cpp-tests/Classes/FileUtilsTest/FileUtilsTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,25 @@ void TestSearchPath::onEnter()
fclose(fp);
}
}

// Save old resource root path
std::string oldDefaultRootPath = sharedFileUtils->getDefaultResourceRootPath();
sharedFileUtils->setDefaultResourceRootPath("extensions");
auto sp1 = Sprite::create("orange_edit.png");
sp1->setPosition(VisibleRect::center());
addChild(sp1);

// Recover resource root path
sharedFileUtils->setDefaultResourceRootPath(oldDefaultRootPath);

auto oldSearchPaths = sharedFileUtils->getSearchPaths();
sharedFileUtils->addSearchPath("Images");
auto sp2 = Sprite::create("btn-about-normal.png");
sp2->setPosition(VisibleRect::center() + Vec2(0, -50));
addChild(sp2);

// Recover old search paths
sharedFileUtils->setSearchPaths(oldSearchPaths);
}

void TestSearchPath::onExit()
Expand All @@ -155,7 +174,7 @@ std::string TestSearchPath::title() const

std::string TestSearchPath::subtitle() const
{
return "See the console";
return "See the console, can see a orange box and a 'about' picture";
}

// TestFilenameLookup
Expand Down