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

Fix layer datasources incorrectly resolving to matching working directory names #60109

Merged
merged 5 commits into from
Jan 28, 2025

Conversation

vsydorov
Copy link
Contributor

@vsydorov vsydorov commented Jan 10, 2025

Description

In short, this addresses the issue outlined here: #60100

  • Remove redundant second call to absoluteToRelativeUri in QgsMapLayer::writeLayerXml which was the cause of the original bug
  • Add a guard in QgsPathResolver::writePath to prevent this happening again. Now, if absoluteToRelativeUri is called on the relative path, the function will return early
  • Remove redundant second call to relativeToAbsoluteUri. QgsPathResolver::readPath already contains a guard that returns early when absolute path is supplied. Still, makes sense to remove the call, as it tries to call relativeToAbsoluteUri with an absolute path argument and was introduced in the same commit b9c1c2c as the redundant call causing issues above.
  • Add a regression test, replicating the scenario outlined in the issue.

I think that the PR should be backported to the latest version, as this is an annoying bug for Linux users (#60100, possibly #59282).

@github-actions github-actions bot added this to the 3.42.0 milestone Jan 10, 2025
@vsydorov vsydorov changed the title Fix layer paths resolving to matching CWD names Fix layer datasources incorrectly resolving to matching working directory names Jan 14, 2025
@strk
Copy link
Contributor

strk commented Jan 20, 2025

Great to see such care in describing the PR and writing the tests, thank you !

Copy link

github-actions bot commented Jan 20, 2025

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit 5461e78)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 5461e78)

@vsydorov
Copy link
Contributor Author

Hi! I've fixed the bugs identified by the CI, squashed the commits and rebased to the latest master. This passes all the CI worksflows on my cloned repo now, could you please approve the workflows on this PR?

@elpaso
Copy link
Contributor

elpaso commented Jan 28, 2025

@vsydorov can you please resolve the conflict so that we can merge this PR?

vsydorov and others added 5 commits January 28, 2025 11:33
- Remove redundant second call to absoluteToRelativeUri in
  QgsMapLayer::writeLayerXml
- Add a guard in QgsPathResolver::writePath to prevent this happening
  again when absoluteToRelativeUri is called two times.
- Add a regression test
- The b9c1c2c commit introduced redundant absoluteToRelativeUri AND
  relativeToAbsoluteUri calls. First one was removed earlier, stopping
  the qgis#60100 issue. Remove the latter for completeness too.
- Minor indentation fix
@vsydorov
Copy link
Contributor Author

@elpaso thanks for reviewing my PR! I've resolved the conflicts.

@vsydorov
Copy link
Contributor Author

@elpaso thanks! I've just noticed that I forgot to squash my commits, is it fine like this or should I merge into a single commit?

@elpaso elpaso merged commit 0099198 into qgis:master Jan 28, 2025
31 checks passed
@elpaso
Copy link
Contributor

elpaso commented Jan 28, 2025

I squashed them for you.

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.

3 participants