-
Notifications
You must be signed in to change notification settings - Fork 64
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
Avoid relativizing paths in the project outline #519
Avoid relativizing paths in the project outline #519
Conversation
In the project outline view, paths for the Makefile and build log were automatically made relative. If the setting was an absolute path, the result would be ugly: For a makeDirectory setting '/tmp/amhello-debug', the outline would print Makefile: [../../../tmp/amhello-debug/Makefile] This was not only unsightly, but also wrong. See https://unix.stackexchange.com/questions/13858/do-the-parent-directorys-permissions-matter-when-accessing-a-subdirectory In short, if one of the parent directories (..) would be inaccessible, the ../../../tmp/amhello-debug path is wrong because it's inaccessible, even though /tmp/amhello-debug is accessible.
@drok To help us confirm that this works, along with our own testing, could you provide a screenshot of this working after your changes? Thanks! Additionally, can you please make a CHANGELOG entry for this change, following the pattern already set? Yours will go in the 0.8 section, feel free to tag yourself to give yourself credit! |
c2238dc
to
492f518
Compare
This PR affects the lines "Makefile:" and "Build Log:" I have highlighted in red ( If #500 is merged after this, The display will change as follows: In this case, the "(not found)" text is wrong, because the Makefile does exist in the makeDirectory. It happens because the outline code (tree.ts) attempts to find the file separately from the There is more work needed to make the outline display accurate and useful, and some of that work (ie, authoritative resolution of the Makefile) happens in another PR I have not yet proposed: "fix-out-of-tree-builddir" I have also added CHANGELOG entry, crediting myself as generously sponsored by @Mergesium. |
492f518
to
a0fbb58
Compare
@gcampbell-msft my previous update conflicted with CHANGELOG.md from main; the latest forced push resolves the conflict by rebasing the branch onto the fresh main. |
@drok , thank you for the suggestion to include make directory (what is given via -C to "make", right?) into the UI panel. I opened work item #520 to track this idea. I am interested to find out more about the wrong location of the makefile in the tree. Indeed getCommandForConfiguration() calculates correctly many things but once it does, it stores the latest values into some globals, which are then read so that we don't compute every time what getCommandForConfiguration does. You some some wrong values, I wonder if the bug would be somewhere else, like we forget to call getCommandForConfiguration after some other actions that result in change of those variables. Indeed, always relativizing was not a good idea but I'm not sure about always not relativizing. The original reason behind the relativizing strategy was to have shorter easier to understand paths. What do you think we would compare the relative versus non relative path and chose the shortest? Or in-tree always relativize and out-of-tree always don't relativize? Additionally, check the permissions situation which I was not aware of. Thank you for explaining that. |
The intent with this PR is to make these forms of feedback less jarringly terrible. However, since you ask, and I appreciate you thinking more in depth about the issue of providing quality feedback to the user, I would say that the outline panel needs some focused effort. Here is how I would present the paths: The
|
@drok I'd like to merge this PR, could you fix the merge conflicts? Thanks! |
a0fbb58
to
c5064ca
Compare
In the project outline view, paths for the Makefile and build log were automatically made relative. If the setting was an absolute path, the result would be ugly:
For a makeDirectory setting '/tmp/amhello-debug', the outline would print
Makefile: [../../../tmp/amhello-debug/Makefile]
This was not only unsightly, but also wrong. See
https://unix.stackexchange.com/questions/13858/do-the-parent-directorys-permissions-matter-when-accessing-a-subdirectory
In short, if one of the parent directories (..) would be inaccessible, the ../../../tmp/amhello-debug path is wrong because it's inaccessible, even though /tmp/amhello-debug is accessible.