fix(views): engine events; update DendronTreeView reliably

[jonathanyeung]

fix(views): engine events; update DendronTreeView reliably

This change introduces a new model for updating plugin-core UI components based on an eventing system using vscode.Event-like event and EventEmitter. The change also refactors the native DendronTreeView to utilize the new events. This change provides a UI model that helps us improve UI reliability, performance, and also allows us to further unwind circular dependencies.

Engine Events

• EngineAPIService implements a new interface called EngineEvents, which exposes a new event that fires when notes have been created, updated, and deleted.
• Some fixes were made in engineClient and storev2 to have the correct data payloads for NoteChangeEvents.

Tree View

• Renamed DendronTreeView -> NativeTreeView
• Separated out TreeView controller, Engine Data Provider, and TreeNote into separate files.
• Changed the generic type of EngineNoteProvider from string to NoteProps
• The Tree View no longer exposes refresh() public methods, but instead has an instance of EngineEvents injected into its constructor. All UI updates are handle internally.

---

Pull Request Checklist

If this is your first time submitting a pull request to Dendron, copy and paste the full Dendron Review Checklist into this request and check off each item as necessary.

This template contains the short checklist which is used by the Dendron core team.

Testing

• Write Tests
• Confirm existing tests pass
• Confirm manual testing
• If your tests changes an existing snapshot, make sure that snapshots are updated
• If you are adding a new language feature (graphically visible in VS Code/preview/publishing), make sure that it is included in the test workspace

Docs

• if your change reflects documentation changes, also submit a PR to dendron-site and mention the doc PR link in your current PR

Analytics

• if you are adding analytics related changes, make sure the Telemetry docs are updated

[kevinslin]

Some thoughts:

• I like the event emitter interface and eventing
• I don't like that this is in plugin-core vs engine-server since this is something that other clients should also be able to re-use
• I don't like that this duplicates the function of HistoryInstance which is in engine-server

Some proposals:

• if we want to start off with a clean slate for eventing, we should at least move this out of plugin-core. Attach these functions to DendronEngineClient in engine-server instead
• event emitters should emit NoteChangeEntry[] as their payload, not just the Note
• can we have a rough migration plan on how we would merge HistoryInstance with the new eventing interface? (eg. interface required to fully replicate functionality )

Other comments:

• we currently have a way of "pausing" the watcher for bulk operations (eg. refactor) to avoid updating the tree view until everything is done. do we have that capability in this new interface?
• similarly, how do we stop listening to events?

[jonathanyeung]

Proposals

• Moved to engine-service; added a version of Event and EventEmitter
• consolidated create/update/delete to a single event with type <NoteChangeEntry[]>
• As Discussed offline, we can gradually move functions from HistoryService into the common EngineEvents interface (or a renamed interface)

Other Comments

• re: pausing - I intentionally left this out. I don't think any code outside of the engine/engine eventing owner should be allowed to pause events. What if a client wanted to hear those events? If we allowed pausing, then the contract between publisher and subscriber of getting notified for EngineChanges would be broken. The reason we pause today is because we're afraid UI's can't update in time. However, some clients may be able to handle the load; we can't make judgments from a controller on which subscribers can or cannot handle the event load. For those subscribers that cannot, it's the responsibility of the subscribing client to be able to handle the event volume, which they can do via their own debouncer. I added a 250 ms debouncer to NativeTreeView.
• stop listening to events: this is part of Event / vscode.Event. It returns a disposable. Calling disposable.dispose() will unsubscribe.

[SeriousBug]

Looks good to me, just one nitpick for docs.

Will this interface be replacing the HistoryInstance then? Or will we be keeping both since HistoryInstance has some features like lookback that this doesn't?

[jonathanyeung]

Will this interface be replacing the HistoryInstance then? Or will we be keeping both since HistoryInstance has some features like lookback that this doesn't?

They will eventually be converged - History has some features that this doesn't currently have, but in its current state it's a bit hard to abstract it to an interface. So I'm keeping them separate for now to reduce circular dependencies, but yeah eventually it should be converged.

