-
Notifications
You must be signed in to change notification settings - Fork 77
Fix paths for projects when not relative to OF folder #332
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
Conversation
|
Ahh - thanks @dimitre! So if I passed in a relative path for the project or an absolute path or an absolute path outside of the OF path Not sure if that affects this PR. But just wanted to mention it. Also wondering how many filesystem bugs we might fix ( for spaces and non English characters ) if we handled paths relatively from an early stage. In the PG app. ( ie: if OF is a relative path and project is relative, we don't even need an abs path ). But that might be another large PR / project 🙂 |
|
|
||
| projectDir = projectDir.lexically_normal(); | ||
| if (projectDir.is_absolute()) { | ||
| relRoot = getOFRoot(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my experience is getOFRoot() is always absolute - but might be worth checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is, yes. in this case I'm setting relRoot to absolute root path, so we don't have some absurd relative paths going all the way down to root, like
../../../../../../../Users/z/Desktop/project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might need a bit more handling here. First you would want to make relRoot relative to projectDir but only make relative if the projectDir exists inside the OF folder.
I think very similar to what I did here:
https://github.com/openframeworks/projectGenerator/blob/master/ofxProjectGenerator/src/projects/baseProject.cpp#L221-L224
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
obviously in this case relRoot is misnamed. in fact updating PG always have the feeling it has to be completely rewritten :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise I think it will end up replacing ../../../ with absolute OF paths :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this is the best option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to update this PR if you have the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool - will do!
I just checked and it is currently making projectDir absolute - so then then relRoot gets made abs.
relRoot is ../..
projectDir is "/Users/theo/Documents/CODE/__OPENFRAMEWORKS/openFrameworks/apps/test123"
relRoot is /Users/theo/Documents/CODE/__OPENFRAMEWORKS/openFrameworks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah with your path inside path detection it will do it right
yesterday before your PR I was searching for the same thing. I was wondering if std::filesystem had a function to check if a path contains another, so nothing in this case.
I was going to test this snipped but I've seen you already solved in your PR
Maybe we could have a function in Utils.cpp (or the core) so your solution can be reused if needed in other places.
bool is_subpath(const std::filesystem::path &path,
const std::filesystem::path &base)
{
auto rel = std::filesystem::relative(path, base);
return !rel.empty() && rel.native()[0] != '.';
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dimitre yes! great idea. having a is_subpath or even more specific isPathInOFRoot or something would be helpful - the rfind approach is kind of hard to follow ( though it seems to work well )
|
Okay this should be good now. a few tests: And all the OF files show up in the side bar as expected. |
I think it closes openframeworks/openFrameworks#7464