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 Imgur Plugin #115

Merged
merged 1 commit into from Jan 14, 2021
Merged

Add Imgur Plugin #115

merged 1 commit into from Jan 14, 2021

Conversation

gavvvr
Copy link
Contributor

@gavvvr gavvvr commented Jan 13, 2021

No description provided.

@lishid
Copy link
Collaborator

@lishid lishid commented Jan 14, 2021

https://github.com/gavvvr/obsidian-imgur-plugin/blob/c3cf8116758bf39fb6a84557dd61883bbe37c3cc/main.ts#L13
Suggest using Object.assign like we do it in the sample plugin.

https://github.com/gavvvr/obsidian-imgur-plugin/blob/c3cf8116758bf39fb6a84557dd61883bbe37c3cc/main.ts#L48
Doesn't seem like you're proxying the paste event anymore - does this mean the original paste event won't work anymore?

https://github.com/gavvvr/obsidian-imgur-plugin/blob/c3cf8116758bf39fb6a84557dd61883bbe37c3cc/main.ts#L45
This function is called each time you change the config, but it's never unhooked properly.
The underlying implementation is just:

this.app.workspace.iterateCodeMirrors(callback);
this.registerEvent(this.app.workspace.on('codemirror', callback));

You should probably register the event once onload instead of registering more events every time the setting changes.

Actually - You should probably just always register the event, and in the handler, only do something if clientId is set. Instead of binding and unbinding the original handler.

gavvvr added a commit to gavvvr/obsidian-imgur-plugin that referenced this issue Jan 14, 2021
@gavvvr
Copy link
Contributor Author

@gavvvr gavvvr commented Jan 14, 2021

Hi @lishid. Thanks for the review!

  • Object.assign - 👌🏻
  • If the image gets uploaded remotely, I do not want it to also be pasted to filesystem. So yes, I get rid of default behaviour and preserve it in cmAndHandlersMap to restore when plugin gets unloaded.
  • I see the problem with calling registerCodeMirror each time config changes

You should probably just always register the event, and in the handler, only do something if clientId is set
Agree. Please see this patch: gavvvr/obsidian-imgur-plugin@590244f

Now seems to be correct and safe. Let me know if it is fine and I will push it to main branch.

@lishid
Copy link
Collaborator

@lishid lishid commented Jan 14, 2021

If the image gets uploaded remotely, I do not want it to also be pasted to filesystem. So yes, I get rid of default behaviour and preserve it in cmAndHandlersMap to restore when plugin gets unloaded.

I'm referring to when the user is not pasting an image - the other behavior of pasting HTML (converted to markdown) gets broken. I think what you'll want to do here is to check for image uploads - if there aren't any, then use the default behavior. If there are then skip calling the default handler like you are doing right now.

@gavvvr
Copy link
Contributor Author

@gavvvr gavvvr commented Jan 14, 2021

Did not know Obsidian converts HTML to MarkDown. Cool!
Then here is another change: gavvvr/obsidian-imgur-plugin@9320629
@lishid if you approve it, I am ready to rewrite it to my main branch and make a release

@lishid
Copy link
Collaborator

@lishid lishid commented Jan 14, 2021

Looks good!

@lishid lishid merged commit 78e36c0 into obsidianmd:master Jan 14, 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