[jonathanyeung]

@kevinslin - as I was working on some tests for the events, I noticed that some scenarios don't seem to have the correct payloads for what's changed in the engine, as reported by storev2. I made some fixes (but there seem to still be other pre-existing issues), but then I have questions about how the engine API's are supposed to work:

• When exactly should updateNote() be used versus writeNote()? It seems that updateNote can still be used if the note specified doesn't exist yet. Similarly, writeNote() looks like it can be called on a note that already exists.
• EngineUpdateNodesOptsV2.newNode - what is this parameter for?
• EngineWriteOptsV2.updateExisting - what constitutes updating an existing note vs. overwriting? Is it if the ID changes?
• refreshNotes / refreshNotesV2 - does this actually need to be public? Seems like it should be an operation internal to only the engine code. Same question with sync()
• renameNote currently will return a change payload with a create and a delete. Is this by-design (does file watcher need it to be this way?). Because semantically, it seems this should instead report a single updated event.

We should add docstrings to these methods detailing their exact intended behavior, especially if they are widely used API's.

[kevinslin]

@kevinslin - as I was working on some tests for the events, I noticed that some scenarios don't seem to have the correct payloads for what's changed in the engine, as reported by storev2. I made some fixes (but there seem to still be other pre-existing issues), but then I have questions about how the engine API's are supposed to work:

* When exactly should `updateNote()` be used versus `writeNote()`? It seems that `updateNote` can still be used if the note specified doesn't exist yet. Similarly, `writeNote()` looks like it can be called on a note that already exists.

* `EngineUpdateNodesOptsV2.newNode`  - what is this parameter for?

* `EngineWriteOptsV2.updateExisting` - what constitutes updating an existing note vs. overwriting?  Is it if the ID changes?

* `refreshNotes` / `refreshNotesV2` - does this actually need to be public? Seems like it should be an operation internal to only the engine code.  Same question with `sync()`

* `renameNote` currently will return a change payload with a `create` and a `delete`.  Is this by-design (does file watcher need it to be this way?).  Because semantically, it seems this should instead report a single `updated` event.

We should add docstrings to these methods detailing their exact intended behavior, especially if they are widely used API's.

We should add docstrings to these methods detailing their exact intended behavior, especially if they are widely used API's.

Yep, agreed. This has been an internal API and can definitely be improved.

When exactly should updateNote() be used versus writeNote()? It seems that updateNote can still be used if the note specified doesn't exist yet. Similarly, writeNote() looks like it can be called on a note that already exists.

The difference is if they write notes to disk. updateNote is meant to update the internal this.notes dictionary. The reason new notes are valid is because the user added the note outside of dendron, its picked up by the filewatcher, and dendron needs to know that there is a new note

writeNote is called when we need to actually write the note to the store.

EngineUpdateNodesOptsV2.newNode - what is this parameter for?

This parameter is for the case when updateNote is adding a note that was written to disk outside of dendron. we need to add parents to the note and do some other actions for this case. see [[../packages/engine-server/src/drivers/file/storev2.ts#L966]]

EngineWriteOptsV2.updateExisting - what constitutes updating an existing note vs. overwriting? Is it if the ID changes?

updateExisting is used in refactoring commands. we are writing the note to disk but want to merge it with the note that the engine already has

refreshNotes / refreshNotesV2 - does this actually need to be public? Seems like it should be an operation internal to only the engine code. Same question with sync()

refreshNotes does not. sync is called when the engine is already initialized and another client connects to it. eg. [[../packages/engine-server/src/topics/connector.ts#L139]]

renameNote currently will return a change payload with a create and a delete. Is this by-design (does file watcher need it to be this way?). Because semantically, it seems this should instead report a single updated event.

Yep. we notify that the old note was deleted and new note is created. this is used by refreshNotes and some other apis to properly update the engine.

[kevinslin]

looks like there's a build error

[jonathanyeung]

looks like there's a build error

should be patched now