Skip to content

Desktop: Make embedded resources optional#3094

Merged
timon-schelling merged 8 commits into
masterfrom
desktop-optional-embedded-resources
Aug 28, 2025
Merged

Desktop: Make embedded resources optional#3094
timon-schelling merged 8 commits into
masterfrom
desktop-optional-embedded-resources

Conversation

@timon-schelling
Copy link
Copy Markdown
Member

@timon-schelling timon-schelling commented Aug 26, 2025

  • Why: Multiple devs where confused/annoyed by an error that came from a missing frontend dist dir.
  • Add a embedded_resources feature
  • Add a build script that sets cfg(embedded_resources) if resource dir exists
  • Emits warning when resource dir doesn't exist
  • embedded_resources are in a separate crate
    • reduces parse times for the desktop crate (dir is included as byte string, about 10 sec extra build time)
    • makes it very clear what the build script is used for
    • no build script in desktop
  • allows specifying GRAPHITE_RESOURCES when build without the embedded_resources feature
  • preparation for watch command for desktop
  • Cleans resource handler code (moved to cef internal module etc.)

@Keavon
Copy link
Copy Markdown
Member

Keavon commented Aug 26, 2025

I'm unfamiliar with what kinds of resources these are, could you give some examples please?

@github-actions github-actions Bot temporarily deployed to graphite-dev (Preview) August 26, 2025 16:51 Inactive
@timon-schelling
Copy link
Copy Markdown
Member Author

I'm unfamiliar with what kinds of resources these are, could you give some examples please?

The frontend files, frontend/dist.

Comment thread desktop/src/cef.rs Outdated

#[derive(Clone)]
pub(crate) struct Resource {
pub(crate) data: Vec<u8>,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it make sense to make this a Cow<'static, [u8]> ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The results of a fs::read cannot be 'static, see usage of this struct.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

if let Ok(data) = std::fs::read(file_path) {
    return Some(Resource { data, mimetype });
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, but include dir should give you static references, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, but this pr also adds the ability to create resources from disk. Could be behind feature flag for now, but would like to have override functionality in the future.

@github-actions github-actions Bot temporarily deployed to graphite-dev (Preview) August 28, 2025 15:51 Inactive
@github-actions github-actions Bot temporarily deployed to graphite-dev (Preview) August 28, 2025 16:13 Inactive
@github-actions github-actions Bot requested a deployment to graphite-dev (Preview) August 28, 2025 16:34 Abandoned
@github-actions github-actions Bot temporarily deployed to graphite-dev (Preview) August 28, 2025 17:01 Inactive
@timon-schelling timon-schelling merged commit 1d4d102 into master Aug 28, 2025
4 checks passed
@timon-schelling timon-schelling deleted the desktop-optional-embedded-resources branch August 28, 2025 17:18
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.

3 participants