[WIP][RFC] Hyperlink Refactor

[SlayerOfTheBad]

I noticed that link abbreviations weren't supported, and wanted to make a PR to do that.
After looking at the code, I quickly started understanding why they weren't supported.
So I set to work refactoring the entire Hyperlink section.

This is a draft because

1. Unittests still need to be written
2. Other unit tests still need to be modified
3. Type annotations still need to be put in
4. Documentation for new features needs to be written

However, I'm going on vacation soon, so I thought I'd present it so this can be reviewed whilst I'm away.
Also, if this is not something that's desired, I don't have to bother writing all the Unit tests.

I've tried to keep the Api stable, but I do know that at least org-roam.nvim does some calls to link.url which doesn't exist anymore.
I am willing to make a PR for org-roam.nvim to resolve those issues.

As for what this pull request changes feature-wise:

1. Improved autocompletion for links on org-insert-link
2. Support for Link Abbreviations
3. Support for Custom Hyperlink Types

In addition to being much more maintainable and easier to extend in future.

[SlayerOfTheBad]

List of issues this would resolve/make easier to resolve:
#387
#783
#506
#593 (maybe)
#556
#228
#578

[seflue]

I really appreciate this refactoring. As the author of the original Url class I also spent a lot of time with the hyperlink feature (the situation before was even more constraining), and I am very happy, that you're driving this more forward.

Currently a couple of tests are still failing - I didn't hat the time to take a deeper look. Is this mainly because of changed API/changed assumptions? If so and you consider this feature as usable, I would be very happy to merge it on my personal development branch and test it during my daily work.

I also took a brief look into the code. It looks promising, but I hadn't had the time for a focused review.

The last saying has @kristijanhusak. I think, he is also a bit busy right now - but I hope he can just have a brief look soon to look, if your PR is aligned with his vision of orgmode, so you can continue and polish your PR.

[SlayerOfTheBad]

Thank you for having a look @seflue!
And yeah the failing tests are because i haven't had time to fix them yet with the changed behaviour.

I have done some brief testing of the major features I believe to have touched, and they all seem to work properly.

It'd be very useful if you could do some more extensive testing, and I'd greatly appreciate it!
So be aware that it currently doesn't work with org-roam.nvim, so if you use that, you might have to disable it, or make a local, compatible version (I think there's like three calls to link.url:is_id() or something, that should be fixed by replacing then with not (link.id == nil))

[seflue]

Are you planning to make a PR to org-roam too? Actually I am using org-roam quite extensively. Can I help you to craft an PR? I am also contributing to this project.

It would be good, to have the PR in for org-roam ready to merge when your PR in Orgmode gets merged, to keep the implications for org-roam users at a minimum.

[SlayerOfTheBad]

I am planning on making a PR to Org-Roam too, as in a user of it too. I just happen to be on vacation until Saturday so i dont currently have time to work on it.

Feel free to make a PR/local version of Org-roam that works with this refactor. That'd be greatly appreciated, and would probably help in getting this merged sooner rather than later.

[kristijanhusak]

