Conversation
|
This is marked a draft, so just a quick glance. I'm generally in favor of this feature. It needs to be documented and the lsp needs to support it in its path completion. Also the tree-sitter grammar needs to be adjusted. |
|
I had three more thoughts about this feature:
|
tronical
left a comment
tronical
left a comment
There was a problem hiding this comment.
Looks like a good direction - would love to have this in the next release.
Request to address the comments, #10920 (comment) ,
#10920 (comment) as well as the need for test coverage, including the errors.
| .unwrap_or(s.clone()) | ||
| }); | ||
|
|
||
| match std::fs::read_to_string(&resolved_path) { |
There was a problem hiding this comment.
I think the approach of embedding the text should be the same that we also use for images. So for Rust we use include_bytes!() (could be include_str!()), and for C++ we read the file at slint-compiler run-time and then embed it as an array or perhaps a u8 literal.
I have the feeling that this will require a dedicated Expression that can be handled in the generators as well as from_at_markdown.
tronical
left a comment
tronical
left a comment
There was a problem hiding this comment.
To summarize yesterday's call and the previous comments:
- For C++, if an included file is changed, the build system needs to re-run the Slint compiler.
- For in-app live-preview, if an included file is changed, the preview needs to be reloaded.
- For live-preview via the LSP, if an included file is changed, the preview needs to be reloaded.
- This feature needs to be documented.
- The feature needs to have test coverage.
|
|
||
| let path = std::path::Path::new(&s); | ||
| if crate::pathutils::is_absolute(path) { | ||
| ctx.diag.all_loaded_files.insert(path.to_owned()); |
There was a problem hiding this comment.
I have the feeling that this will create unnecessary dependencies:
export component Test inherits Window {
property <string> foo: @include-string("blah.txt");
// other stuff here that doesn't use foo
}
foo is unused, and as such it should get eliminated as a property, along with its binding. So afterwards touching blah.txt should not reload the file or recompile.
Perhaps this should be treated like images and glyphs, via the embedded resources infrastructure?
There was a problem hiding this comment.
Right, yeah sounds good. The existing infrastructure is a bit of a mystery to me.
internal/interpreter/live_preview.rs
Outdated
| } | ||
|
|
||
| for path in &result.all_loaded_files { | ||
| Watcher::watch(&self.watcher, path); |
There was a problem hiding this comment.
@ogoffart @LeonMatthes can you chime in here. Is this the right mechanism to watch dependencies? How does it work with images right now in the lsp?
2 participants
This is almost entirely bashed out by AI. It mostly looks good, though
parse_include_stringandfrom_at_include_string_nodecould probably be more concise. Will clean up before merging.