Skip to content
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

Jump to bottom

Fix project path when path contains a symbolic link #6414

Merged
merged 1 commit into from Mar 6, 2018
Merged

Fix project path when path contains a symbolic link #6414

merged 1 commit into from Mar 6, 2018

Conversation

alexispolti
Copy link
Contributor

@alexispolti alexispolti commented Feb 22, 2018 •
edited

Fixes #18337
When project's path contains a symbolic link and relative path are used for layers, all layers are currently stored relative to the symbolic path. Example:

  • Project : ~/Documents/map/map.qgs
  • Raster layer : ~/Documents/map/raster/layer.tif
  • Symbolic link ~/map poiting to ~/Documents/map

If one's opens ~/map/map.gqs and saves the project, the project will now contains these layers paths : ../Documents/map/raster/layer.tif instead of ./raster/layer.tif

This PR should fix it.

Checklist

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include fixes #11111 in the commit message next to the description
  • Commits which add new features are tagged with [FEATURE] in the commit message
  • Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and containt sufficient information in the commit message to be documented
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit

@nyalldawson
Copy link
Collaborator

Nice fix! Could you add a unit test for this please?

@alexispolti
Copy link
Contributor Author

Sorry, I won't have time to do it soon. Besides I have absolutely no idea if symbolic links are possible under Windows.

@nyalldawson
Copy link
Collaborator

Besides I have absolutely no idea if symbolic links are possible under Windows.

No problem there - we have a lot of tests which don't play nice on windows. The most important thing is to get test which works on Linux so that it's covered by the CI infrastructure.

(FYI - you CAN make symbolic links in windows)

When you get a chance to revisit this PR, the test needs adding in test_qgsproject.py

@nyalldawson nyalldawson added the Requires Tests! Waiting on the submitter to add unit tests before eligible for merging label Feb 22, 2018
@alexispolti
Copy link
Contributor Author

I looked into the unit tests and it is much more simple than I thought. Which made me discover another bug when writing projects. The process of file handling / canonicalization is much more complicated than I thought as the project's file is modified sometimes through mFile.fileName() and sometimes through QgsProject::setFileName( const QString &name ).
Definitively needs much more thoughts…

@alexispolti
Copy link
Contributor Author

Sorry, I shouldn't have closed the PR (it is really an issue). But not ready to merge yet. Will look more deeply into it as soon as possible.

@alexispolti alexispolti reopened this Feb 23, 2018
@nyalldawson
Copy link
Collaborator

Thanks @alexispolti!

@alexispolti alexispolti force-pushed the fix-project-path branch 5 times, most recently from 422aa69 to 57f5a6d Compare February 25, 2018 17:25
@alexispolti
Copy link
Contributor Author

alexispolti commented Feb 25, 2018 •
edited

Hi @nyalldawson,
The PR is ready. After much thoughts the best way to go seemed to modify the resolver. Symbolic links are now handled correctly. There's a caveat though : for the resolver to work correctly, the project's directory (not necessarily its name, just its directory) must exist on the disk. I didn't see any cases where that might be a problem. But if you see any, please let me know!
Unit tests are OK on the Python side and on the C++ side.
And sorry to not have had time to fix it before the 3.0 release :/

@alexispolti
Copy link
Contributor Author

Hello @nyalldawson ,
Any news on this PR ? You might want to remove the "Requires Tests" label, I included them.

@nyalldawson nyalldawson added Needs Backporting and removed Requires Tests! Waiting on the submitter to add unit tests before eligible for merging labels Mar 3, 2018
@nyalldawson
Copy link
Collaborator

Looks good to me! @wonder-sk can you take a look here too? It's in the path resolver and I'd value a second opinion.

@wonder-sk
Copy link
Member

wonder-sk commented Mar 5, 2018 •
edited

I am wondering whether we could remove the new requirement that the base path had to exist. The path resolver may used in various contexts, not just for project loading/saving, so it would be nice if the class could write relative paths even if the base path does not exist. How about using logic where we would first try to use the canonicalFilePath() and if it fails to resolve the path (returns empty string), we would fall back to absoluteFilePath() as was the case until now?

@alexispolti
Copy link
Contributor Author

alexispolti commented Mar 5, 2018 •
edited

Good idea! PR has been updated and tests added to cover the case where the base path doesn't exist.

@wonder-sk
Copy link
Member

Great, thanks for the quick fix! Looks good to me.

@alexispolti alexispolti force-pushed the fix-project-path branch 3 times, most recently from 924a740 to 9d55e1d Compare March 5, 2018 18:11
@nyalldawson nyalldawson mentioned this pull request Mar 6, 2018
Closed
8 tasks
@luipir
Copy link
Contributor

luipir commented Mar 6, 2018 •
edited

Just faced this bugs and duplicated the PR (#6545), just a doubt on the unit test, but other then this I think it can be merged

luipir
luipir reviewed Mar 6, 2018
View reviewed changes
tests/src/core/testqgsproject.cpp Outdated
QCOMPARE( resolverLegacy.readPath( "/home/qgis/file1.txt" ), QString( "/home/qgis/file1.txt" ) );

// Test resolver with symbolic links in path
QTemporaryDir tmpDir;
Copy link
Contributor

Choose a reason for hiding this comment

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

hi @alexispolti how this part of code, tests the management of the symbolik link?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, this code doesn't test the management of symbolic link. The management is tested in tests/src/python/test_qgsproject.py. I updated the comment.

@nyalldawson
Copy link
Collaborator

@luipir happy for this to be merged if Travis approves?

@luipir luipir merged commit 74d14ae into qgis:master Mar 6, 2018
@luipir
Copy link
Contributor

luipir commented Mar 6, 2018

travis is happy :)

@alexispolti alexispolti deleted the fix-project-path branch March 6, 2018 22:51
@nyalldawson
Copy link
Collaborator

@alexispolti great work! Thanks for your responsiveness to the requested changes.

I'll make sure this is backported to 3.0 also.

@alexispolti
Copy link
Contributor Author

My pleasure! Thanx to you all!

@qgib qgib mentioned this pull request May 25, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luipir luipir luipir left review comments

Assignees
No one assigned
Labels
Needs Backporting
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@alexispolti @nyalldawson @wonder-sk @luipir