Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Enhance file: link support #112

Merged
merged 1 commit into from
Nov 1, 2021
Merged

Enhance file: link support #112

merged 1 commit into from
Nov 1, 2021

Conversation

levouh
Copy link
Contributor

@levouh levouh commented Oct 24, 2021

As a summary:

  • Complete org files on 'file:'
org_complete_file-2021-10-24_16.59.21.mp4
  • Complete headers based on current 'file:' context
org_complete_in_file-2021-10-24_17.00.11.mp4
  • Support jumping to specific section in file from hyperlink
org_jump_to_file_position-2021-10-24_17.00.53.mp4

For original Emacs functionality, see here. I still have some things to review myself and clean up before I would consider this "non-WIP", but the main functionality is there.

@levouh levouh marked this pull request as draft October 24, 2021 23:10
@levouh levouh changed the title WIP: Enhance file: link support Enhance file: link support Oct 24, 2021
Copy link
Member

@kristijanhusak kristijanhusak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now lets just move resolution to the links context instead of having a separate orgfile context, and that should generally move some things around.

@levouh
Copy link
Contributor Author

levouh commented Oct 25, 2021

Have to head out for now and hopefully didn't miss much (other than some notes I've left myself), but let me know what you think about these updates @kristijanhusak.

I'll also look at fixing the CI checks before I move it past a draft.

@levouh levouh requested a review from kristijanhusak October 26, 2021 18:09
@levouh
Copy link
Contributor Author

levouh commented Oct 26, 2021

Looks like I have some tests to fix @kristijanhusak but otherwise let me know what you think. I'll remove the draft label for now.

@kristijanhusak
Copy link
Member

@levouh Everything looks ok to me for now. Just address the tests, and run the formatter (make format) to tidy everything up, and then I'll give it a manual test.

@levouh
Copy link
Contributor Author

levouh commented Oct 27, 2021

@kristijanhusak fixed the old tests, but it turns out that they were actually pointing out a seemingly important flaw. Moving links higher up in the omni-context list solved the problem within links, but actually broke other completion options elsewhere.

The regex was getting a bit out of hand, so I added an optional extra_cond (a function) to each context that just returns a boolean-esque result that is checked. In this case, for filetags and properties, we just check that the line doesn't end with file:. This maintains order, and ensures that the file: context links still work as expected. Let me know what you think about that.


Also I started adding some tests for hyperlinks, however I'm not 100% sure which style you'd prefer. I went full unit-style (mocking out Files) instead of adding a new file to the set of fixtures/. Feedback there would be appreciated and then I'll be happy to add more tests as well.

@levouh levouh marked this pull request as ready for review October 27, 2021 14:48
@kristijanhusak
Copy link
Member

@levouh since we moved file: to the links scope and updated the regex, doesn't this mean we can put the order of contexts back? I know you mentioned changing the order here, but I'm not sure how directives steal the context since it doesn't match the : at all in the regex.

Regarding the tests, if you can do the same thing it's done for other tests in that file, that would be great. If it's too tricky, mocking is also fine.

@levouh
Copy link
Contributor Author

levouh commented Oct 27, 2021

I know you mentioned changing the order here

This is "undone" now; there is no change to the order of the contexts.

since we moved file: to the links scope and updated the regex, doesn't this mean we can put the order of contexts back?

Per above, the order is back to where it started. The problem is that [[file: was getting completion for the properties context (due to the base being a :, same things with filetags), hence why the order was changed in my original implementation so that file: would take precedence. This messes up other orderings though (as the failing test was pointing out), more specifically having links first would steal contexts from e.g. begin_blocks/directives (as links has a # at the beginning as a match, so it'd steal #+ matches).


In the latest implementation then, the order is the same as it originally was, I just prevent properties and filetags from "stealing" the context from file: by checking for each of those that the "lookbehind" isn't file:.

Make sense?

@levouh
Copy link
Contributor Author

levouh commented Oct 27, 2021

An alternative solution might be to not match on a + after the "base" (the only one we care about is a # in this case out of the 3 that links provides) for non-begin_blocks and directives items. The regex gets a bit out of hand, and it seems more explicit to just note that file: is specific to the links context in the other ones that try to trigger on :.

@kristijanhusak
Copy link
Member

Thanks, everything looks good for now. I'll give it a test manually today or tomorrow.

Copy link
Member

@kristijanhusak kristijanhusak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave this a test. It's working mostly fine, but I would like to also have more filtering after user starts typing the file.
For example, if you have ~/orgmodes/todos.org, ~/orgmodes/test.org and ~/orgmodes/work.org, and if you type [[file:/home/user/orgmodes/t]] I would expect to get only files that start with t. I explained more in the review comments.

@levouh
Copy link
Contributor Author

levouh commented Oct 31, 2021

Rebased it into a single commit @kristijanhusak, in hindsight that makes it a bit harder to review "net new" changes so apologies for that. Versus what existed before (that you'd reviewed), I've added the changes from your comments above and one other small change to ensure we modify the filename to be an absolute path before handing it off to Files.get(...) such that both relative and include things like ~ as part of the path.

@kristijanhusak kristijanhusak merged commit efeb35f into nvim-orgmode:tree-sitter Nov 1, 2021
@kristijanhusak
Copy link
Member

Everything looks good. Thanks for contributing!

@cands
Copy link

cands commented Nov 10, 2021

Thanks for this important contribution, file links are most useful. I wonder if you could extend the functionality so that some more variants for file/general links are supported, as seen in the examples here. In particular, for my use case the possibility to use custom-id and/or id (globally unique in Orgmode) for the links is fundamental, since those links can be made unique and persistent (whereas the title of the header may be changed often, rendering the link broken). There doesn't seem to be a way to link using ‘file:projects.org::#custom-id’ or complete the custom-id or id strings.

By the way, thanks very much @kristijanhusak for making this totally awesome plugin, I have been thinking to use Orgmode for quite some time but did not want to switch editor, now I got the chance and it is great. Your plugin works very well, it is very smooth and quick to work with, splendid work!

@levouh
Copy link
Contributor Author

levouh commented Nov 10, 2021

This would be relatively easy to add, can you make a new issue with the specific request @cands (and mark it as an enhancement)? I can take a look from there.

@kristijanhusak
Copy link
Member

@cands @levouh there was actually a small bug with finding the headlines by custom id. Should be fixed now.

@cands
Copy link

cands commented Nov 14, 2021

Thanks for the bug fix @kristijanhusak It fixed the possibility to use custom-id, which is great! I don't think the possibility to use id strings (by Orgmode default is an UUID string that becomes globally unique and can be found even when the headline is moved around in the different org files) is supported yet. Also, we would need the ability to store links, see 'M-x org-store-link' Handling Links (The Org Manual) to easily retrieve links by custom-id and id (from org files and agendas). Do you want me to open a new issue for this?

@kristijanhusak
Copy link
Member

@cands yeah, open up a separate issue, and please write an explanation how it exactly works in emacs orgmode. I never used it, and it's a bit hard to understand it from this documentation. I managed to store the links and insert it via prompt, but I didn't see any UUIDs or anything.

gzagatti pushed a commit to gzagatti/orgmode that referenced this pull request Oct 19, 2022
SlayerOfTheBad pushed a commit to SlayerOfTheBad/orgmode that referenced this pull request Aug 16, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants