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

Error linking file in subdirectory #806

Closed
memeplex opened this issue Oct 29, 2021 · 24 comments · Fixed by #841
Closed

Error linking file in subdirectory #806

memeplex opened this issue Oct 29, 2021 · 24 comments · Fixed by #841
Labels
bug Something isn't working

Comments

@memeplex
Copy link
Contributor

  • Foam version: v0.15.2
  • Platform: macOS Big Sur
  • Issue occur on the foam template repo: Yes

Summary

Steps to reproduce

  1. Create a directory d with files a.md and b.md
  2. Add [[d/b]] to a.md
  3. Follow that link: it will offer to create b and the fail because b already exists.
Screen.Recording.2021-10-29.at.06.22.49.mov
@memeplex memeplex added the bug Something isn't working label Oct 29, 2021
@memeplex
Copy link
Contributor Author

memeplex commented Nov 2, 2021

The problem is here:

 static computeRelativeURI(reference: URI, relativeSlug: string): URI {
    // if no extension is provided, use the same extension as the source file
    const slug =
      posix.extname(relativeSlug) !== ''
        ? relativeSlug
        : `${relativeSlug}${posix.extname(reference.path)}`;
    return URI.create({
      ...reference,
      path: posix.join(posix.dirname(reference.path), slug),
    });
  }

In posix.join(posix.dirname(reference.path), slug) the slug is dir/file.md and since dirname only removes the file from the url you end up with .../dir/dir/file.md. I will try to fix this.

@memeplex
Copy link
Contributor Author

memeplex commented Nov 2, 2021

I will need some guidance regarding the expected behavior here:

  1. Is [[dir/file]] a supported wikilink?
  2. If it is, should it be interpreted as starting:
    a. from the root of the workspace
    b. as many levels above the source file as the depth of the wikilink
    c. relative to the source file (but then the "already exists" error is wrong)

b makes no sense to me.

I would like to interpret it as in c and reserve paths starting with / for a, but maybe this isn't intercompatible with other tools, I don't know.

The current implementation is compatible with c except for the error message.

@memeplex
Copy link
Contributor Author

memeplex commented Nov 2, 2021

This is the way it's done for Cmd-click:

        const basedir =
          vscode.workspace.workspaceFolders.length > 0
            ? vscode.workspace.workspaceFolders[0].uri
            : vscode.window.activeTextEditor?.document.uri
            ? URI.getDir(vscode.window.activeTextEditor!.document.uri)
            : undefined;
    ....
    URI.createResourceUriFromPlaceholder(basedir, uri)

So here the wikilink is taken to hang from the root of the workspace as in (a) instead of (c) above. Hence this results in an error message.

@riccardoferretti
Copy link
Collaborator

Hi @memeplex, I have been doing some thinking around it and spoke to other project leads in the space.

