search

thSoft / elysium

LilyPond IDE for Eclipse
http://elysium.thsoft.hu
14 stars 3 forks source link

#44 allow relative includes pointing outside a project #66

Closed nittka closed 9 years ago

nittka commented 9 years ago

proposed patch for ticket #44 I am not quite happy about relative imports staying within the workspace projects also being handled, though.

thSoft commented 9 years ago

I tested this PR on my machine and like it. I have some comments though. What do you mean about " relative imports staying within the workspace projects also being handled"?

nittka commented 9 years ago

Regarding "relative imports staying within the workspace": Assume you have two projects P1 with the including file and P2 with the file f.ly to be included. So you would write \include "../P2/f.ly" in the including file.

The current implementation will fire because the String contains ".." and the resolved file exists and hence will yield a file-URI. However, the file is located in the workspace and would have been found anyway (resulting in a valid platform-URI). I don't know if there are undesirable side effects of this behaviour.

Here is a short description of what the code does. In short: if importUri might leave the workspace (platform URIs will not work) then calculate and use the file URI instead. This works by obtaining the URI of the file (resourceUri, it should be non-null and a platform URI, i.e. within the workspace - if not we try no magic). Then the actual file is obtained (base - platform location is the workspace path, toPlatformString is the file path starting with the project). base should exist but if not - no magic. The rest is resolving the import wrt. the file URI rather than the platform URI. I do not claim that there is no more elegant way to turn the file's platform URI into a file URI.

The check importUri.contains("..") is a very rough heuristic and should actually be isRelativeUriPointingSomewhereOutsideTheWorkspace(object, importUri) with a proper implementation. The check which is commented out would not make sense at all because "include.ly" or "subdir/imclude.ly" are relative URI as well, and they are not pointing outside a project.

thSoft commented 9 years ago

Thanks for the thorough explanation! I think you can use IWorkspaceRoot#findFilesForLocationURI for checking whether a URI points outside the workspace. Could you please try it?

nittka commented 9 years ago

Thanks for the hint. I included the check and refactored the code to be more readable (hopefully). A test for this functionality would have to be a plugin test.

thSoft commented 9 years ago

Thank you for the contribution! Yes, tests would be great. (My plan is to use Tycho #56, which would anyway run the tests as plugin tests.)

thSoft commented 9 years ago

I made a copy-paste error in the message of the merge commit, the correct one is:

Merge pull request #66 from nittka/relativeImport

#44 allow relative includes pointing outside a project