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

Implement and provide default mappings for some Obsidian-specific Vim motions/commands #222

Merged
merged 20 commits into from
Jul 23, 2024

Conversation

alythobani
Copy link
Contributor

@alythobani alythobani commented May 11, 2024

See discussion in improved-obsidian-vimcursor, which is no longer functional as a plugin due to Obsidian switching from CodeMirror 5 to 6.

New motions/actions

This PR now has this plugin automatically provide the following vim motions/actions in Obsidian:

  • Jump to next/previous heading: [[ / ]]
  • Jump to next/previous link/wikilink: gl / gL
  • Move up/down, skipping folds: zk / zj
  • Follow link under cursor, temporarily moving cursor if necessary: gf

This PR also implements associated helper functions and types (defineAndMapObsidianVimMotion, defineAndMapObsidianVimAction, jumpToPattern, MotionFn, ObsidianActionFn) that would hopefully make it easier to add more motions/actions like these to the plugin in the future.

Not done yet

  • Update README and JSSnippets.md accordingly (want to get @esm7 's feedback here first before doing that)
  • Unit tests (could write some if desired)
    • Testing manually, the commands do seem to work well, including on edge cases e.g. cursor already on a match, and various counts to jump multiple headings/links at a time)

Screen recording

(Keystroke viewer may be a little hard/fast to see here, but including anyway in case it's useful)

Screen.Recording.2024-05-11.at.4.11.42.PM.mov

Comment on lines 5 to 6
// Polyfill for String.prototype.matchAll, in case it's not available (pre-ES2020)
matchAllShim();
Copy link
Contributor Author

@alythobani alythobani May 12, 2024

Choose a reason for hiding this comment

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

I'm guessing we don't actually need this polyfill since users are probably using a version of Obsidian that runs on a post-2020 Electron/Chromium version (which will have String.matchAll implemented). In which case we should update tsconfig to specify ES2020 for target and lib

@esm7
Copy link
Owner

esm7 commented Jul 9, 2024

First, I must express my deepest regrets for taking so long to review this. I think this is a great contribution to this plugin in so many ways, it's a shame users should have waited.
I made a few tiny updates to the branch which were required for me to build it and removed the polyfills. Other than that things seem to work well.
If you just update the plugin documentation, I think I can get this released this week.

@alythobani
Copy link
Contributor Author

alythobani commented Jul 15, 2024

Hey @esm7 sorry for the slightly late reply myself and no worries at all, I know very well how life can get in the way of things like this :) thanks for getting around to looking through this, glad things look good to you on the whole!

I'll try and find time soon to update the documentation!

@alythobani
Copy link
Contributor Author

Okay, just pushed some simple docs updates! @esm7

Things could maybe be more organized since now I guess we have some outdated code examples in JsSnippets.md, but I figured maybe this is good enough for now, and those examples can still mainly serve as demonstrations for using JS commands for custom motions. But let me know if you'd like me to make any more changes.

@esm7 esm7 merged commit d258307 into esm7:master Jul 23, 2024
@lebigot
Copy link

lebigot commented Jul 25, 2024

This is very useful, thanks!

I tried this through the 0.10.0 plugin version, and the ]] command also jumps to comments inside ``` blocks, like in:

```tasks
# …

I'm not sure if this can easily be avoided, but I thought it doesn't hurt to mention this. :)

@alythobani
Copy link
Contributor Author

Hey @lebigot glad you find it useful, and yeah thanks for pointing this out, I didn't think of this case! Someone else also noticed it and filed an issue #231 for it. I'm quite busy these next couple days, but this should definitely be fixable when I get a chance soon :) hopefully it's not the biggest annoyance in the meantime haha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants