Should ofPathToString(), ofGetExtensionLower(), and FS-functions be private?

[danomatika]

With of_v20250330_osx_release:

I notice ofFileUtils.h adds ofPathToString() and ofGetExtensionLower() however neither is documented and it appears both are used internally.

In ofFileUtils.cpp, bot are commented with:

// Function used internally in OF core. API can change later

Should they be private within of::priv?

UPDATE: The overview list I am wondering about is:

ofPathToString()
ofGetExtensionLower()
ofToDataPathFS()
ofGetPathForDirectoryFS(const of::filesystem::path &);
ofGetAbsolutePathFS(const of::filesystem::path &, bool);
ofFilePath::getCurrentExeDirFS();
ofFile::pathFS()
ofDirectory::getAbsolutePathFS()

[danomatika]

Relatedly, I notice there is a ofToDataPathFS() function which was added above the original ofToDataPath(). This also appears to be private in its usage or is it supposed to replace ofDataPath()? As it is now, the original documentation comments for ofDataPath() would now be rendered by Doxygen for ofToDataPathFS(). (Assuming oxygen is still being used, I dunno.)

My feeling is that this was unintentional and I think, at a minimum, the declarations should be swapped in order and ofToDataPathFS() documented.

[danomatika]

Also, there is a new ofFile::pathFS() getter. As a beginner, why would I need this over ofFile::path() or should it also be private?

UPDATE: The same is true for ofDirectory::getAbsolutePathFS().

[ofTheo]

@danomatika there is more info on this here: #8143

Essentially the goal was to move all file paths and internal file logic to std::filesystem to help with non English filesystem support, but Windows does not like to do:

std::string myFilePath = myFileSystem.string()

As Windows uses wide string for all std::filesystem calls.

So the current situation with the pathFS was a temporary solution.
We're still trying to figure out the best path forward.

See also #8353

[danomatika]

I think my suggestion is more to not exposes this if it's currently internal only. People will just end up using it, then things break. If it's marked private or in some sort of experimental name space, they are at least warned.

You can push a change over to OF 0.13.

[ofTheo]

Thanks @danomatika!
I am curious if that FS suffixed functions could be made internal only the way they are used across the core.
I'll let @dimitre chime in there.

I get your point though, if people start using these in 0.12.1 and we remove them in 0.13 it will be problematic.

[dimitre]

Sure! it is best if we use them as private.
Anybody else have the availability to work on this change?

[artificiel]

private is one way, or FS functionality could already be folded back into the normal non-FS function name, and wrap the results in ofToDataPath() string wrapper — this refactor will be happening anyhow when #8353 is merged post-12.1. (unfortunately bad timing this week for me can't commit to that now).

[ofTheo]

Yeah I took a quick look and it would be quite messy to make internal as the functions are being used across several classes.
I am tempted to suggest leaving as is for now in the interest of getting 0.12.1 out.

[danomatika]

At a minimum I would suggest:

• swap order of ofDataPath() and ofDataPathFS() declarations so original commenting is associated with original function.
• add commenting for each *FS function noting the more experimental/temporary status and that they may change in the future

As for making private, would wrapping them in a namespace in ofFileUtils.h then adding a using namespace to each .cpp file where they are used be messy? I'm not seeing a lot of usage just doing a quick find in Xcode, but I also don't know to what degree they are being used in addons.

EDIT: I did some checking and it seems the usage is down to

ofSystemUtils.cpp
ofFileUtils.h
ofFileUtils.cpp
ofTrueTypeFont.cpp
ofImage.cpp

Output from a tool which uses find:

% check-for-src-pattern ../../../ "FS(" h hpp cpp cc m mm inl
Checking for "FS(" in ../../../
./libs/openFrameworks/utils/ofSystemUtils.cpp:
  326:		wcscpy(wideCharacterBuffer, ofToDataPathFS(defaultPath).c_str());
  406:			wcscpy(szDir, ofToDataPathFS(defaultPath).c_str());

./libs/openFrameworks/utils/ofFileUtils.h:
  339:	static of::filesystem::path getPathForDirectoryFS(const of::filesystem::path & path);
  351:	static of::filesystem::path getAbsolutePathFS(const of::filesystem::path & path, bool bRelativeToData = true);
  437:	static of::filesystem::path getCurrentExePathFS();
  446:	static of::filesystem::path getCurrentExeDirFS();
  578:	of::filesystem::path pathFS() const;
  891:	of::filesystem::path getAbsolutePathFS() const;
  1236:of::filesystem::path ofToDataPathFS(const of::filesystem::path & path, bool absolute=false);

./libs/openFrameworks/utils/ofFileUtils.cpp:
  32:			return fs::canonical(ofFilePath::getCurrentExeDirFS() / "../../../data/");
  34:			return ofFilePath::getCurrentExeDirFS() / "../../../data/";
  40:            return fs::canonical(ofFilePath::getCurrentExeDirFS() / "data/").make_preferred();
  42:			return (ofFilePath::getCurrentExeDirFS() / "data/");
  49:		static auto * defaultWorkingDirectory = new fs::path(ofFilePath::getCurrentExeDirFS());
  561:	myFile = ofToDataPathFS(_path);
  651:fs::path ofFile::pathFS() const {
  657:	return ofPathToString(pathFS());
  931:		path = ofToDataPathFS(path);
  981:		path = ofToDataPathFS(path);
  1167:	originalDirectory = ofFilePath::getPathForDirectoryFS(path);
  1169:	myDir = ofToDataPathFS(originalDirectory);
  1174:	originalDirectory = ofFilePath::getPathForDirectoryFS(path);
  1216:fs::path ofDirectory::getAbsolutePathFS() const {
  1227:	return ofPathToString(ofDirectory::getAbsolutePathFS());
  1299:		path = ofToDataPathFS(path, bRelativeToData);
  1603:		dirPath = ofToDataPathFS(dirPath);
  1637:			dirPath = ofToDataPathFS(dirPath);
  1651:		dirPath = ofToDataPathFS(dirPath);
  1766:fs::path ofFilePath::getPathForDirectoryFS(const fs::path & path){
  1788:	return ofPathToString(ofFilePath::getPathForDirectoryFS(path));
  1821://fs::path ofFilePath::getEnclosingDirectoryFS(const fs::path & _filePath, bool bRelativeToData){
  1837:fs::path ofFilePath::getAbsolutePathFS(const fs::path & path, bool bRelativeToData){
  1851:	return ofPathToString(ofFilePath::getAbsolutePathFS(path, bRelativeToData));
  1872:fs::path ofFilePath::getCurrentExePathFS(){
  1904:	return ofPathToString(getCurrentExePathFS());
  1908:fs::path ofFilePath::getCurrentExeDirFS(){
  1909:	return ofFilePath::getCurrentExePathFS().parent_path() / "";
  1914:	return ofPathToString(getCurrentExeDirFS());
  1964:fs::path ofToDataPathFS(const fs::path & path, bool makeAbsolute){
  2056:	return ofPathToString(ofToDataPathFS(path, makeAbsolute));

./libs/openFrameworks/graphics/ofTrueTypeFont.cpp:
  371:	auto filename = ofToDataPathFS(fontname);

./libs/openFrameworks/graphics/ofImage.cpp:
  270:		auto fileName = ofToDataPathFS(_fileName);
  450:	auto fileName = ofToDataPathFS(_fileName);

[danomatika]

I reported the last post as spam.