Ensure filename in xopp format is always saved as a relative path to the directory it resides in

[hyperupcall]

There are several issues stemming from the fact that annotated PDF backgrounds break when:

1. Renaming / moving containing directories because filename is stored as an absolute path
2. Saving a .xopp file different than the $PWD when opening one from the command line (passing it as $1)

• This bug doesn't surface in the common case because xournal is usually launched through a launcher, where the $PWD is equivalent to $HOME, which is nearly always the common parent of xopp and pdf files.

Specifically, this

• Closes Bad way of storing path of background PDF file inside .xopp files? #3880 (relative path is now relative with respect to the newly created .xopp file, rather than $PWD)
• Closes Annotated pdf background not found if not residing in the same directory  #3273 (this particular issue is a specialization of the previous issue)
• Closes Add option to make filename path relative for pdf background #2736 (relative paths are guaranteed to be generated, since we call std::filesystem::relative with two absolute paths, which uses lexically_relative under the hood after canonicalization
• Closes Relative path to pdf (not the attach mode) #2638 (same as previous, the issue seems stale too)
• Closes Customized load path for pdf annotations #2774 (see comment)

One of the issues floated around this changeset, which I unfortunately found only after I did my on work - which appears to do nearly the same thing except:

• I applied my changes to not just background PDFs, but for images too. (wherever I saw a background->setAttrib("domain", "absolute") near a background->setAttrib("filename", ...) in the save handler, I performed this change)

Validation method:

• manual testing

[bhennion]

Only one mutex related comment. The rest LGTM.

Ideally, this should be tested on various platforms, on shared partitions (e.g. in dual boot), but also on external media (usb key, external drive), and on cloud sync'd folders (e.g. Dropbox) opened on different computers.

[bhennion]

@rolandlo If I remember correctly, you had worked on the attached pdf mode back in the day. Would you mind giving this a glance in case there was a hidden reason for absolute paths I'm not aware of?

[rolandlo]

@rolandlo If I remember correctly, you had worked on the attached pdf mode back in the day. Would you mind giving this a glance in case there was a hidden reason for absolute paths I'm not aware of?

I will take a look at it. There absolutely might be a hidden reason for absolute paths, but I'm not aware of it right now and I will have to test in various situations.

[Technius]

I think there's one use case for absolute paths, which is moving the .xopp file but leaving the .pdf file in the same place and only using these annotations on the same computer. Otherwise, I think relative paths are preferred in all other scenarios.

This whole relative/absolute path problem can be entirely avoided with the packaged file format, but I've had almost no time to work on it in the past few months (help would be appreciated).

[hyperupcall]

Cool beans, got that locking fixed!

@Technius Yeah, that's the only use case I can think of for absolute paths too. I figured it's much less intuitive and is extremely fragile. For example, if a set of files is on a USB, it could break even when switching between different Linux distros.

[bhennion]

LGTM!
Merging in 72 hours, leaving time for anyone who can to remember if there was a hidden reason for using absolute paths.

[rolandlo]

Does that really work on Windows when the PDF (or image) and the xopp-file are on different drives?
Seems @Febbe has some experience, see this question
In general since the implementation of std::filesystem is operating system dependent, a lot of care and tests should be made before stepping ahead with this PR.

[hyperupcall]

OMG!! MSVC returns an empty string? Yeah let's hold this - it looks like we have to give special treatment to msvc and cygwin or something... Thanks for bringing this up!

Just to clarify, we are supporting cygwin/mingw or mysys2 as a platform? I usually forget Windows, so I'm not sure if to treat those as separate platforms or lump some of them together, or which ones are most common? Want to make sure this is tested properly

[bhennion]

So... I'm not sure what to do with this. I guess we should treat differently the case were the .xopp file and the .pdf file are not on the same partition. I don't know if this can be easily achieved using std::filesystem.

[hyperupcall]

Yeah - I too was thinking we can have an exception for Windows - where if the drive letter is different, use the absolute path. I suppose this would mean we would have to call a helper function, and do a little dirty work in there just for Windows - does that sound okay?

With some of my other PRs merged, I think this is now something I can consider implementing

[Technius]

Given that the file name / path conversions have been a source of numerous bugs over the years, it would be nice if we could also refactor the path resolution logic into helper functions, and then write unit tests for them.

One potential solution for dealing with the separate drive problem is to only save as a relative path when their root paths are the same.

[Febbe]

So... I'm not sure what to do with this. I guess we should treat differently the case were the .xopp file and the .pdf file are not on the same partition. I don't know if this can be easily achieved using std::filesystem.