Hey, thanks for the PR!
I really appreciate the effort, but I had something different in mind.
My idea is to have very similar structure to what we currently have for autocompletion (see https://github.com/nvim-orgmode/orgmode/tree/master/lua/orgmode/org/autocompletion).
This PR has similar ideas, but it's not structured in that way.

If you would like to attempt to do the changes, I'd prefer if we do this in several steps (PRs), so it's easier to review and catch any potential bugs:

1. Create a separate folder that will hold all the structure, but is not actively used in the code. It should be testable manually by calling it directly
2. Introduce the usage of the new code internally, without exposing the extension through configuration
3. Expose a configuration option (or an api method) to add custom link handlers.

[SlayerOfTheBad]

Thank you for having a look @kristijanhusak!
I've got some questions to try and ensure we have the same ideas in mind.

What do you mean exactly with "a similar structure to autocompletion"?
Do you mean that we have a registry class (like OrgCompletion) which holds the different link types internally?
If so, this should be fairly easy to accomplish. I basically do this already, but I use config.hyperlinks as the registry, instead of having a dedicated class.

I would also like to know how you imagine the usage of the current link handling (Primarily OrgUrl) during this transitional period.
I understand the desire for a gradual phase-out, but I also do not believe it would be a good idea to have multiple different implementations for the same link type.

Do you have any suggestions on how to handle this gracefully? I would be more than willing to take on the work, especially since I'm now very familiar with how all the hyperlink stuff works.

[chipsenkbeil]

As this is a breaking change, @kristijanhusak do you plan to bump the released version of this plugin when this rolls out? So far, org-roam.nvim has been compatible with the latest version of this plugin without needing to support new and old logic.

If this is going to be a break, having a tag so I can publish a new version of org-roam.nvim matching that tag would be great 😄

[kristijanhusak]

What do you mean exactly with "a similar structure to autocompletion"?

In the autocompletion that I linked https://github.com/nvim-orgmode/orgmode/blob/master/lua/orgmode/org/autocompletion/init.lua, there is a "master" OrgCompletion class that has a list of sources, and orchestrates everything. In completions example, it knows how to figure out the context, collect all sources for the completion and return it.

I'd like to have a similar thing for hyperlinks, basically:

1. Have an OrgHyperlink class that has a list of sources (for start only internal ones), like CustomId, Id, File, etc. that adhere to the same interface. You do similar thing here but structured differently
2. Have a handler that will know how to find the matching source for the link, and handle it (for example follow, autocomplete, etc). This should work similar to how we collect autocompletions in this section https://github.com/nvim-orgmode/orgmode/blob/master/lua/orgmode/org/autocompletion/init.lua#L43. By similar, I mean there should be no switch case or some hardcoded logic. Sources should be dynamic, and the handler should have a matcher to find what it needs.
3. This class needs to be initialized and accept the files (and any other dependencies), and attached to the main org instance.

To give you an example of usage. Once we have this, and we want to autocomplete all of the links that match a partial string, we would do this:

local autocompletion_string = 'file:/home`

local results = OrgHyperlink:complete(autocompletion_string)

--- results = OrgHyperlinkSource[]
--- where OrgHyperlinkSource is something like this:

---@class OrgHyperlinkSource
---@field protocol string - id,file,headline
---@field path
--- and other similar fields that `OrgUrl` currently have

I would also like to know how you imagine the usage of the current link handling (Primarily OrgUrl) during this transitional period.

OrgUrl is somewhat of a God class that holds all internal hyperlinks logic at this moment. With this (and other) PRs, we would have it split across multiple classes, which I refer above as sources. None of the current logic should be changed. We should just have new code that works from top to bottom, and we will just swap usages in the mappings and everywhere. I know it's not that straightforward, but just explaining the idea.

I understand the desire for a gradual phase-out, but I also do not believe it would be a good idea to have multiple different implementations for the same link type.

As mentioned, current code is not touched, we just write the new one. I don't expect any bigger changes to the OrgUrl or Hyperlink at this moment.

Do you have any suggestions on how to handle this gracefully?

Yeah, basically these steps (high level):

1. Create a Hyperlink class as explained above, with the internal sources and stucture (interface) around it
2. Once that's deployed, Swap the usages that we currently have with the new code
3. Deprecate/Remove old code

@chipsenkbeil No worries, I don't plan to release any breaking changes without a notice.

[SlayerOfTheBad]

---@class OrgHyperlinkSource
---@field protocol string - id,file,headline
---@field path
--- and other similar fields that `OrgUrl` currently have

Are we sure we want every (Hyper)link source to have all the fields the current OrgUrl class has? I think they should only contain the fields relevant to themselves, and you can check whether the field exists on attempting to access it (non-existant fields will give nil anyways).

One of the reasons I wanted to restructure this OrgUrl is because it contains too many of these fields, because it's responsible for too many disparate things currently. I think it's much more extensible to check whether the returned link has link.id than make sure every type of link implements link:is_id() and link:get_id().

[kristijanhusak]

Are we sure we want every (Hyper)link source to have all the fields the current OrgUrl class has? I think they should only contain the fields relevant to themselves, and you can check whether the field exists on attempting to access it (non-existant fields will give nil anyways).

That's correct, we don't want to have all of them. We just need to figure out what they all share.
Looking at the OrgUrl class should give you an idea how to split it. For example, we have path_type and target type that are enums.
There are some catches there because a headline can either be from file, or directly with *. In this case, I would have only a OrgHyperlinkHeadline source, that would have a reference to the file that it operates on. If it's a file:/path/to/file::*headline, then the reference is /path/to/file`. Otherwise, the reference is the current working file. Just giving an idea.

Ideally we should have the interface with methods that are generic, like get_type and get_target, instead of going for get_id and similar.

[SlayerOfTheBad]

Thanks for all the responses @kristijanhusak! I'm currently working on restructuring my current code to the way you had in mind (similar to OrgCompletion). I might have some questions as I run into unresolved questions.

Previously you suggested

local results = OrgHyperlink:complete(autocompletion_string)

--- results = OrgHyperlinkSource[]

I originally used a structure similar to this, but ended up falling back to returning string[]. This made it easier to provide partial completion (Such as only completing the 'protocol' (Bit before the :) in the same way Emacs does on org-link-insert.
Is this alright, or do you have a suggestion in mind to mitigate this issue?

[kristijanhusak]

Is this alright, or do you have a suggestion in mind to mitigate this issue?

Start with what you can work with, and we can expand on it later if necessary.

[SlayerOfTheBad]

I've made a branch on my fork to work on switching what I have to a structure similar to orgmode.org.autocompletion.

If you could look and confirm whether this is more like you had in mind, I would greatly appreciate it @kristijanhusak.

The branch is links/registry, and all the new code can be found under lua/orgmode/org/links (Except for a links field addition in lua/orgmode/init.lua)

[kristijanhusak]

@SlayerOfTheBad please open up a separate PR with those changes so I can leave comments on it.

[SlayerOfTheBad]

@kristijanhusak new PR has been created, #793.

As that one superseeds this one, I will close this one.