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

Feat #879: Different embedding styles prep #1273

Merged
merged 20 commits into from
Aug 23, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
4e2f07a
Refactor note embedding to be extensible
badsketch Aug 18, 2023
e05c1c2
Refactor pattern to use more functional approach
badsketch Aug 20, 2023
33e4438
Simplify extractor formatter invocation
badsketch Aug 20, 2023
e3fe400
Prepare new setting to replace preview.embedNoteInContainer
badsketch Aug 20, 2023
3b1c903
Fix wikilink-embed e2e tests
badsketch Aug 20, 2023
7d97e78
Improve readability
badsketch Aug 22, 2023
2598689
Try to fix wikilink-embed e2e tests by returning md content instead o…
badsketch Aug 22, 2023
35536c5
Revert "Try to fix wikilink-embed e2e tests by returning md content i…
badsketch Aug 22, 2023
c1eaded
Try to fix wikilink-embed e2e tests by resetting mocks in unit tests
badsketch Aug 22, 2023
f86dc7e
Try to fix wikilink-embed e2e tests by deleting unit test
badsketch Aug 22, 2023
7caac91
Try to fix wikilink-embed e2e tests by removing extra await
badsketch Aug 22, 2023
17baa5c
Try to fix wikilink-embed e2e tests by deleting the e2e tests to chec…
badsketch Aug 22, 2023
f4e8575
Try to fix wikilink-embed e2e tests by recreating the e2e tests to ch…
badsketch Aug 22, 2023
2f5fe5e
Try to fix wikilink-embed e2e tests by restoring deleted spacing
badsketch Aug 22, 2023
4f773f9
Try to fix wikilink-embed e2e tests by removing embedNoteInContainer …
badsketch Aug 23, 2023
24a6b59
Revert "Try to fix wikilink-embed e2e tests by removing embedNoteInCo…
badsketch Aug 23, 2023
4190c43
Try to fix wikilink-embed e2e tests by fixing the name of the config …
badsketch Aug 23, 2023
090d941
Try to fix wikilink-embed e2e tests by initializing embedNoteInContai…
badsketch Aug 23, 2023
7b9c95d
Revert "Try to fix wikilink-embed e2e tests by deleting unit test"
badsketch Aug 23, 2023
7365f5d
Set embedNoteInContainer to null in e2e tests to more closely mimic l…
badsketch Aug 23, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions packages/foam-vscode/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,22 @@
"default": true,
"description": "Wrap embedded notes in a container when displayed in preview panel"
},
"foam.preview.embedStyle": {
"type": "string",
"default": "full-card",
"enum": [
"full-inline",
"full-card",
"content-inline",
"content-card"
],
"enumDescriptions": [
"Include the section with title and style inline",
"Include the section with title and style it within a container",
"Include the section without title and style inline",
"Include the section without title and style it within a container"
]
},
"foam.graph.titleMaxLength": {
"type": "number",
"default": 24,
Expand Down
125 changes: 109 additions & 16 deletions packages/foam-vscode/src/features/preview/wikilink-embed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,25 +47,20 @@ export const markdownItWikilinkEmbed = (
let content = `Embed for [[${wikilink}]]`;
switch (includedNote.type) {
case 'note': {
let noteText = readFileSync(includedNote.uri.toFsPath()).toString();
const section = Resource.findSection(
const noteStyle = getFoamVsCodeConfig(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the next phase, I'd update the regex to capture the new syntax. Then here we'd have logic to determine noteStyle and noteContent. For now, noteStyle pulls from the existing config setting, and there is no noteContent, it just defaults to "full".

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct, although from the configuration we should be getting both the style and the content, is that planned for this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'm a little fuzzy on release strategy. The new preview.embedStyle isn't used anywhere at the moment, but I can change it so that this wikilink-embed reads it and it overrides preview.embedNoteInContainer if both are set. I can also add a deprecation flag to preview.embedNoteInContainer so it no longer appears in settings UI. In the next PR, I can remove it altogether package.json as well as this file. wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The release strategy you outlined is what I had in mind and I think works pretty well.

I am not aware of a built-in way in VS Code to do that, did you come across something or had something in mind?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's great, I was already thinking of a small utility to deal with deprecation, but that won't be necessary 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweet!

CONFIG_EMBED_NOTE_IN_CONTAINER
)
? 'card'
: 'inline';
const noteEmbedder = new NoteEmbedder(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if NoteEmbedder helps encapsulate a behavior, and compare that with the additional "complexity" of the indirection - what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little confused by "encapsulate a behavior". Is that a type of implementation pattern?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no no, what I mean is basically that currently in NoteEmbedder lives the code that was in this function (that's what I mean by "encapsulating": we don't see the actual code and rely on this object to "do the work").
This indirection removes complexity by removing some code here (and putting it in NoteEmbedder) but also adds complexity because understanding the code requires an extra step (that is, check the implementation of NoteEmbedder).
My original comment was whether what's gained offsets what's lost, I wonder if we could in fact remove the NoteEmbedder and simply have the code here, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, I see what you mean. That's a good point. For e05c1c2 I changed the interfaces to function types and replaced all the classes with functions. Is this what you had in mind?

I feel like the class-based approach would be preferable after we add more variations, so maybe we can go with the functional approach and reconsider it as an option in the future? I say that because with the functional approach, it seems dependencies get added for all formatters and extractors. Like if extractor A requires parser: ResourceParser, but extractor B doesn't, the type still needs to include it, and extractor B has to have it in its signature:

export type EmbedNoteExtractor = (
  note: Resource,
  parser: ResourceParser,
) => string;

function extractorA(note: Resource, parser: resourceParser) {}
function extractorB(note: Resource, parser: resourceParser) {} // doesn't need the parser, but generateNoteEmbedding() expects a type EmbedNoteExtractor 

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about moving the two lines of generateNoteEmbedding in the caller and remove the fn altogether?

Your comment about the dependencies is correct. And in some of my functions this situation happens. For example in the feature activate function I used as example before, sometimes the Foam object is not required, but I pass it anyways.
Depending on the number of dependencies and how related they are to the method, I am ok with the compromise.

In this specific case of the extractor and the formatter, so far the parameters being passed IMO read well with the function signature, so I don't see yellow/red flags.
This is something that can be reassesed in the future shall the need arise.


As an aside, the functional way of mimicking the constructor would be using a factory, e.g.

function createExtractor(extraDep: any) : (note: Resource, parser: resourceParser) => string {
  return (note: Resource, parser: resourceParser) => {
    // do something with all extraDep, note and parser
  }
}

extractorC = createExtractor(extra)

extractorC(note, parser)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion, done! Also really appreciate the snippet

includedNote,
includedNote.uri.fragment
);
if (isSome(section)) {
const rows = noteText.split('\n');
noteText = rows
.slice(section.range.start.line, section.range.end.line)
.join('\n');
}
noteText = withLinksRelativeToWorkspaceRoot(
noteText,
'full',
noteStyle,
parser,
workspace
workspace,
md
);
content = getFoamVsCodeConfig(CONFIG_EMBED_NOTE_IN_CONTAINER)
? `<div class="embed-container-note">${md.render(noteText)}</div>`
: noteText;
content = noteEmbedder.generateEmbedding();
break;
}
case 'attachment':
Expand Down Expand Up @@ -123,4 +118,102 @@ function withLinksRelativeToWorkspaceRoot(
return text;
}

interface EmbedNoteExtractor {
extract(note: Resource): string;
}

class FullExtractor implements EmbedNoteExtractor {
parser: ResourceParser;
workspace: FoamWorkspace;

constructor(parser: ResourceParser, workspace: FoamWorkspace) {
this.parser = parser;
this.workspace = workspace;
}

extract(note: Resource) {
let noteText = readFileSync(note.uri.toFsPath()).toString();
const section = Resource.findSection(note, note.uri.fragment);
if (isSome(section)) {
const rows = noteText.split('\n');
noteText = rows
.slice(section.range.start.line, section.range.end.line)
.join('\n');
}
noteText = withLinksRelativeToWorkspaceRoot(
noteText,
this.parser,
this.workspace
);
return noteText;
}
}

badsketch marked this conversation as resolved.
Show resolved Hide resolved
interface EmbedNoteFormatter {
format(content: string): string;
}

class CardFormatter implements EmbedNoteFormatter {
md: markdownit;
constructor(md: markdownit) {
this.md = md;
}
badsketch marked this conversation as resolved.
Show resolved Hide resolved
format(content: string) {
return `<div class="embed-container-note">${this.md.render(content)}</div>`;
}
}

class InlineFormatter implements EmbedNoteFormatter {
format(content: string) {
return content;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you still wanna render here right? this.md.render(content)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the switch statement for note, attachment, and image all return content and outside the switch statement content gets rendered here https://github.com/foambubble/foam/pull/1273/files#diff-42b32f07212d469a3715b0a74b0f67ff192e592d0724fcfce358130debe8dcfaL84

It does feels weird for InlineFormatter to be doing nothing, but the Extractor kind of already does everything and I couldn't find something in

  extract(note: Resource) {
    let noteText = readFileSync(note.uri.toFsPath()).toString();
    const section = Resource.findSection(note, note.uri.fragment);
    if (isSome(section)) {
      const rows = noteText.split('\n');
      noteText = rows
        .slice(section.range.start.line, section.range.end.line)
        .join('\n');
    }
    noteText = withLinksRelativeToWorkspaceRoot(
      noteText,
      this.parser,
      this.workspace
    );
    return noteText;   // already "formatted"
  }
}

that could belong in formatter.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CardFormatter renders the md, so I feel the behavior should be consistent: do we expect the formatter to return an html string or a md string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the issue is that card style needs an additionalmd.render(content) step.
Existing code returns an md string:

  content = getFoamVsCodeConfig(CONFIG_EMBED_NOTE_IN_CONTAINER)
    ? `<div class="embed-container-note">${md.render(noteText)}</div>`
    : noteText;

So we can have both return an html string:

function cardFormatter(content: string, md: markdownit): string {
  return md.render(
    `<div class="embed-container-note">${md.render(content)}</div>`
  );
}

function inlineFormatter(content: string, md: markdownit): string {
  return md.render(content);
}

or we can have both return an md string

function cardFormatter(content: string, md: markdownit): string {
    return `<div class="embed-container-note">${md.render(content)}</div>`
}

function inlineFormatter(content: string, md: markdownit): string {
  return content;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see what you mean, I had forgotten about the double rendering.
I don't have a strong preference between the two options, so long as we are consistent, happy to go with what you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, I decided to go with producing an html and having the rest of the cases like embedding images or attachments also produce the html as well

}
}

class NoteEmbedder {
includedNote: Resource;
extractor: EmbedNoteExtractor;
formatter: EmbedNoteFormatter;

/* extractor dependencies */
parser: ResourceParser;
workspace: FoamWorkspace;

/* formatter dependencies */
md: markdownit;

constructor(
includedNote: Resource,
extractorType: string,
formatterType: string,
parser: ResourceParser,
workspace: FoamWorkspace,
md: markdownit
) {
this.includedNote = includedNote;

switch (extractorType) {
case 'full':
case 'content': // TODO: IMPLEMENT
default:
this.extractor = new FullExtractor(parser, workspace);
break;
}

switch (formatterType) {
case 'card':
this.formatter = new CardFormatter(md);
break;
case 'inline':
default:
this.formatter = new InlineFormatter();
break;
}
}

generateEmbedding() {
const rawContent = this.extractor.extract(this.includedNote);
return this.formatter.format(rawContent);
}
}

export default markdownItWikilinkEmbed;
Loading