-
Notifications
You must be signed in to change notification settings - Fork 615
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
material: base setup components import #6497
base: feature/material
Are you sure you want to change the base?
Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
I thought a liitle bit about this stuff. Maybe it makes sense to have a separate library import hashmap for the slint components. So sperate this one from the ones that can be set by the developer. Maybe also in a followup PR. |
'examples/gallery', | ||
'examples/material-gallery', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to change the default really? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ups, thanks
build = "build.rs" | ||
license = "MIT" | ||
publish = false | ||
description = "Slint Widgets Gallery Example" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "Slint Material Widgets Gallery Example"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sure
// SPDX-License-Identifier: MIT | ||
|
||
fn main() { | ||
slint_build::compile("ui/index.slint").unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have main.slint
, app-window.slint
, index.slint
- it would really be nice to reduce that set eventually :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I will change it.
let mut slint_dir = PathBuf::from(std::env::var_os("CARGO_MANIFEST_DIR").unwrap()); | ||
slint_dir.push("components"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an issue with this, which is that it doesn't work when the slint-compiler is used from Node.js, C++, or WASM, or the LSP. In those cases the directory doesn't exist like that anymore. That's why the compiler ships the widgets built-in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes true. OK then I need to handle this like the std-widgets
self.config.library_paths = library_paths; | ||
self.config.library_paths.extend(library_paths); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not quite right. What if the developer calls set_library_paths
twice? That should replace the previous call, shouldn't it?
I think the builtin library mapping may need to be a separate field within the compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is what I am reference in my comment below ;-)
t.deepEqual(compiler.libraryPaths, { | ||
"libfile.slint": "third_party/libfoo/ui/lib.slint", | ||
libdir: "third_party/libbar/ui/", | ||
}); | ||
t.deepEqual( | ||
compiler.libraryPaths["libfile.slint"], | ||
"third_party/libfoo/ui/lib.slint", | ||
); | ||
t.deepEqual(compiler.libraryPaths.libdir, "third_party/libbar/ui/"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this can be un-done if you separate the fields (see comment further below).
// Copyright © SixtyFPS GmbH <[email protected]> | ||
// SPDX-License-Identifier: MIT | ||
|
||
import { FilledButton } from "@slint/material.slint"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can bike shed about this later, but I think we should offer a library as full encapsulated (so @my-library
) instead of also exposing file paths. That's just asking to access internal .slint files
and end up with accidental public API.
I'd go with something like @slint/material-widgets/1
or so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought of .slint
because we are doing the same with the std-widgets. But I see not problem to do it without to file extension
Yep. If you do that in this PR, then you can also un-do the node.js changes right away. |
This is first minimal setup to give access to the
headless
andmaterial
component libraryExample
This should also be the base setup to start with the material FilledButton.
What is missing is
lsp
support. This will follow in a separate pr.