-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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: path resolution for non-existent files when using :org-mode/insert-file-link #10889
base: master
Are you sure you want to change the base?
Conversation
It would probably be good to add E2E tests in the future... This is to test proper path resolution across different platforms. The path resolution for electron/mobile is different than "native". We're trying to solve two things: 1. Resolution of _unknown_ files should not be made absolute (breaks platform-independence) 2. Resolution of absolute paths should _not_ add `..` in front of the absolute path.
These are very basic tests for path resolution using test-get-relative-path. It is mainly to test whether `../` gets added to _absolute_ paths.
Fixes `get-relative-path` so that _absolute_ paths do not get prepended with "..". When the path is parsed, the first item in `paths-2` is `""`, since we split away the `/` character.
When running in Electron or on Mobile, Logseq will convert the paths for _new_ files from relative paths to absolute paths. This appears to be unnecessary based on testing in both platforms. This also breaks the platform independence of Logseq graphs, since absolute paths are likely not the same between platforms. This causes problem not only for new files, but also aliases as well (since they do not exist on disk).
I also removed some left behind debug statements and incorrectly named/commented tests.
Should I be keeping this up-to-date with the base branch? Or should I just wait for a code review? |
I'm marking this PR as a draft now until I have time to understand the failing tests better. Even just running
If anyone has the time, I'd appreciate some help... |
This PR fixes file path resolution with using
:org-mode/insert-file-link
when a file does not already exist on-disk (for both new pages and aliases). As a side-effect, this PR fixes #9342.Problem Statement
When using
:org-mode/insert-file-link
in Logseq, linking to a page that does not exist yet creates afile://
path to that page that does not make sense. This not only breaks Org-Roam's file link support, but also prevents Aliases from working from within Logseq.Expected:
From a page in
journals/
:[[New Page]]
->[[../pages/new page.org]]
Actual:
From a page in
journals/
[[New Page]]
->[[file:..//Users/user/logseq-home/pages/new page.org][New Page]]
This problem not only exists for files that have yet to be created, but also Aliases that have already been defined. Note that aliases are, in a way, a type of missing file.
Isolation & Explanation
There are two errors that I see here
file:..//Users/user/
should instead readfile:/Users/user/
. I'm honestly not sure why Logseq handles creation of files using these paths file, because every application I know of fails to parse this strange url. That being said, it seems like there is a lot of work that could be done on path resolution in the code base, so there is probably some weird stuff there that makes it work.The problem that causes the absolute path resolution lies in platform-specific code for handling creating file paths for non-existent pages. This code is platform specific -- the bug lies in Electron and Mobile apps. When running the code through unit tests, you will not see this file-path resolution bug.
Link to offending lines:
logseq/src/main/frontend/handler/page.cljs
Line 697 in b6382d5
In my manual testing, the both the mobile (iOS) and Electron app are able to handle relative paths when trying to create a new page. Thus, it seems to me that this platform-specific handling is not necessary. Even if there was special sauce there, I think the proper resolution should happen internally in the creation API itself, not in the file reference that's in the text file.
For issue (2), the absolute path is being resolved to a
file://
path incorrectly because the functionget-relative-path
will append a..
to the path, even if the first character is a/
. When the path is parsed by this function, the first element will be""
, since we are splitting the path string based on/
. A simple check to see if the first split character is""
resolves this issue.Test Cases
I wrote unit tests for both situations here. My
get-relative-path
tests are a little sparse, but the major change there was just related to the incorrectly prepended..
to absolute paths.More robust/complete units tests are provided for the
get-ref-text
function.I did not add E2E tests, but I don't think it would be unreasonable to add. That being said, from my brief look at the existing E2E tests they seem to be in a bit of an immature state and it might be a bit of work to set everything up that's necessary for them.
Besides the unit tests, I tested proper resolution and subsequent creation of files via the correct relative link in Electron on macOS and the app on iOS. In that testing, the alias issues in #9342 seems to be resolved too.
Future Improvements
Error: No protocol method IDeref.-deref defined for type null:
error. I don't think this is related to my changes. The test function is in my commit, but it is commented out. In case there is a better solution for unit tests relevant to electron in the future, it should be able to be uncommented.:block/page
when the page is pulled from the db. It's only used for the unit tests though.contents.md
tocontents.org
, even if the graph is fresh and I have not personally touchedcontents.md
. While this is annoying on desktop, it is a punch in the gut on iOS. I was working over the (classically broken) iCloud Drive so I could inspect files from macOS, and so I was unable to access the Logseq files through Files on iOS, and through macOS I could only access that folder through the terminal. Thecontents.md
problem cause me a bunch of headaches as a result since I could only remove it from the Terminal on macOS...Thanks everyone!