That should be achievable via std::filesystem
Build relative path, when that fails with empty path or exception use absolute path.

@Technius it might be better to directly try to calculate a relative path. When it does not exist or is unresolvable then use the canonical path.

[hyperupcall]

The latest commit addresses feedback; namely:

• If the drive letters are different on Windows (the root_path() checks), then simply use the absolute path (see test)
• If the final calculated "relative path" is an empty string, then simply use the absolute path
  • This check was added so that if there are any other path bugs with the filesystem library in the future, it will use the safe path of using the absolute path. I didn't think this was a full replacement for the first .root_path() check though. For example, if the filesystem library somehow fixes the bug and calculates the path correctly, I wanted to ensure that the behavior stays consistent by still using absolute paths

[hyperupcall]

I just fixed up the PR again, renaming the method to normalizeAssetPath, and changing the following:

On Windows, the path is now normalized to use UNIX-style file separators. I checked that these paths are still serializable to fs::path on Windows. This was done to make the data in the file format more cross-platform and it additionally partially address #3412. By making the path separators the same, simply saving the file on a different platform won't needlessly clutter the diff.

Additional tests were also added to ensurethe saved path is non-empty (which is a possibility on windows when .root_path() is different, or when the first argument to std::relative is not an absolute path.

While we're on the topic of background PDF annotation paths, I propose we close #2774. That issue proposes having a set of paths to search for background PDFs. One reason to include it was to mitigate the effects of saving PDF background files as absolute paths. But now that these background PDFs are saved as relative paths, there is little reason for the feature, besides its extra complexity and non-intuitive behavior. Issues are piling up as they are, and I like to say no thanks to some of them to improve the issue signal to noise ratio.

Lastly, are there any blockers for this change? It addresses the latest feedback - namely more tests and that the canonical path should be used if the relative path could not be calculated.

[danemtsov]

I think it's better to check the root path even for non-Windows platforms (see cppreference on path syntax) - better safe than sorry.
Plus I don't think it makes sense to store a relative path if the locations are that 'far apart' (i.e. not even same root path).

xournalpp/src/core/control/xojfile/SaveHandler.cpp

Lines 279 to 284 in edc6645

	background->setAttrib("domain", "absolute");
	auto normalizedPath = Util::normalizeAssetPath(
	std::filesystem::current_path() / p->getBackgroundImage().getFilepath().string(),
	target.parent_path());
	background->setAttrib("filename", normalizedPath);
	p->getBackgroundImage().setCloneId(id);

Probably better to ensure all paths are absolute?

BTW we must also ensure all the strings are converted as UTF-8 to avoid locale and issues.

[danemtsov]

We'll want the result to always have forward slashes.

[Febbe]

@hyperupcall this PR is nearly done and merge-able, actually only the tests must be corrected. So my question is, if you still want to continue working on it and if you have time to do it?

If you are pretty busy, we would just finish it.

[hyperupcall]

If I don't push any commits by April 13th (tomorrow), feel free to finish it up. I wish I was able to close this earlier

[Febbe]

Probably better to ensure all paths are absolute?

A heuristic, and the ability, to manage assets afterward would be good. Neither completely absolute nor relative paths are a good final solution.
But we can implement the heuristic and the asset manager later.

[Febbe]

That should be reverted, or we should choose, to abandon compilers / STL implementations which do not provide std::filesystem

[Febbe]

@xournalpp/core before we merge this, we should discuss, if we really wan't to override all old files.

• I think in all loaded files, asset paths should be saved the same as they are loaded before.
• when a user relocates a xournal file via "save as" he should be asked, if the relative paths should be updated, or if he want to keep them.
• new assets should be added with an relative path

[Technius]

Let's save this for after the release of 1.2.0. I don't think we should be making such a large change right before the next release.

[KronosTheLate]

Any news on this? Allowing the use of relative paths, so that it always works as long as I keep the PDF and xopp file in the same directory, is something that i am waiting for with excitement ^_^

Edit: Relative paths are currently possible by attatching the pdf in the "Annotate PDF dialouge", but it is a relatively hidden option, not default behaviour, and for a document "test.pdf" produces the file "test.xopp.bg.pdf", which clutters my directories. I have not read the details of this proposal, as I believe they are beyond me, but I am really hoping that we could look for a relative path by default, and without the extra ".xopp.bg.pdf" file.

[KlausBlum]

+1 for relative paths to background PDFs!

This would solve lots or problems. It would be cool to have an option for relative or absolute paths as default setting.

Are there any reasons this PR is still waiting?
Putting it into a nightly build would enable us to do all kinds of tests. :-)