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

Add Auto Link Title #249

Merged
merged 3 commits into from Apr 20, 2021
Merged

Add Auto Link Title #249

merged 3 commits into from Apr 20, 2021

Conversation

zolrath
Copy link
Contributor

@zolrath zolrath commented Apr 15, 2021

[X] I am submitting a new Community Plugin

https://github.com/zolrath/obsidian-auto-link-title

Release Checklist

  • I have tested this on Windows, macOS, and Linux (if applicable)
  • Github release contains all required files
    • main.js
    • manifest.json
    • styles.css (optional)
  • Github release name matches the exact version number specified in your manifest.json (Note: Use the exact version number, don't include a prefix v)
  • The id in my manifest.json matches the id in the community-plugins.json file.
  • README clearly describes the plugins purpose and provides clear usage instructions.

@lishid
Copy link
Collaborator

@lishid lishid commented Apr 20, 2021

https://github.com/zolrath/obsidian-auto-link-title/blob/987b49d94c38d05a71e9bfa352935ea8b86c3438/main.ts#L163
Here you should be using this.app.workspace.getActiveViewOfType(MarkdownView) instead of casting to any. Make sure you test for truthyness in case the active view is null or isn't a markdown view.

https://github.com/zolrath/obsidian-auto-link-title/blob/987b49d94c38d05a71e9bfa352935ea8b86c3438/main.ts#L164
Here you should be using view.editor instead of view.sourceMode.cmEditor.

https://github.com/zolrath/obsidian-auto-link-title/blob/987b49d94c38d05a71e9bfa352935ea8b86c3438/main.ts#L2
Your plugin will error out here on the mobile apps, causing it to not load. I think while the clipboard API isn't available on mobile, the rest of your plugin can still work. What you can do is use window.require('electron').clipboard.readText right in the function it's used, and that will just error out on mobile without breaking the entire plugin.

https://github.com/zolrath/obsidian-auto-link-title/blob/987b49d94c38d05a71e9bfa352935ea8b86c3438/main.ts#L110
Please add a warning to your README.md to indicate that you are using this method to bypass CORS (and that your plugin will actually download the webpage to find the title). This helps privacy sensitive users to know what to expect.

@zolrath
Copy link
Contributor Author

@zolrath zolrath commented Apr 20, 2021

@lishid Thank you very much for the review!

Looking at the exposed Editor interface it doesn't look like it supports the creation of markers like the CodeMirror.Editor.
In order to keep the UI responsive the use of markers to allow us to immediately paste and then asynchronously replace the title with the fetched title is fairly important.

Am I missing an alternately exposed method of doing this or should I switch this to isDesktop only for now?

@lishid
Copy link
Collaborator

@lishid lishid commented Apr 20, 2021

Ah indeed it doesn't support markers. I guess if you don't want to try and support mobile that's fine. There are probably other ways to make placeholder links that you can replace easily later, such as using a unique random ID and searching for it after.

@zolrath
Copy link
Contributor Author

@zolrath zolrath commented Apr 20, 2021

Ok! I've made the changes aside from moving from cmEditor.
I've swapped it to isDesktopOnly for now and will reassess once Editor expands in features.
I've also swapped to checkCallback to allow the default paste to take over when the clipboard isn't a URL or is a URL that links to an image.

@lishid
Copy link
Collaborator

@lishid lishid commented Apr 20, 2021

I took a look at the way you're attempting to find the URL - I would highly recommend to use a regex on the line text instead of using the token information and findWordAt - the token information is tied to the CodeMirror 5 implementation and won't be available on Mobile and/or web, probably ever, because CodeMirror 6 uses a completely different parse & token system.

I am also not sure if we will ever attempt to make Marker compatible with CodeMirror 6. It does have a concept of marker, but is mostly for internal uses and would need an actual view extension to use markers.

Editor is meant to be a common API to unify CM5 and CM6 for the common things such as getting and setting selection, getting and replacing text, etc. It is not meant to include the full CM5's API for all the advanced use cases, most of which don't have an exact equivalent in CM6 anyway.

In the end, it's up to you. Restricting your plugin for desktop use only is an ok solution too.

@lishid
Copy link
Collaborator

@lishid lishid commented Apr 20, 2021

Let me know if you want it merged it the current state. The code looks fine at this point 👍

@zolrath
Copy link
Contributor Author

@zolrath zolrath commented Apr 20, 2021

I'm up for a merge at this point!
I'll look towards adapting to Editor in the future as I'm sure I'll end up wanting it on Mobile but I'll leave that as a task for future me.

@lishid lishid merged commit dd01eaa into obsidianmd:master Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants