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.

Sign up for GitHub

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

Refactor build script and expose build functions for crate dependents #19

Merged
merged 9 commits into from
Oct 25, 2024
Merged

Refactor build script and expose build functions for crate dependents #19

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
07c9fb1
Extract build specific logic to build crate, refactor crate
dastansam Jul 23, 2024
b175bdd
Fix CI
dastansam Aug 5, 2024
99c0c18
Resolve comments
dastansam Aug 8, 2024
f24bb69
Remove `Config` and make constants file configurable
dastansam Aug 26, 2024
133b001
Convert `initialize_icons` to function
dastansam Aug 27, 2024
7d33e62
Move icons to `build_icons`
dastansam Aug 28, 2024
95bc7de
Update icons path
dastansam Aug 28, 2024
bfbd358
Add resource prefix constant
dastansam Oct 13, 2024
4120f43
Merge branch 'main' of github-dastansam:Relm4/icons into refactor-cra…
dastansam Oct 13, 2024
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
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ mod icon_names {

fn main() {
///...///
relm4_icons::initialize_icons(icon_names::GRESOURCE_BYTES, icon_names::APP_ID, Some(icon_names::BASE_RESOURCE_PATH));
relm4_icons::initialize_icons(icon_names::GRESOURCE_BYTES);
}
```

Expand Down
10 changes: 2 additions & 8 deletions build_icons/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,14 @@ fn main() {

let manifest_path = std::env::var("CARGO_MANIFEST_DIR").unwrap();

// `icons` is one folder higher than the manifest
let icons_path = std::path::Path::new(&manifest_path)
.join("..")
.canonicalize()
.unwrap();

println!("Icons path: {}", icons_path.display());
println!("Icons path: {}", manifest_path);

// Create file that contains the icon names as constants
let constants = format!(
"
/// Path to the shipped icons.
pub const SHIPPED_ICONS_PATH: &str = \"{}/icons\";",
icons_path.display()
manifest_path
Copy link
Contributor

Choose a reason for hiding this comment

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

This will work, but consider proper path handling, including when it has ":

    std::fs::write(
        std::path::Path::new(&out_dir).join(CONSTANTS_FILE),
        manifest_path.join("icons').canonicalize().to_str().unwrap(),
    )
    .unwrap();

Storing OsString would be even more valid, but it requires platform-specific code to decode back on read, this way it will simply fail on non-utf8 characters at build time.

);

std::fs::write(
Expand Down
8 changes: 0 additions & 8 deletions build_icons/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,14 +128,6 @@ pub fn bundle_icons<P, I, S>(
"
)
})
.chain([format!(
"pub(crate) const APP_ID: &str = \"{}\";",
app_id.unwrap_or_default()
)])
.chain([format!(
"pub(crate) const BASE_RESOURCE_PATH: &str = \"{}\";",
base_resource_path.unwrap_or_default()
)])
.collect();

std::fs::write(Path::new(&out_dir).join(constants_file), constants).unwrap();
Expand Down
17 changes: 5 additions & 12 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,24 +24,17 @@ use gtk::{
gio::{resources_register, Resource},
glib,
};
use std::path::Path;

/// Initialized the icons and registers them globally for your application.
pub fn initialize_icons<P: AsRef<Path>>(
gresource_bytes: &'static [u8],
app_id: &str,
base_resource_path: Option<P>,
) {
pub fn initialize_icons(gresource_bytes: &'static [u8]) {
let bytes = glib::Bytes::from(gresource_bytes);
let resource = Resource::from_data(&bytes).unwrap();
resources_register(&resource);

gtk::init().unwrap();
dastansam marked this conversation as resolved.
Show resolved Hide resolved

if base_resource_path.is_none() && app_id.is_empty() {
let display = gdk::Display::default().unwrap();
let theme = gtk::IconTheme::for_display(&display);
theme.add_resource_path("/org/gtkrs/icons/");
theme.add_resource_path("/org/gtkrs/icons/scalable/actions/");
}
let display = gdk::Display::default().unwrap();
let theme = gtk::IconTheme::for_display(&display);
theme.add_resource_path("/org/gtkrs/icons/");
theme.add_resource_path("/org/gtkrs/icons/scalable/actions/");
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not look like an equivalent change. It only previously applied if app_id was not specified, but now it applies unconditionally despite the fact that app ID is required.

Looking at generation code, I think you'll want to store prefix in a constant and then call initialize_icons with it. Maybe @AaronErhardt can clarify, but I think it should not be necessary to have both resource paths, just the prefix used in generation code should be sufficient.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the code in question is more a hack than an actual documented feature of GTK. It is not strictly harmful to call it unconditionally, but if the user has specified an app ID, the path of the icons can be adjusted such that it doesn't need this hack in the first place (that's why the if was there). We might as well deprecate or remove this code path entirely instad of relying on it.

}