Skip to content

Conversation

@lmnbeyond
Copy link
Contributor

... not limited to just 4.

@lmnbeyond
Copy link
Contributor Author

@clelland
If you're available, please review this PR!

BTW, I'm wondering should we provide an approach to request fs via file system's name?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's really only one condition here -- could this be
if (!localURL || (index < [[object fsRoot] length])) {
to avoid duplicating the logic inside?

@clelland
Copy link
Contributor

This is a change that I had definitely wanted to make -- it makes the file-system-roots plugin not dependent on the order of installed filesystems as well.

And I agree that we should provide an interface to select a filesystem by name. Right now you can do it by just constructing the URL manually, but that's pretty tightly coupled to the implementation.

@lmnbeyond
Copy link
Contributor Author

@clelland Many thanks to you! I have updated the code to avoid duplicating logic inside, please review this again!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this code a bit more closely now, I think that this is the wrong comparison to make.

The fsRoot property is only a property of the CDVLocalFilesystem class, not CDVFilesystem. It is at least possible for other CDVFilesystem subclasses to implement URLforFilesystemPath: without having an fsRoot property, and in those cases, this code would throw an exception.

Maybe a better way to do this is to keep track of the fullPath property of the url variable (which will be present on any CDVFilesystemURL). That property will be shorter in all of the cases where fsRoot would be longer, so a shorter fullPath would imply that the filesystem is a better match for the local path.

What do you think of changing index into something like shortestFullPath, and testing whether

[[url fullPath] length] < shortestFullPath

?

@lmnbeyond
Copy link
Contributor Author

Sorry for my mistakes! Your comment really makes sense. I have followed your advice and updated my PR, please review it again!

Many thanks for your time. :)

@clelland
Copy link
Contributor

Merged! You can close this PR

I ported your code to the Android side as well, for parity. If you want to, check out 8b0ada7 -- I think that it's an accurate translation of your iOS code.

@lmnbeyond lmnbeyond closed this Feb 25, 2014
asfgit pushed a commit that referenced this pull request Feb 25, 2014
This merges the code from #30
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.

2 participants