Here is where my head is at:

  • [[filename]] a file in the repo with that name

    • in case of multiple files with the same name:
      • point to the first one (by an arbitrary, not yet explicitly defined, order)
  • [[path/to/filename]] absolute from repo root

    • in the case of a multi-folder workspaces
      • if the path is present in multiple folders, use the first matching folder
      • if the path is not present, create it in the first folder of the workspace
  • [[./path/to/filename]] relative from link location

  • [[file:https:///path/to/filename]] absolute from HDD root

  • [[/path/to/filename]] - unclear, could be alias for either [[path/to/filename]] or [[file:https:///path/to/filename]]

Thoughts?

@memeplex
Copy link
Contributor Author

memeplex commented Nov 4, 2021

@riccardoferretti your definitions make sense to me. Regarding the points yet to define:

in case of multiple files with the same name: point to the first one (by an arbitrary, not yet explicitly defined, order)

Given that the meaning could change without the user being aware of it the only safe course of action I see here is to report an error, maybe facilitating the task of entering an unambiguous link, for example: filename is ambiguous, please change it to ./filename, d/filename, ..., not now.

[[/path/to/filename]] - unclear, could be alias for either [[path/to/filename]] or [[file:https:///path/to/filename]]

I think /path/to/filename should be file:https:///path/to/filename, because it actually is what's in the url without the protocol part. Also, writing the full URL is cumbersome and it's nice to have a shortcut when the protocol is obvious.

@riccardoferretti
Copy link
Collaborator

I think /path/to/filename should be file:https:///path/to/filename

I see your point, the counter-argument is that, within Foam (which e.g. could be living in a cloud) an absolute path refers to the absolute path of the repo.

But I do like /path/to/filename to equal file:https://path/to/filename, also because a [[wikilink]] and [[another/wikilink]] feels more natural without slashes

Given that the meaning could change without the user being aware of it the only safe course of action I see here is to report an error

The idea here would be to monitor files and fix things accordingly, using the same principle used to monitor renamed files (see #809) . That means that:

a/
  filename.md 
b/
  hello.md (containing a wikilink to [[filename]])
  • given this repo, if we rename a/filename.md to a/filename2.md, all the links to [[filename]] will be replaced by [[filename2]].
  • given this repo, if we add b/filename.md, all the links to [[filename]] will be replaced by [[a/filename]].

What you say about the error could be surfaced via the diagnostic API, and even provide quick fixes:

  • given the above repo, if outside of Foam we added a file b/filename.md the next time we open Foam the link [[filename]] will be shown as error/warning, and the quick fixes will propose a/filename and b/filename.

Yup, that's a separate task (pretty straightforward in reality once the other pieces are in place) but yeah, not now

@ivl
Copy link

ivl commented Nov 10, 2021

Guys can you help me check if it bug. I have subfolder and on this subfolder in current document I am trying to insert link via Cmd+Click it creates two files. In this subfolder and in root folder and links to root.

@riccardoferretti
Copy link
Collaborator

Do you also have markdown-notes installed?

@ivl
Copy link

ivl commented Nov 10, 2021

Do you also have markdown-notes installed?

Yes

@riccardoferretti
Copy link
Collaborator

That's likely the cause, try disabling it, but if the problem is still present let's create a new issue

@ivl
Copy link

ivl commented Nov 10, 2021

That's likely the cause, try disabling it, but if the problem is still present let's create a new issue

If to disable markdown-notes - really only one file created but on the root folder not In subfolder.

@riccardoferretti
Copy link
Collaborator

riccardoferretti commented Nov 10, 2021

hi @memeplex a couple of things related to this issue/
The two sections here are different, depending on how the conversation goes I might split these in two separate issues.

URI.computeRelativeURI

I have just come across the bug in URI.computeRelativeURI somewhere else, is this something you are interested in addressing? I believe the issue is that when calling this with a directory, the posix.dirname will now use the parent as reference. What's your thinking around this?

[[path/to/file]] identifier

I have been doing some additional thinking around [[path/to/file]] and I am no longer sure it should be interpreted as a path.
Here is where my head is at right now:

  • [[./file]] is a "path-like" link to a resource, relative from the source
  • `[[/path/to/file]] is a "path-like" link to a resource, relative from the repo root
  • [[file]] is an identifier of a resource (based on the filename)
  • [[path/to/file]] is an identifier of a resource (based on the path), the same is true for [[to/file]]

Basically, [[file]] is an instance of [[path/to/file]], where the file is enough to identify this resource.

An example might help.

dir1/
  subdir1/
    file1.md
    file2.md
  subdir2/
    file1.md
dir2/
  file1.md

In the above repo:

  • [[file2]] is a unique identifier of a resource - it can be used anywhere in the repo
  • [[file1]] is an non-unique identifier as it can refer to multiple resources
  • [[subdir1/file1]] is a unique identifier of a resource - it can be used anywhere in the repo
  • [[dir1/subdir1/file1]] is a unique identifier of a resource - it can be used anywhere in the repo
  • [[/dir1/subdir1/file1]] is a path reference to a resource
  • [[./file1]] is a path reference to a resource

Basically we could say as a rule:

  • if the link starts with / or . we consider it a path like reference, in the first case from the repo root, otherwise from the source note
  • if a link doesn't start with / or . it is an identifier
    • generally speaking we use the shortest identifier available to identify a resource, but all are valid
      • [[dir1/subdir1/file2]] [[subdir1/file2]] [[file2]] are all unique identifier to the same resource, and are all valid in a document
      • the same can be said for [[dir1/subdir1/file1]] [[subdir1/file1]]
        • but not for [[file1]], because it can refer to more than one resource

Thoughts?

@riccardoferretti
Copy link
Collaborator

really only one file created but on the root folder not In subfolder

This is the current behavior.
I believe there is already an issue related to objection to this behavior (but can't find it atm). Let's move this conversation that issue or a new one, as this issue is already pretty convoluted and I would like to keep it focused on the original conversation, thanks!

@memeplex
Copy link
Contributor Author

@riccardoferretti sorry for the delay.

I have just come across the bug in URI.computeRelativeURI somewhere else, is this something you are interested in addressing?

It will take some time because I'm currently very busy, so you'll have to be patient. The issue is with dirname, yes, that was my conclusion here too.

I've some questions about your new proposal. Do you want to disallow non-unique identifiers? If you want, how would you enforce uniqueness given that a file may be externally created at any moment? If not, what's the point in differentiating them from their unique counterparts? And how do you plan to resolve ambiguities?

Maybe it's worth taking a look at how Obsidian manages this. Besides inspiration, intercompatibility with Obsidian seems desirable per se.

@memeplex
Copy link
Contributor Author

I think I prefer to interpret everything as paths, so a is /d1/a inside d1 but /d2/a inside a, that is a is ./a, and if you want to refer to /d2/a from d1 you will have to write /d2/a. This way, there is no risk of "capturing" a reference later. If you are organizing stuff into directories it's likely that there already is an idea of hierarchy in the back of your mind, so nodes will be clustered and cross-references will be less frequent, therefore not so many full paths would be necessary. But I don't know if this is the way Obsidian does it.

@memeplex
Copy link
Contributor Author

I've been testing what Obsidian does. While links are unique it allows to specify just the node name. Once you introduce an ambiguity by creating a new "capturing" node, it automatically updates all references so as to make them unique (it prepends the path relative to the vault root to each ambiguous link). But if you create the node from outside, Obsidian does nothing at all to prevent the capture. OTOH everything with a / in it is taken to be relative to the vault root, even if the / is not at the beginning.

Some thoughts, 1c each:

  1. The first proposal was compatible with Obsidian, AFAICS. The second one isn't.
  2. Keeping uniqueness may be difficult, especially inside VSCode which makes it easier to create files circumventing Foam mechanisms.
  3. My strict proposal would allow automatic migration to Obsidian but not necessarily from Obsidian.
  4. Both your second proposal and (to a lesser extent) my strict proposal may break current Foam vaults insofar as they are using directories (not likely I guess, but p > 0).

I think the best move is the first proposal: Obsidian compatible, backwards compatible and it's easier to convey that unqualified names may become ambiguous when referring to other directories, but that this is the one and only case.

@riccardoferretti
Copy link
Collaborator

Thanks for the comments @memeplex

Here are my thoughts:

Do you want to disallow non-unique identifiers? If you want, how would you enforce uniqueness given that a file may be externally created at any moment? If not, what's the point in differentiating them from their unique counterparts? And how do you plan to resolve ambiguities?

We can't disallow them, but we can flag them. We need the following contract with the user:

  • a clear resolution mechanism (e.g. alphabetic) so that if nothing changes a non-unique identifier will always return the same note. Resolution has to be deterministic.
  • a diagnostic entry (warning or error) showed to the user for non-unique identifiers, so that it knows that it's using a "risky" identifier (as it relies on the above mechanism). The quick resolution for this item will show the available unique identifiers matching the non-unique one

If a file is added/renamed in Foam and it breaks existing identifiers, like Obsidian we would automatically update the references.
If a file is added/renamed outside of Foam and it breaks existing identifiers, the diagnostic will inform the user, but Foam won't try to be smarter that than.

Also, with regards to the syntax, given my example above:

dir1/
  subdir1/
    file1.md
    file2.md
  subdir2/
    file1.md
dir2/
  file1.md
  unique.md
Scenario Obsidian Foam
1 [[unique]] ✔ unique identifier in repo ✔ unique identifier in repo
2 [[/dir2/unique]] ✔ valid path from repo root ✔ valid path from repo root
3 [[dir2/unique]] ✔ valid path from repo root ✔ valid identifier in repo
4 [[dir1/subdir1/file1]] ✔ valid path from repo root ✔ valid unique identifier
5 [[/dir1/subdir1/file1]] ✔ valid path from repo root ✔ valid path from repo root
6 [[subdir1/file1]] ✘ incorrect path from repo root ✔ valid unique identifier
7 [[file1]] ✘ ambiguous identifier ✘ ambiguous identifier
8 [[/subdir1/file1]] ✘ incorrect path from repo root ✘ incorrect path from repo root

Which means that Foam's identifiers are a super set of Obsidian (in my proposal): all Obsidian links are supported by Foam, but Foam multi-part identifier (scenario 6) is only supported by Foam. We could make that configurable.

@memeplex
Copy link
Contributor Author

memeplex commented Nov 17, 2021

Ok, if you assume uniqueness you're right it's a superset, my remark was because your proposal might change the order of resolution vis-a-vis Obsidian in the face of ambiguity. If you have a strong commitment to enforce or at least promote uniqueness I believe it's fine, but to import from another tool you still have to resolve the ambiguity some way or another.

That said, I have doubts regarding the value of path prefixes as mere disambiguators, I think they may be confusing, it's natural to interpret a path like d/a as either starting here or starting from some root, it's also easy to convey that a works as a shortcut to something that may be in another branch of the tree, but that in d/a the prefix d/ was introduced merely as the shortest available disambiguator doesn't strike me as particularly natural. For example:

d1/d2
    a: references d4/b
d3/d4
    b

I don't see a great trade off there, you can omit d3 but at the cost of lack of clarity and some degree of lock-in (because this kind of smartness won't likely be part of other tools). I'd prefer d3/d4/b or, as a convenience, b in that case.

@memeplex
Copy link
Contributor Author

memeplex commented Nov 17, 2021

you can omit d3 but at the cost of lack of clarity

I retract this statement, at least to some extent. Thinking about more concrete examples I realize there is much more information in the name than a mere position in the tree and often going just one level above the node will be enough (languages/eiffel instead of computer/languages/python to distinguish it from architecture/towers/eiffel) and you always have the choice to keep adding levels up to the root on a semantic basis, that is if they help make things clearer to you.

I still think this option entails some degree of lock-in though, as any sophisticated choice will do in this matter.

@riccardoferretti
Copy link
Collaborator

riccardoferretti commented Nov 18, 2021

I retract this statement, at least to some extent. Thinking about more concrete examples I realize there is much more information in the name than a mere position in the tree and often going just one level above the node will be enough (languages/eiffel instead of computer/languages/python to distinguish it from architecture/towers/eiffel) and you always have the choice to keep adding levels up to the root on a semantic basis, that is if they help make things clearer to you.

This is the way I think about it, maybe I should have made a more tangible example to highlight the benefits (might steal yours when the time comes ;) )

I agree with your point about interoperability because it is a Foam format here.
I believe this should live behind a user setting to either use paths (/architecture/towers/eiffel), full identifiers (architecture/towers/eiffel) or minimum identifiers (towers/eiffel)

@memeplex
Copy link
Contributor Author

Impressive! Thank you @riccardoferretti

@memeplex
Copy link
Contributor Author

memeplex commented Dec 1, 2021

I believe this should live behind a user setting to either use paths (/architecture/towers/eiffel), full identifiers (architecture/towers/eiffel) or minimum identifiers (towers/eiffel)

@riccardoferretti have you added a setting like this? Is it possible to use full paths in order to ensure intercompatibility?

@memeplex
Copy link
Contributor Author

memeplex commented Dec 1, 2021

Mmmm I've been playing with a Foam generated tree inside Obsidian and the short identifiers that you proposed seem to be supported after all. They're not the ones that Obsidian inserts but it seemingly can follow them anyway.

@riccardoferretti
Copy link
Collaborator

That's good news.
In the end I haven't put a setting around that, to do it properly a few more changes need to happen and I don't consider it as high priority as other things

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants