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

proposed fixes and enhancements #1

Merged
merged 4 commits into from Apr 13, 2021
Merged

Conversation

luckman212
Copy link
Contributor

@luckman212 luckman212 commented Apr 13, 2021

hey @Vinzent03 👋

thanks for your awesome plugin!

I had a problem and made this patch. It fixes the case that caused the plugin to fail when core Templates plugin is disabled but you still have @SilentVoid13's Templater enabled.

summary:

  • fixes a case when core Templates plugin is disabled but @SilentVoid13's Templater is enabled
  • I believe I fixed a problem that caused plugin settings not to be saved when flipping the toggles on/off
  • some changes to make it easier to add support for other Template plugins in the future
  • and a few other little tweaks in there, a bit of logging, added notice to settings panel if no template plugin is enabled in case user misses the Notice popup

Tested against Obsidian 0.11.13, macOS
Hope this is ok.

Copy link
Owner

@Vinzent03 Vinzent03 left a comment

Thanks for your contribution. I had the problem with the disabled core plugin in mind, but I must have forgotten it to fix.
In addition to my comments I want to mention, that the activated plugins are only registered on onLoad(). When the plugin is already loaded and the user activates for example Templater, he cannot see the templates to activate.

main.ts Outdated Show resolved Hide resolved
main.ts Outdated Show resolved Hide resolved
@luckman212
Copy link
Contributor Author

@luckman212 luckman212 commented Apr 13, 2021

@Vinzent03 thanks for reviewing. I see your point about the renames causing the existing hotkeys to need to be re-entered. I'll revert that, but maybe some sort of migrateSettings() function could be created that reads in the old keys and updates them to the new name? Maybe for v1.2.0.

As for dynamically rebuilding the list upon loading/unloading the plugins, that's a nice idea but I don't think there's a hook to register for plugin load/unload events? I looked at the current API and could not find one.

I asked on Discord and posted to the Obsidian forum as well.

@Vinzent03
Copy link
Owner

@Vinzent03 Vinzent03 commented Apr 13, 2021

I don't thing the need to change those names is that high that a migration is worth it.

I would just re-recognize the activated plugins in PluginSettingTab.display()

- break out plugin/template enumeration to separate func
- settings panel will trigger re-check
@luckman212
Copy link
Contributor Author

@luckman212 luckman212 commented Apr 13, 2021

ok @Vinzent03 I split out the enumeration to a separate function so it dynamically loads/updates when plugins are toggled on/off. Didn't even think that was possible but—it works! I would still like your second opinion.

2 specific things I am unsure about:

  1. should this enumerateTemplates() function be async/await? I'm not familiar enough with that to know for sure.
  2. do we need to remove all commands first before re-enumerating and pushing them back into the stack? E.g. if no plugins are toggled on/off but Settings pane is just opened/closed a bunch of times, are duplicate commands being added by addCommand(). Again, not sure and would love your feedback.

Copy link
Owner

@Vinzent03 Vinzent03 left a comment

should this enumerateTemplates() function be async/await? I'm not familiar enough with that to know for sure.

No it does not have to be, because there happens nothing async.

do we need to remove all commands first before re-enumerating and pushing them back into the stack? E.g. if no plugins are toggled on/off but Settings pane is just opened/closed a bunch of times, are duplicate commands being added by addCommand(). Again, not sure and would love your feedback.

In my experience that was not a problem. I guess they are overwriting the existing one because of the id.

main.ts Show resolved Hide resolved
main.ts Outdated Show resolved Hide resolved
main.ts Outdated Show resolved Hide resolved
@luckman212
Copy link
Contributor Author

@luckman212 luckman212 commented Apr 13, 2021

@Vinzent03 The answer to your questions above is mostly "because the compiler told me to" 😛
but I will double-check now to see if I can tighten these up.

Copy link
Owner

@Vinzent03 Vinzent03 left a comment

Looks great! Thanks again for fixing this and cleaning up the code. It was not the best before, because of the adding of the Templater plugin.

@Vinzent03 Vinzent03 merged commit dbbcd5f into Vinzent03:master Apr 13, 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
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants