Bugfix of to data path #1251

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

elliotwoods commented May 10, 2012

couple of minor bugfixes (absolute path wasn't being detected on second if [absolute] for windows paths)
fixes unixy paths no matter what on windows

Owner

bilderbuchi commented May 10, 2012

Is this a fix for #1250, or any other existing issues? If so, would be great to indicate that in the PR description.

Contributor

elliotwoods commented May 10, 2012

no it doesn't fix the other issue with current path (i.e. #1250 ) so i didn't mention it at the time
but fixes other issues mentioned above

Contributor

ofTheo commented May 10, 2012

there are some issues here with this PR.
its also a very core and critical bit of code so it would be good to triple check all logic.

  1. on windows a "/" starting path should indicate absolute also as some network shares are accessed this way.
  2. potential issue for lines 263 to 267, do we need to check that the string has length before do the substr?
Contributor

elliotwoods commented May 11, 2012

Is there a reason we don't use Poco::Path ? From what I can see we don't have that in our poco package at the moment. Could be useful here for cross platform compatibility / avoiding boiler plate code
This function has often been a bag of pain for me on windows, with new versions often breaking it completely. Perhaps with an external lib doing the work, we can take a higher level approach to defining what we want it to do, make it more stable, and avoid platform specific code.

Good spot on the substr!
Windows network paths begin with
Should I check for that as well?

Contributor

elliotwoods commented May 11, 2012

Perhaps pseudo code should be something simpler like:


string result

if (path.length == 0)
    result = dataPath
else if ( isAbsolute(path) )
    result = path;
else {
    string dataPath = makeAbsolute ? absoluteDataPath : relativeDataPath
    result = dataPath + path
}
formatForCurrentOS(result) 

Return result

Also to get rid of 'current directory' issues, calling getcwd once at start oF and caching the result would remove 1/2 the 'changing current working directory' issue. the other 1/2 would require to actually switch the current working directory back to the exe path. This could be fixed with some Poco makeRelative stuff i'm sure

do you think this #819 could be related to same issue?

Contributor

elliotwoods commented May 11, 2012

@hvfrancesco - aha yes! #819 references the same issue i'm having with current working directory changing after a load dialog.

Contributor

ofTheo commented May 11, 2012

hey elliot - good point, it should also check for the double backslash.
also the pseudo code looks cleaner. maybe you could update the PR with an implementation and then get lots of eyes on the diff to make sure we aren't introducing new bugs :)

Contributor

kylemcdonald commented Jun 20, 2012

i can confirm that absolute paths are broken on windows.

@ofTheo what do you think about @elliotwoods proposal to use Poco::Path instead?

any solution should be able to handle:

  • absolute and relative paths
  • regardless of running from the IDE or from the binary directly
  • on all platforms

which is about 2x2x4=16 use cases to check, but it's important to fix this.

Contributor

elliotwoods commented Jun 30, 2012

bump! @ofTheo

Contributor

ofZach commented Jul 4, 2012

Quickly, I agree with doing something like this:

getcwd once at start oF and caching the result would remove 1/2 the 'changing current working directory' issue.

On osx we've got a problem where the fixes to make the cwd inside the bundle (so ../../../data works) happens when you call ofGetDataPath. This is because sometimes the app's cwd is not inside the bundle (if you are missing a plist, have adjusted settings etc). What this means is that until you interact with OF's file loading and ofGetDataPath, you are not guaranteed to know where the exe actually is. Doing something at startup to get the paths (and in the case of OSX set the paths) would be really beneficial.

@ghost ghost assigned ofTheo Aug 4, 2012

Contributor

ofTheo commented Aug 5, 2012

@elliotwoods @kylemcdonald

I'm for fixing this in the most reliable way for the short term.
If this is just a windows issues right now, I would suggest doing a fix which makes it work on Win ( with the use cases I mentioned above taken into account ).

So maybe just amend this PR with a quick fix for Win and then do a seperate issue for switching this stuff over to Poco::Path so it can be tested thoroughly first ?

T

Contributor

elliotwoods commented Aug 30, 2012

I'm going to look into this today
Briefly, I'm thinking to:

  1. try Poco::Path first
  2. look into cached getcwd
Contributor

ofTheo commented Aug 30, 2012

great!
I would argue for the most conservative solution that solves the problem, (just incase we introduce a new bug as a result).

I'm able to test on OSX, CB and VS 2010.

Contributor

elliotwoods commented Aug 30, 2012

Closed due to replacement by #1523

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment