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

Initializes Slint gui #1084

Open
wants to merge 37 commits into
base: main
Choose a base branch
from
Open

Conversation

SuchAFuriousDeath
Copy link
Contributor

No description provided.

@SuchAFuriousDeath SuchAFuriousDeath requested a review from a team November 3, 2024 15:06
@ultimaweapon
Copy link
Member

Require #1083 to merge first.

@ultimaweapon
Copy link
Member

You need to change add_crate(gui) to add_crate(gui LIBRARY).

@ultimaweapon
Copy link
Member

#238 should be able to close by this PR.

@ultimaweapon
Copy link
Member

Seems like this PR is going too big. My idea is just pushing only the GUI without functionality first.

@SuchAFuriousDeath SuchAFuriousDeath marked this pull request as ready for review November 6, 2024 14:28
.github/workflows/ci-mac.yml Outdated Show resolved Hide resolved
.vscode/launch.json Outdated Show resolved Hide resolved
@github-actions github-actions bot added the S-ready Status: Ready for review label Nov 6, 2024
gui/build.rs Outdated
Comment on lines 19 to 33
fn build_bin() {
// Compile installer
slint_build::compile_with_config(
"slint/installer.slint",
CompilerConfiguration::new().with_style(String::from("fluent-dark")),
)
.unwrap();

// Compile main
slint_build::compile_with_config(
"slint/main.slint",
CompilerConfiguration::new().with_style(String::from("fluent-dark")),
)
.unwrap();
}
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to use a wildcard here? Also I think it is better to rename the folder to the other name (e.g. ui) so we don't need to use ::slint to reference the Slint crate.

Copy link
Contributor Author

@SuchAFuriousDeath SuchAFuriousDeath Nov 6, 2024

Choose a reason for hiding this comment

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

I renamed the module, but wildcards don't seem possible based on the the code: Maybe with some compile time magic

Copy link
Member

Choose a reason for hiding this comment

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

In that case we can recursive list all files in the directory.

@@ -8,6 +9,30 @@ const LINUX_INCLUDE: &str = r#"
"#;

fn main() {
if std::env::var("CARGO_FEATURE_GUI_SLINT").is_ok() {
Copy link
Member

Choose a reason for hiding this comment

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

There should be a better way than this. Feature flag should be used to enable additional code instead of using like this.

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 tried, but AFAIK:

  1. there can be only one build script per crate.
  2. There is no way to tell if we are compiling the library or the binary.

Copy link
Member

Choose a reason for hiding this comment

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

Have you try CARGO_BIN_NAME?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh 😶

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 tried CARGO_BIN_PATH, but it's not set :(.

Copy link
Member

Choose a reason for hiding this comment

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

Does Slint support including those files in Rust instead of building it here? Something like `include_slint!("file.slint").

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 don't think so. The only other option is including slint source directly in Rust, there's a macro for that

Copy link
Member

@ultimaweapon ultimaweapon Nov 8, 2024

Choose a reason for hiding this comment

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

Another idea is putting the FFI for Qt behind a feature flag and enabling it from CMake and treating both features as an addition 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.

I did that, take a look

Copy link
Member

Choose a reason for hiding this comment

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

I mean we should not treat features flags as switches here. Feature flags should be used to enable additional code instead of turning it off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-ready Status: Ready for review
Development

Successfully merging this pull request may close these issues.

2 participants