From 026023dc7a72af57c302445f703b2d0fcba6d44f Mon Sep 17 00:00:00 2001 From: Riccardo Date: Tue, 23 Feb 2021 16:59:55 +0100 Subject: [PATCH] Fix - Foam workspace update (live) (#497) * improved delta logic in graph.js fixes a bug that was due to using object.splice inside a forEach loop (sometimes stackoverflow is wrong - removing the element inside the loop will actually reduce the iterations and not all elements in the array will be visited!) * various fixes to live update of workspace model + tests * added tab size option in settings.json --- .vscode/settings.json | 1 + packages/foam-core/src/model/workspace.ts | 108 +++++++--- packages/foam-core/test/core.test.ts | 1 - .../janitor/generateLinkReferences.test.ts | 1 - .../foam-core/test/markdown-provider.test.ts | 1 - packages/foam-core/test/plugin.test.ts | 1 - packages/foam-core/test/workspace.test.ts | 184 ++++++++++++++++++ packages/foam-vscode/src/features/dataviz.ts | 2 +- .../static/graphs/default/graph.js | 17 +- 9 files changed, 277 insertions(+), 39 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index 71a2b631..9df7555c 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -21,5 +21,6 @@ "editor.defaultFormatter": "esbenp.prettier-vscode", "prettier.requireConfig": true, "editor.formatOnSave": true, + "editor.tabSize": 2, "jest.debugCodeLens.showWhenTestStateIn": ["fail", "unknown", "pass"] } diff --git a/packages/foam-core/src/model/workspace.ts b/packages/foam-core/src/model/workspace.ts index 32a382c3..2eb6646b 100644 --- a/packages/foam-core/src/model/workspace.ts +++ b/packages/foam-core/src/model/workspace.ts @@ -184,7 +184,7 @@ export class FoamWorkspace implements IDisposable { if (keepMonitoring) { workspace.disposables.push( workspace.onDidAdd(resource => { - FoamWorkspace.resolveResource(workspace, resource); + FoamWorkspace.updateLinksRelatedToAddedResource(workspace, resource); }), workspace.onDidUpdate(change => { FoamWorkspace.updateLinksForResource( @@ -194,7 +194,10 @@ export class FoamWorkspace implements IDisposable { ); }), workspace.onDidDelete(resource => { - FoamWorkspace.deleteLinksForResource(workspace, resource.uri); + FoamWorkspace.updateLinksRelatedToDeletedResource( + workspace, + resource + ); }) ); } @@ -325,16 +328,28 @@ export class FoamWorkspace implements IDisposable { const id = uriToResourceId(uri); const deleted = workspace.resources[id]; delete workspace.resources[id]; + + const name = uriToResourceName(uri); + workspace.resourcesByName[name] = workspace.resourcesByName[name].filter( + resId => resId !== id + ); + if (workspace.resourcesByName[name].length === 0) { + delete workspace.resourcesByName[name]; + } + isSome(deleted) && workspace.onDidDeleteEmitter.fire(deleted); return deleted ?? null; } public static resolveResource(workspace: FoamWorkspace, resource: Resource) { - // prettier-ignore - resource.type === 'note' && resource.links.forEach(link => { - const targetUri = FoamWorkspace.resolveLink(workspace, resource, link) - workspace = FoamWorkspace.connect(workspace, resource.uri, targetUri) - }); + if (resource.type === 'note') { + delete workspace.links[resource.uri.path]; + // prettier-ignore + resource.links.forEach(link => { + const targetUri = FoamWorkspace.resolveLink(workspace, resource, link); + workspace = FoamWorkspace.connect(workspace, resource.uri, targetUri); + }); + } return workspace; } @@ -354,32 +369,60 @@ export class FoamWorkspace implements IDisposable { } if (oldResource.type === 'note' && newResource.type === 'note') { const patch = diff(oldResource.links, newResource.links, isEqual); - workspace = patch.removed.reduce((g, link) => { - const target = workspace.resolveLink(oldResource, link); - return FoamWorkspace.disconnect(g, oldResource.uri, target); + workspace = patch.removed.reduce((ws, link) => { + const target = ws.resolveLink(oldResource, link); + return FoamWorkspace.disconnect(ws, oldResource.uri, target); }, workspace); - workspace = patch.added.reduce((g, link) => { - const target = workspace.resolveLink(newResource, link); - return FoamWorkspace.connect(g, newResource.uri, target); + workspace = patch.added.reduce((ws, link) => { + const target = ws.resolveLink(newResource, link); + return FoamWorkspace.connect(ws, newResource.uri, target); }, workspace); } return workspace; } - private static deleteLinksForResource(workspace: FoamWorkspace, uri: URI) { - delete workspace.links[uri.path]; - // we rebuild the backlinks by resolving any link that was pointing to the deleted resource - const toCheck = workspace.backlinks[uri.path]; - delete workspace.backlinks[uri.path]; + private static updateLinksRelatedToAddedResource( + workspace: FoamWorkspace, + resource: Resource + ) { + // check if any existing connection can be filled by new resource + const name = uriToResourceName(resource.uri); + if (name in workspace.placeholders) { + const placeholder = workspace.placeholders[name]; + delete workspace.placeholders[name]; + const resourcesToUpdate = workspace.backlinks[placeholder.uri.path] ?? []; + workspace = resourcesToUpdate.reduce( + (ws, res) => FoamWorkspace.resolveResource(ws, ws.get(res.source)), + workspace + ); + } - toCheck.forEach(link => { - const source = workspace.get(link.source); - source.type === 'note' && - source.links.forEach(l => { - const targetUri = FoamWorkspace.resolveLink(workspace, source, l); - workspace = FoamWorkspace.connect(workspace, uri, targetUri); - }); - }); + // resolve the resource + workspace = FoamWorkspace.resolveResource(workspace, resource); + } + + private static updateLinksRelatedToDeletedResource( + workspace: FoamWorkspace, + resource: Resource + ) { + const uri = resource.uri; + + // remove forward links from old resource + const resourcesPointedByDeletedNote = workspace.links[uri.path] ?? []; + delete workspace.links[uri.path]; + workspace = resourcesPointedByDeletedNote.reduce( + (ws, link) => FoamWorkspace.disconnect(ws, uri, link.target), + workspace + ); + + // recompute previous links to old resource + const notesPointingToDeletedResource = workspace.backlinks[uri.path] ?? []; + delete workspace.backlinks[uri.path]; + workspace = notesPointingToDeletedResource.reduce( + (ws, link) => FoamWorkspace.resolveResource(ws, ws.get(link.source)), + workspace + ); + return workspace; } private static connect(workspace: FoamWorkspace, source: URI, target: URI) { @@ -402,11 +445,20 @@ export class FoamWorkspace implements IDisposable { target: URI ) { workspace.links[source.path] = workspace.links[source.path]?.filter( - c => c.source.path === source.path && c.target.path === target.path + c => c.source.path !== source.path || c.target.path !== target.path ); + if (workspace.links[source.path].length === 0) { + delete workspace.links[source.path]; + } workspace.backlinks[target.path] = workspace.backlinks[target.path]?.filter( - c => c.source.path === source.path && c.target.path === target.path + c => c.source.path !== source.path || c.target.path !== target.path ); + if (workspace.backlinks[target.path].length === 0) { + delete workspace.backlinks[target.path]; + if (isPlaceholder(target)) { + delete workspace.placeholders[uriToPlaceholderId(target)]; + } + } return workspace; } } diff --git a/packages/foam-core/test/core.test.ts b/packages/foam-core/test/core.test.ts index 612ca917..8e48d52c 100644 --- a/packages/foam-core/test/core.test.ts +++ b/packages/foam-core/test/core.test.ts @@ -1,5 +1,4 @@ import { NoteLinkDefinition, Note, Attachment } from '../src/model/note'; -import { uriToSlug } from '../src/utils'; import { URI } from '../src/common/uri'; import { Logger } from '../src/utils/log'; diff --git a/packages/foam-core/test/janitor/generateLinkReferences.test.ts b/packages/foam-core/test/janitor/generateLinkReferences.test.ts index fefcaf45..458a164b 100644 --- a/packages/foam-core/test/janitor/generateLinkReferences.test.ts +++ b/packages/foam-core/test/janitor/generateLinkReferences.test.ts @@ -7,7 +7,6 @@ import { FileDataStore } from '../../src/services/datastore'; import { Logger } from '../../src/utils/log'; import { URI } from '../../src/common/uri'; import { FoamWorkspace } from '../../src/model/workspace'; -import { Resource } from '../../src/model/note'; import { getBasename } from '../../src/utils'; Logger.setLevel('error'); diff --git a/packages/foam-core/test/markdown-provider.test.ts b/packages/foam-core/test/markdown-provider.test.ts index 2190182f..8ccb11e6 100644 --- a/packages/foam-core/test/markdown-provider.test.ts +++ b/packages/foam-core/test/markdown-provider.test.ts @@ -199,7 +199,6 @@ date: 20-12-12 }); it('should parse empty frontmatter', () => { - const workspace = new FoamWorkspace(); const note = createNoteFromMarkdown( '/page-f.md', ` diff --git a/packages/foam-core/test/plugin.test.ts b/packages/foam-core/test/plugin.test.ts index 0db4a528..47ad81b7 100644 --- a/packages/foam-core/test/plugin.test.ts +++ b/packages/foam-core/test/plugin.test.ts @@ -1,7 +1,6 @@ import path from 'path'; import { loadPlugins } from '../src/plugins'; import { createMarkdownParser } from '../src/markdown-provider'; -import { createTestNote } from './core.test'; import { FoamConfig, createConfigFromObject } from '../src/config'; import { URI } from '../src/common/uri'; import { Logger } from '../src/utils/log'; diff --git a/packages/foam-core/test/workspace.test.ts b/packages/foam-core/test/workspace.test.ts index 956ddc9e..c487747e 100644 --- a/packages/foam-core/test/workspace.test.ts +++ b/packages/foam-core/test/workspace.test.ts @@ -422,4 +422,188 @@ describe('Updating workspace happy path', () => { expect(() => ws.get(placeholderUri('page-b'))).toThrow(); expect(ws.get(noteB.uri).type).toEqual('note'); }); + + it('removing link to placeholder should remove placeholder', () => { + const noteA = createTestNote({ + uri: '/path/to/page-a.md', + links: [{ to: '/path/to/another/page-b.md' }], + }); + const ws = new FoamWorkspace(); + ws.set(noteA).resolveLinks(); + expect(ws.get(placeholderUri('/path/to/another/page-b.md')).type).toEqual( + 'placeholder' + ); + + // update the note + const noteABis = createTestNote({ + uri: '/path/to/page-a.md', + links: [], + }); + ws.set(noteABis).resolveLinks(); + + expect(() => + ws.get(placeholderUri('/path/to/another/page-b.md')) + ).toThrow(); + }); +}); + +describe('Monitoring of workspace state', () => { + it('Update links when modifying note', () => { + const noteA = createTestNote({ + uri: '/path/to/page-a.md', + links: [{ slug: 'page-b' }], + }); + const noteB = createTestNote({ + uri: '/path/to/another/page-b.md', + links: [{ slug: 'page-c' }], + }); + const noteC = createTestNote({ + uri: '/path/to/more/page-c.md', + }); + const ws = new FoamWorkspace(); + ws.set(noteA) + .set(noteB) + .set(noteC) + .resolveLinks(true); + + expect(ws.getLinks(noteA.uri)).toEqual([noteB.uri]); + expect(ws.getBacklinks(noteB.uri)).toEqual([noteA.uri]); + expect(ws.getBacklinks(noteC.uri)).toEqual([noteB.uri]); + + // update the note + const noteABis = createTestNote({ + uri: '/path/to/page-a.md', + links: [{ slug: 'page-c' }], + }); + ws.set(noteABis); + + expect(ws.getLinks(noteA.uri)).toEqual([noteC.uri]); + expect(ws.getBacklinks(noteB.uri)).toEqual([]); + expect( + ws + .getBacklinks(noteC.uri) + .map(link => link.path) + .sort() + ).toEqual(['/path/to/another/page-b.md', '/path/to/page-a.md']); + }); + + it('Removing target note should produce placeholder for wikilinks', () => { + const noteA = createTestNote({ + uri: '/path/to/page-a.md', + links: [{ slug: 'page-b' }], + }); + const noteB = createTestNote({ + uri: '/path/to/another/page-b.md', + }); + const ws = new FoamWorkspace(); + ws.set(noteA) + .set(noteB) + .resolveLinks(true); + + expect(ws.getLinks(noteA.uri)).toEqual([noteB.uri]); + expect(ws.getBacklinks(noteB.uri)).toEqual([noteA.uri]); + expect(ws.get(noteB.uri).type).toEqual('note'); + + // remove note-b + ws.delete(noteB.uri); + + expect(() => ws.get(noteB.uri)).toThrow(); + expect(ws.get(placeholderUri('page-b')).type).toEqual('placeholder'); + }); + + it('Adding note should replace placeholder for wikilinks', () => { + const noteA = createTestNote({ + uri: '/path/to/page-a.md', + links: [{ slug: 'page-b' }], + }); + const ws = new FoamWorkspace(); + ws.set(noteA).resolveLinks(true); + + expect(ws.getLinks(noteA.uri)).toEqual([placeholderUri('page-b')]); + expect(ws.get(placeholderUri('page-b')).type).toEqual('placeholder'); + + // add note-b + const noteB = createTestNote({ + uri: '/path/to/another/page-b.md', + }); + + ws.set(noteB); + + expect(() => ws.get(placeholderUri('page-b'))).toThrow(); + expect(ws.get(noteB.uri).type).toEqual('note'); + }); + + it('Removing target note should produce placeholder for direct links', () => { + const noteA = createTestNote({ + uri: '/path/to/page-a.md', + links: [{ to: '/path/to/another/page-b.md' }], + }); + const noteB = createTestNote({ + uri: '/path/to/another/page-b.md', + }); + const ws = new FoamWorkspace(); + ws.set(noteA) + .set(noteB) + .resolveLinks(true); + + expect(ws.getLinks(noteA.uri)).toEqual([noteB.uri]); + expect(ws.getBacklinks(noteB.uri)).toEqual([noteA.uri]); + expect(ws.get(noteB.uri).type).toEqual('note'); + + // remove note-b + ws.delete(noteB.uri); + + expect(() => ws.get(noteB.uri)).toThrow(); + expect(ws.get(placeholderUri('/path/to/another/page-b.md')).type).toEqual( + 'placeholder' + ); + }); + + it('Adding note should replace placeholder for direct links', () => { + const noteA = createTestNote({ + uri: '/path/to/page-a.md', + links: [{ to: '/path/to/another/page-b.md' }], + }); + const ws = new FoamWorkspace(); + ws.set(noteA).resolveLinks(true); + + expect(ws.getLinks(noteA.uri)).toEqual([ + placeholderUri('/path/to/another/page-b.md'), + ]); + expect(ws.get(placeholderUri('/path/to/another/page-b.md')).type).toEqual( + 'placeholder' + ); + + // add note-b + const noteB = createTestNote({ + uri: '/path/to/another/page-b.md', + }); + + ws.set(noteB); + + expect(() => ws.get(placeholderUri('page-b'))).toThrow(); + expect(ws.get(noteB.uri).type).toEqual('note'); + }); + + it('removing link to placeholder should remove placeholder', () => { + const noteA = createTestNote({ + uri: '/path/to/page-a.md', + links: [{ to: '/path/to/another/page-b.md' }], + }); + const ws = new FoamWorkspace(); + ws.set(noteA).resolveLinks(true); + expect(ws.get(placeholderUri('/path/to/another/page-b.md')).type).toEqual( + 'placeholder' + ); + + // update the note + const noteABis = createTestNote({ + uri: '/path/to/page-a.md', + links: [], + }); + ws.set(noteABis); + expect(() => + ws.get(placeholderUri('/path/to/another/page-b.md')) + ).toThrow(); + }); }); diff --git a/packages/foam-vscode/src/features/dataviz.ts b/packages/foam-vscode/src/features/dataviz.ts index 3146ce32..52f4277d 100644 --- a/packages/foam-vscode/src/features/dataviz.ts +++ b/packages/foam-vscode/src/features/dataviz.ts @@ -1,7 +1,7 @@ import * as vscode from 'vscode'; import * as path from 'path'; import { FoamFeature } from '../types'; -import { Foam, Logger, FoamWorkspace } from 'foam-core'; +import { Foam, Logger } from 'foam-core'; import { TextDecoder } from 'util'; import { getGraphStyle, getTitleMaxLength } from '../settings'; import { isSome } from '../utils'; diff --git a/packages/foam-vscode/static/graphs/default/graph.js b/packages/foam-vscode/static/graphs/default/graph.js index a30ea578..10c52797 100644 --- a/packages/foam-vscode/static/graphs/default/graph.js +++ b/packages/foam-vscode/static/graphs/default/graph.js @@ -99,15 +99,20 @@ const Actions = { const links = graphInfo.links; // compute graph delta, for smooth transitions we need to mutate objects in-place - const remaining = new Set(Object.keys(m.nodeInfo)); - m.data.nodes.forEach((node, index, object) => { - if (remaining.has(node.id)) { - remaining.delete(node.id); + const nodeIdsToAdd = new Set(Object.keys(m.nodeInfo)); + const nodeIndexesToRemove = new Set(); + m.data.nodes.forEach((node, index) => { + if (nodeIdsToAdd.has(node.id)) { + nodeIdsToAdd.delete(node.id); } else { - object.splice(index, 1); // delete the element + nodeIndexesToRemove.add(index); } }); - remaining.forEach(nodeId => { + // apply the delta + nodeIndexesToRemove.forEach(index => { + m.data.nodes.splice(index, 1); // delete the element + }); + nodeIdsToAdd.forEach(nodeId => { m.data.nodes.push({ id: nodeId, });