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

Bake results of templates and queries #719

Merged
merged 1 commit into from Feb 24, 2024

Conversation

Maarrk
Copy link
Contributor

@Maarrk Maarrk commented Feb 17, 2024

This is an implementation for (#708)

I used the "bake" verb, that's how they call calculating dynamic lights and shadows, and paths to static ones in multiple game dev and 3d graphics programs. But I'm not strongly opinionated on this, can change to "Materialize".

Works: baking any block with a button

Breaks sometimes: baking the entire page - I don't know how to do multiple replacements at once. Normally I'd do them by changing ParseTree, but I don't know how to reuse the widget() functions to be compatible with that. I also tried the approach of parsing the document after every replacement in a for loop, but I was getting the old content most of the time (with the non-baked query still).

Needs a rework: This should work on every code block, for now it's hardcoded for the two built-in plugs. Would there be a way to hook it up more globally? Perhaps by finding CodeWidgetContent on the page? Also the bakeButton function is duplicated between query and template plugs, I don't like it either.

Minor improvements:

  • Make the replacement of the page a single operation, so Ctrl+Z works for all blocks at once
  • Add a command to download the baked content as .md file instead of replacing the page

@Maarrk Maarrk changed the title WIP: Bake results of templates and queries Bake results of templates and queries Feb 17, 2024
@zefhemel
Copy link
Collaborator

Man you're on fire. I like it. I'll have closer look at this later. Regarding terminology, I'm not opposed to "bake". I used materialized before since it's a term from databases (materialize views), but bake is maybe more accessible.

plugs/query/widget.ts Outdated Show resolved Hide resolved
plugs/template/widget.ts Outdated Show resolved Hide resolved
@Maarrk Maarrk force-pushed the pr-bake-button branch 2 times, most recently from 0412a9b to 5827f20 Compare February 20, 2024 23:18
@Maarrk
Copy link
Contributor Author

Maarrk commented Feb 21, 2024

Even if I find some hacky solution to update the widgets myself, the ranges are still broken after undo.

Not a bug, I just need to know how to parse the page again without refreshing it (that loses edit history)

@zefhemel
Copy link
Collaborator

This seems a bit convoluted. What I'd imagine my approach would be, requiring no new syscalls:

When the user clicks a bake button on the widget: find the node at the widget position, renderToText, then pass it to

export function render(

If this results in content with a markdown property:
Then editor.dispatch() the resulting change. If not: error.

For the command that does this for all blocks on a page: find all fenced code blocks with a tree traversal, do the as what I described for each.

@Maarrk
Copy link
Contributor Author

Maarrk commented Feb 21, 2024

That is infinitely better solution, thanks @zefhemel! I was figthing the buttons not updating position, and somehow got tunnel vision.

Now that the code is simpler, I added error messages in editor and console make the problem really clear.

Error demonstration

Manually, also on main:

  • Write some simple template, eg. {{ 2 + 2 }}
  • Write some text immediately preceding the template, and don't enter the template with cursor
  • Click the edit button

Notice that the cursor is moved before the template, instead of to the start, because of the old position passed to button


With this plug it becomes much easier to run into:

  • Have a few pages in the space
  • Paste the content below into an empty page
  • Keep the cursor outside either Live block
  • Bake with the button the first template, then the second:
  • See error notification similar to: Expected a "CodeText" node at 149 to bake, check console for more details
## Recent pages
```query
page
order by lastModified desc
limit 15
render [[Library/Core/Query/Page]]
```

```template
{{3 + 3}}
```


The button gets the position here, and doesn't update it until you enter it with the cursor.

div.querySelector(`button[data-button="${i}"]`)!.addEventListener(
"click",
(e) => {
e.stopPropagation();
console.log("Button clicked:", button.description);
this.client.clientSystem.localSyscall("system.invokeFunction", [
button.invokeFunction,
this.from,

@zefhemel
Copy link
Collaborator

zefhemel commented Feb 21, 2024

Ah I've noticed this happening but could never figure out when and why. Now I think I know it’s because of the equals check in the CM widget I think. I can fix that separately.

@Maarrk
Copy link
Contributor Author

Maarrk commented Feb 21, 2024

I can paste the repro independent of baking into a new issue to not lose it, and finish this in a moment then

@Maarrk
Copy link
Contributor Author

Maarrk commented Feb 21, 2024

I think it's ready now:

  • Implemented the command for the page
  • Made the search for the right code block more resilient to Code widget buttons use old widget position #734
    • Now also works for toc, it used to break with empty body
  • Documented usage in website
    • Described in Live Queries, since it's implemented there
    • Linked from other supported blocks

@Maarrk Maarrk marked this pull request as ready for review February 21, 2024 12:16
@Maarrk Maarrk requested a review from zefhemel February 21, 2024 12:18
Copy link
Contributor

@gorootde gorootde left a comment

Choose a reason for hiding this comment

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

I love this implementation! 👍

Added a few minor remarks to the pr

let blockNode: ParseTree;
if (!textNode) {
editor.flashNotification(
`Could not find node starting at ${pos} to bake`,
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be too low level for the user. Proposal: Output this message to the log instead, and flash a notification with something more user oriented "Cannot bake Button without text" or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was written this way because of #734. The user has {[Navigate: Move cursor to position]}, but I guess you're right with low level, there's no space to reference that here.

An option would be to wrap everything and show just one message that it failed, I agree that details should be only in the console.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure when this would occur though, so not even sure it's worth thinking TOO much about the specific error message.

blockNode = textNode;
} else {
editor.flashNotification(
`Expected a part of code block at ${pos} to bake`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as previously commented. I'd log this and instead display some more general message here.

} else {
editor.flashNotification(
`Baking node at ${pos} didn't produce any changes`,
"error",
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be an info instead of an error. Baking actually worked, but now we inform the user that nothing has changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this is only displayed if you click the button. If no changes were made, then it is a pathological condition either way:

  • Something failed during the process
  • This node has an incompatible format and shouldn't have this button at all

On the other hand, if you do the bake for the entire page and just happen to make 0 changes, I agree it's an info message.

await editor.flashNotification(`Baked ${changes.length} live blocks`);
}

async function changesForBake(
Copy link
Contributor

Choose a reason for hiding this comment

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

What about naming this collectChangesForBake to stay consistent with the util function naming?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe a rename should be done, but it's just one change, so changeForBake? I'm not collecting anything.

I didn't think about a name, since it's just a private refactoring of repeated piece of logic, but maybe it'll need to be exported to implement {[Page: Bake and download text]} 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok got it. Maybe I was just missing some inline documentation in the code and that’s why I didn’t get it in the first run 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point


# Baking
A query block can be replaced with its current output by clicking the "Bake result" button in the top right corner. This will make it a regular part of page, which will not respond to changes in the data source anymore.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can make it explicit here that this is a disruptive command: The changes will be directly performed in the current page. Please ensure to copy the page in advance, in order to not loose your queries and templates.

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 had some text about this being a normal replacement that you can undo with Ctrl+Z / Cmd+Z but I didn't see it anywhere else in the docs so dropped it.

But I'm biased by a few days thinking about that, so your opinion as some fresh eyes is much more valuable, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly when reading #708 a few days back before your PR existed, I always thought of this command creating a copy of the page by default (or even download the materialized MD without adding it to the space). Maybe that’s because I don’t see any use case for materializing something that doesn’t leave SB at all.
So if the use case we want to support is to export MD for non-SB usage, then we shouldn’t touch the original at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

download the materialized MD without adding it to the space.

I just found out that Share Ctrl+S outputs all templates as rendered, not their source code. Maybe you'll find it useful 😃

@@ -7,7 +7,7 @@ In its most basic form it looks like this (click the edit button to see the code

You can use it in two ways:

1. _Manually_, by adding a `toc` widget to the pages where you’d like to render a ToC
1. _Manually_, by adding a `toc` widget to the pages where you’d like to render a ToC (only this way can be [[Live Queries#Baking|baked]])
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather mention this as limitation in Live Queries#baking and remove it here.

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, I think this can be generalized that it won't bake any Live Template. That explanation would be better for the future

Copy link
Collaborator

@zefhemel zefhemel left a comment

Choose a reason for hiding this comment

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

My main comment is on the button ordering, other than that it looks like you wanted to address some of the comments from @gorootde . Once that's there I think this is ready to go. Cool stuff!

@@ -85,6 +85,12 @@ export async function widget(
`<svg xmlns="http://www.w3.org/2000/svg" width="15" height="15" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" class="feather feather-refresh-cw"><polyline points="23 4 23 10 17 10"></polyline><polyline points="1 20 1 14 7 14"></polyline><path d="M3.51 9a9 9 0 0 1 14.85-3.36L23 10M1 14l4.64 4.36A9 9 0 0 0 20.49 15"></path></svg>`,
invokeFunction: "index.refreshWidgets",
},
{
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 since is not a super commonly used options, it should move to the left of the list. Intuitively I'd expect the most common options to appear on the right in this context.

@@ -39,6 +48,12 @@ export async function widget(
`<svg xmlns="http://www.w3.org/2000/svg" width="15" height="15" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" class="feather feather-refresh-cw"><polyline points="23 4 23 10 17 10"></polyline><polyline points="1 20 1 14 7 14"></polyline><path d="M3.51 9a9 9 0 0 1 14.85-3.36L23 10M1 14l4.64 4.36A9 9 0 0 0 20.49 15"></path></svg>`,
invokeFunction: "query.refreshAllWidgets",
},
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

let blockNode: ParseTree;
if (!textNode) {
editor.flashNotification(
`Could not find node starting at ${pos} to bake`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure when this would occur though, so not even sure it's worth thinking TOO much about the specific error message.

@@ -91,6 +91,11 @@ export async function includeWidget(
svg:
`<svg xmlns="http://www.w3.org/2000/svg" width="15" height="15" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" class="feather feather-refresh-cw"><polyline points="23 4 23 10 17 10"></polyline><polyline points="1 20 1 14 7 14"></polyline><path d="M3.51 9a9 9 0 0 1 14.85-3.36L23 10M1 14l4.64 4.36A9 9 0 0 0 20.49 15"></path></svg>`,
invokeFunction: "query.refreshAllWidgets",
}, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

And here

@@ -142,6 +147,11 @@ export async function templateWidget(
svg:
`<svg xmlns="http://www.w3.org/2000/svg" width="15" height="15" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" class="feather feather-refresh-cw"><polyline points="23 4 23 10 17 10"></polyline><polyline points="1 20 1 14 7 14"></polyline><path d="M3.51 9a9 9 0 0 1 14.85-3.36L23 10M1 14l4.64 4.36A9 9 0 0 0 20.49 15"></path></svg>`,
invokeFunction: "query.refreshAllWidgets",
}, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

And here

@Maarrk Maarrk marked this pull request as draft February 23, 2024 12:56
@zefhemel
Copy link
Collaborator

Very nice, thanks!

@zefhemel zefhemel merged commit e63c6c1 into silverbulletmd:main Feb 24, 2024
1 check passed
@Maarrk
Copy link
Contributor Author

Maarrk commented Feb 24, 2024

A few closing comments:

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

Successfully merging this pull request may close these issues.

None yet

3 participants