[idea] ofPathToString -> ofToString

[dimitre]

As ofToString converts a wide variety of objects, what do you think of renaming ofPathToString to ofToString ?
we still don't have a release out using this new function.
cc: @ofTheo @artificiel

[artificiel]

ah well actually filesystem::path is implicitly convertible to string through operator string_type() (adapted to native platform), so that function is probably obsolete as it is implemented within filesystem::path. simply remove the call.

so the ofFilePath::*FS methods can be changed back to the normal names and overwrite their string versions (i.e we only return fs::path) and anywhere an std::string is expected, it will convert.

if the call site needs an actual string for some reason, it can convert it by itself with .string(), .wstring() or .native() depending on context. the idea is to have that conversion done outside the OF API. (it is interesting to notice even the gcc stdlib has not yet solved the wstring/string mess... https://github.com/gcc-mirror/gcc/blob/de4eea7d7ea86e54843507c68d6672eca9d8c7bb/libstdc%2B%2B-v3/include/bits/fs_path.h#L868-L892)

there might be edge cases in projects or addons that expect a string without explicitly having a string type in place (so the implicit conversion cannot resolve) but that would be either a very creative operation, or it reveals an ambiguous one to start with (and probably not cross-platform/encoding). considering the gains of working with filesystem::path instead of plain strings and reducing the surface of the OF API it is worth it and shakes the tree for bugs. and if genuine problems pop up the quick fix is also very simple (-> wrap the fs::path-returning function in std::string() (until the 3rd-party code is properly updated for path support).

[dimitre]

we start using ofPathToString after your suggestion of try / catch path to string conversions

[artificiel]

I remember but searching GitHub issues does not reveal the conversation — can you provide the context for the discussion? maybe there is something more specific to take into account.

And (for information) as of now, does the try-catch actually catch things? it is a defensive approach that makes sense for understanding the behaviour (especially with wstring vs string behaviour), but if it catches, it returns an empty string, so it will not "work" for the end-user of the pathToString...

[dimitre]

I didn't find it too.

[artificiel]

ahhh... what I think: the .string() conversion will throw only when there is an untranslatable glyphs from wstring to string. so a situation on Windows where a file path contains one or more such characters, thus is not representable in string format, in a context where such a string is expected.

before the major change to fs::path it was impossible to end up with such Windows paths (you either get them programmatically though fs::path, or you have to obtain them from the OS, for instance copy-pasting a path into source code, with proper encoding of the text files). so "opening" the API with filesystem::path means suddenly there are situations where wstrings can appear where they could not exist before.

wrapping paths access forces the conversion, and thus forces the error. but it's specific to help reason about how existing stuff reacts to wstring paths, not as a general process. so for instance, there is a little blur between paths and URLs -- URLs are generally paths in a system — but an URL cannot be wstring as the URL spec is UTF-8. so if someone generates URL based on a wstring path, it's an unknown territory. if it's OF doing it we have to find out about it, and because it's an implicit conversion, having the try-catch makes it easier to work on.

I'm not sure how the idea of having *FS versions of function originated, but it makes sense as a transitive operation: to prepare the new functions without removing the existing, and estimate how it resonates with 3rd party stuff — once the evident aspects are ironed-out, remove the wrapper (what I suggest above) and the conversion becomes implicit. I now realize this work is only in git, not in a release, I guess it should be released (along the deprecation of functions that are superseded by native fs::path methods) to perform its role of validating what happens (specifically windows with widestring-exploiting languages). as implicit conversions are by default more "mysterious" a problem might not be instantly connected to the source. logging the catch makes it easier in the context of an end-user that might not be planning for a debug session.

[dimitre]

yes, the FS functions are meant to be used internally in OF Core to streamline fs::path the most possible so no implicit conversions are needed. they can be dumped once we have a breaking release.

std::string ofToString is used all around in OF Core, even ofPixels, this gave me the idea to just use ofToString to convert path to string. in fact I was writing automatically like this today, until I've noticed it is ofPathToString.

There is no release using it yet, and there is a comment in the function:
// Function used internally in OF core. API can change later

[artificiel]

ok the top question is more clear! a reason to keep ofPathToString makes it easier to locate places where the conversion is actually required. for instance it is in ofPixels: auto uriStr = ofPathToString(_fileName); that is a case where it's illegal to have wstring content, so it makes sense to handle it explicitly.

I would suggest:

std::optional<std::string> ofPathToString(const fs::path & path) {
	try {
		return path.string();
	} catch(fs::filesystem_error & e) {
		ofLogError("ofFileUtils") << "ofPathToString: error converting fs::path to string " << e.what();
		return {};
	}
}

and (this also helps in the direction of removing magic values like "empty string" to denote failure)

if (auto uriStr = ofPathToString(_fileName)) {
  // proceed with the URL as *uriStr
} else {
  // it may exist, but the provided _fileName cannot be used in URL context
}

optional behaves a bit like a pointer, so it needs adaptation on the end, but there should not be so many cases where the conversion is actually required.

[dimitre]

This one is next to remove suffix FS from core
#7817
it is ready to go