Closed roymacdonald closed 4 weeks ago
Thanks @roymacdonald - I added a couple of comments but apart from that looks good to go. I am going to take a stab at the ofBaseVideoPlayer stuff now.
@ofTheo should we merge?
@roymacdonald - If you can address the two comments I left then I think its good to merge! Thanks!
@ofTheo as for the public members, I think it is better to leave as strings, since changing to ::path will break stuff (actually, the ofDropInfo class whose public members were changed to path break backwards compatibility, such as the example in ofxHapPlayer). So, I think we should leave those as strings.
As of the function parameter, changing string to ::path requires some implementation reworking so it is an useful change and to make sure it works properly.
In the implementation, defaultPath
could be renamed to _defaultPath
and then internally declare std::string defaultPath
so there is no need to rework any of the code.
ofFileDialogResult ofSystemLoadDialog(std::string windowTitle, bool bFolderSelection, of::filesystem::path _defaultPath){
std::string defaultPath = ofPathToString(_defaultPath);
...
}
But that would give the false impresion that the use of std::filesystem::path is done properly, and because of such I think it is better to keep the function parameter as std::string, and only change it when it is properly handled.
Thanks @roymacdonald !
@dimitre - I am tempted to just merge this due to the public member issue Roy mentioned.
Would that be fine with you and then we can PR in some improvements for ofFileDialogResult
where we can make the std::string filePath
a copy of the real internal path ( which can be protected ) and then getter functions for Path and String?
Hey @ofTheo great. if this makes a smoother transition
I still consider your comment in ofSystemUtils.h
valid, preserving internally as fs::path. @roymacdonald can comment if he has any objection to this
Thanks @dimitre - I think we can now go in and fix up some of this stuff with the goal of not breaking Windows and then figure out a 'path' ;-) forward for making path returns Win compatible.
@ofTheo