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

fs::path <> string interoperability #7817

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

Conversation

dimitre
Copy link
Member

@dimitre dimitre commented Dec 12, 2023

this PR is to experiment interoperability between fs::path and string.
I think this was not happening before because of some systems were using
std::experimental::filesystem::v1::__cxx11::path which caused errors when converting fs::path to string.
I've updated ofConstants.h in this PR so __cxx11 is not being used anymore

@dimitre
Copy link
Member Author

dimitre commented Dec 12, 2023

it will cause some test errors with stuff like
ofToDataPath(path).back() to get the latest character of a string, and it doesnt' work with fs::path
I personally think the gains outweight the potential differences.
the idea here is change return type to fs::path and remove the FS() versions
thoughts? @artificiel @ofTheo

@artificiel
Copy link
Contributor

+1 for path return type everywhere.

for the failing tests they should be updated to explicitly perform the string conversion:

ofxTestEq(ofPathToString(ofToDataPath("movies\\",true)).back(), '\\', "absolute ofToDataPath with \\ should end in \\");

(incidentally it's more or less what someone holding to some legacy code using string methods directly on the return of ofDataPath might need to do, but method chaining is not prevalent in user code)

@dimitre
Copy link
Member Author

dimitre commented Dec 12, 2023

Great. if we decide to proceed with this idea I'll complete the work in this same PR.

@dimitre dimitre marked this pull request as ready for review January 6, 2024 21:47
@dimitre
Copy link
Member Author

dimitre commented Jan 6, 2024

I've finally finished this one. I've updated the tests accordingly.
This PR changes return type from a number of core functions from string to fs::path
but brings end to end fs::path to the core.

opinions? @ofTheo @NickHardeman @artificiel @danoli3

@ofTheo
Copy link
Member

ofTheo commented Jan 6, 2024 via email

@dimitre
Copy link
Member Author

dimitre commented Jan 6, 2024

In the tests the things I had to change is something like ofToDataPath("").back()
and my own usage the only thing I had to change is one addon using this line

	string resultado = ofSystem("open " + ofToDataPath(fullFileName));

I had to switch to something like:

	string resultado = ofSystem("open " + ofPathToString(ofToDataPath(fullFileName)));
or 
	string resultado = ofSystem("open " + ofToDataPath(fullFileName).string());

@dimitre
Copy link
Member Author

dimitre commented May 13, 2024

I think this one is ready to go
fs::path in more places, using implicit conversion to std::basic_string (char & wchar) where possible.
and explicit string conversion where not possible.
cc: @ofTheo

@NickHardeman
Copy link
Contributor

Hi @dimitre,
This is great, thank you.
Would this PR fix the issue mentioned here:
#7913

@dimitre
Copy link
Member Author

dimitre commented May 13, 2024

What do you think @ofTheo ?
the only downside is this:
#7817 (comment)

@artificiel
Copy link
Contributor

and just to collect what was discussed elsewhere in the context here, given an fullFileName that contains non-POSIX unicode:

	string resultado = ofSystem("open " + ofPathToString(ofToDataPath(fullFileName)));
        // will not crash but will do something unpredictable (as the string returned will be "")
	string resultado = ofSystem("open " + ofToDataPath(fullFileName).string());
        // will throw/crash

I know @dimitre wants C++14 but the proper way of handling such a disappointment is solved with <optional>, where the code would look like:

        if (auto conformant = ofConformPathToNarrowString(ofToDataPath(fullFileName))) {
            auto resultado = ofSystem("open " + conformant.value());
            // use resultado with confidence
        } else {
            ofLogWarning("Your path contains incompatible wide unicode characters)
	}

a similar behaviour is obtainable with checking for an empty string but it's exactly the pattern that optional wants to eradicate (magic numbers etc).

        auto conformant = ofConformPathToNarrowString(ofToDataPath(fullFileName));
        if (!conform.empty()) {
            auto resultado = ofSystem("open " + conformant);
            // use resultado with confidence
        } else {
            ofLogWarning("Your path is empty or contains incompatible wide unicode characters)
	}

@dimitre
Copy link
Member Author

dimitre commented May 17, 2024

@ofTheo please check this one when you have time.
I have other ideas to test that build upon this one merged

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

Successfully merging this pull request may close these issues.

None yet

4 participants