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 vslinko/obsidian-outliner #212

Merged
merged 1 commit into from Mar 26, 2021
Merged

Add vslinko/obsidian-outliner #212

merged 1 commit into from Mar 26, 2021

Conversation

vslinko
Copy link
Contributor

@vslinko vslinko commented Mar 22, 2021

Hello!

I created plugin to work with lists like in Workflowy or RoamResearch.

I am submitting a new Community Plugin

Repo URL

https://github.com/vslinko/obsidian-outliner

Release Checklist

  • I have tested this on Windows, macOS, and Linux (if applicable)
    • Windows
    • macOS
    • Linux
  • 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 Mar 22, 2021

Woah that's really cool!

https://github.com/vslinko/obsidian-outliner/blob/44c32cdcd2f196c0b4df20f846270b869159e0e2/main.ts#L764
Don't do this (it doesn't actually work properly since you're overwriting the dispose function every time there's a new editor). You're also binding the entire CodeMirror instance to your function which causes it to never be garbage collected even when the app no longer uses it.

Instead use Workspace.iterateCodeMirrors in your onunload to remove the key event. https://github.com/obsidianmd/obsidian-api/blob/55946e5a6259a28c416d2d6e600a7964a86a01dd/obsidian.d.ts#L3085

Also are you sure you aren't missing any stylesheets? I don't think the editor looks like that without styles.

@vslinko
Copy link
Contributor Author

@vslinko vslinko commented Mar 22, 2021

Instead use Workspace.iterateCodeMirrors in your onunload to remove the key event.

Fixed in https://github.com/vslinko/obsidian-outliner/releases/tag/1.0.1

Also are you sure you aren't missing any stylesheets?

I have custom CSS for lists, but I think it should be released as a separate snippet.
BTW, what is the best way to release the snippet?

.cm-hmd-list-indent .cm-tab {
  position: relative;
}

.cm-hmd-list-indent .cm-tab::before {
  content: '';
  border-left: 1px solid #444;
  position: absolute;
  left: 3px;
  top: -8px;
  bottom: -3px;
}

.cm-s-obsidian .HyperMD-list-line {
  padding-top: 0.4em;
}

.cm-formatting-list-ul {
  letter-spacing: 3px;
}

.cm-formatting-list-ul:before {
  content: '•';
  position: absolute;
  margin-left: -3px;
  margin-top: -5px;
  font-size: 24px;
  color: var(--accent);
  visibility: visible !important; 
}

@pjeby
Copy link
Contributor

@pjeby pjeby commented Mar 22, 2021

Shouldn't this be registering Obsidian commands instead of directly processing keystrokes? The way it's currently doing things means that 1) users can't choose their own keys, and 2) if they've already used any of these keys for Obsidian commands, then the Obsidian command will be run simultaneous to the CodeMirror command.

@vslinko
Copy link
Contributor Author

@vslinko vslinko commented Mar 23, 2021

@pjeby I have thought about it. Of course, it would be better, but I can’t figure out how to get it to work without canceling the native events. Do you have any idea?

@pjeby
Copy link
Contributor

@pjeby pjeby commented Mar 23, 2021

As long as commands are assigned hotkeys that do not conflict with those defined by CodeMirror, then what events do you need to cancel?

As I understand it, CodeMirror has a keymap that defines the mapping of keystrokes to editing operations. Obsidian also has a hotkeymanager, that similarly maps hotkeys to Obsidian commands.

You aren't using either of these mechanisms, but simply hardcoding events. Now, it might also be that CodeMirror has some additional event handling taking place for the default behavior of certain keys, and certainly cancelling the event would be useful in such a case.

What I'm not clear on is why Obsidian's hotkey manager wouldn't capture and cancel those events instead, if you register the commands with it instead of hotwiring them directly.

@pjeby
Copy link
Contributor

@pjeby pjeby commented Mar 23, 2021

What I'm not clear on is why Obsidian's hotkey manager wouldn't capture and cancel those events instead, if you register the commands with it instead of hotwiring them directly.

Okay, I investigated this, and it turns out that Obsidian's Keymap only receives keyboard events after they are processed by CodeMirror, because it's not using capture mode, nor is it listening to CodeMirror keyboard events. This is arguably an Obsidian bug that should be remedied by adding a handler that overrides CodeMirror's handling for things that match the keymap.

I created a proof-of-concept fix by adding this to a plugin:

    onload() {
        this.registerCodeMirror((cm) => cm.on("keydown", checkObsidianKey));
        this.register(() => this.app.workspace.iterateCodeMirrors(
            (cm) => cm.off("keydown", checkObsidianKey)
        ));
        const app = this.app;
        function checkObsidianKey(cm, e) {
            const res = app.keymap.onKeyEvent(e);
            if (res === false) e.stopPropagation();
            return res;
        }
    }

Something like this should probably be added to Obsidian directly, as it is a fix for what is apparently a known issue with CodeMirror shortcut conflicts in Obsidian.

You could perhaps include this in your plugin as a temporary workaround until the known issue is fixed, as that would allow you to use Obsidian's command API, which would in turn allow users to define the hotkeys.

@lishid
Copy link
Collaborator

@lishid lishid commented Mar 23, 2021

Hmmm I agree we should override CodeMirror behavior. We will be implementing this soon.

I have custom CSS for lists, but I think it should be released as a separate snippet.
BTW, what is the best way to release the snippet?

Hmm we don't have a repo for snippets at the moment so it's probably easiest to just include in the plugin, unless you have a better idea?

@pjeby
Copy link
Contributor

@pjeby pjeby commented Mar 23, 2021

Including it with the plugin should probably have a settings toggle or something, since there exist various themes and CSS snippets that already do similar things (e.g. adding bullets or alignment lines). Since those themes and snippets are usually an attempt at comprehensive WYSIWYG-ness, people who are already using them might prefer to keep their existing list stylings. Perhaps it would be better to include the snippet in the documentation and/or link to some of the existing WYSIWYG-ish options?

@lishid
Copy link
Collaborator

@lishid lishid commented Mar 23, 2021

Oh yeah you could totally put it in the README as a code block 👍

@pjeby pjeby mentioned this pull request Mar 24, 2021
11 tasks
@vslinko
Copy link
Contributor Author

@vslinko vslinko commented Mar 25, 2021

Perhaps it would be better to include the snippet in the documentation and/or link to some of the existing WYSIWYG-ish options?

Added my snippet to the README file.

Hmmm I agree we should override CodeMirror behavior. We will be implementing this soon.

That's good. I'll register Obsidian commands after this.

@lishid
Copy link
Collaborator

@lishid lishid commented Mar 25, 2021

Could you change your PR to have your plugin at the very end - that'll help you with discoverability when users sort the plugin list as "newest first".

@vslinko vslinko force-pushed the patch-1 branch 2 times, most recently from a6dc3d6 to fcf7a8e Compare Mar 26, 2021
@vslinko
Copy link
Contributor Author

@vslinko vslinko commented Mar 26, 2021

@lishid done!

@lishid
Copy link
Collaborator

@lishid lishid commented Mar 26, 2021

Seems like you're using main branch but you didn't indicate that in the PR - that would cause the plugin to fail to download since we default to master. Either make a master branch or change your PR to include "branch": "main".

@vslinko
Copy link
Contributor Author

@vslinko vslinko commented Mar 26, 2021

@lishid done! thx

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