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

Remove the directories dependency #4904

Merged
merged 5 commits into from
Sep 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 5 additions & 3 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1180,6 +1180,7 @@ dependencies = [
"glow",
"glutin",
"glutin-winit",
"home",
"image",
"js-sys",
"log",
Expand All @@ -1200,6 +1201,7 @@ dependencies = [
"web-time",
"wgpu",
"winapi",
"windows-sys 0.52.0",
"winit",
]

Expand Down Expand Up @@ -2059,11 +2061,11 @@ checksum = "dfa686283ad6dd069f105e5ab091b04c62850d3e4cf5d67debad1933f55023df"

[[package]]
name = "home"
version = "0.5.5"
version = "0.5.9"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "5444c27eef6923071f7ebcc33e3444508466a76f7a2b93da00ed6e19f30c1ddb"
checksum = "e3d1354bf6b7235cb4a0576c2619fd4ed18183f689b12b006a0ee7329eeff9a5"
dependencies = [
"windows-sys 0.48.0",
"windows-sys 0.52.0",
]

[[package]]
Expand Down
2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ document-features = " 0.2.8"
glow = "0.13"
glutin = "0.32.0"
glutin-winit = "0.5.0"
home = "0.5.9"
image = { version = "0.25", default-features = false }
log = { version = "0.4", features = ["std"] }
nohash-hasher = "0.2"
Expand All @@ -95,6 +96,7 @@ wgpu = { version = "22.1.0", default-features = false, features = [
# Make the renderer `Sync` even on wasm32, because it makes the code simpler:
"fragile-send-sync-non-atomic-wasm",
] }
windows-sys = "0.52"
winit = { version = "0.30.5", default-features = false }

[workspace.lints.rust]
Expand Down
9 changes: 7 additions & 2 deletions crates/eframe/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ glow = ["dep:egui_glow", "dep:glow", "dep:glutin-winit", "dep:glutin"]

## Enable saving app state to disk.
persistence = [
"directories",
"dep:home",
"egui-winit/serde",
"egui/persistence",
"ron",
Expand Down Expand Up @@ -147,7 +147,6 @@ image = { workspace = true, features = ["png"] } # Needed for app icon
winit = { workspace = true, default-features = false, features = ["rwh_06"] }

# optional native:
directories = { version = "5", optional = true }
egui-wgpu = { workspace = true, optional = true, features = [
"winit",
] } # if wgpu is used, use it with winit
Expand All @@ -157,6 +156,7 @@ pollster = { version = "0.3", optional = true } # needed for wgpu
# this can be done at the same time we expose x11/wayland features of winit crate.
glutin = { workspace = true, optional = true }
glutin-winit = { workspace = true, optional = true }
home = { workspace = true, optional = true }
puffin = { workspace = true, optional = true }
wgpu = { workspace = true, optional = true, features = [
# Let's enable some backends so that users can use `eframe` out-of-the-box
Expand Down Expand Up @@ -184,6 +184,7 @@ objc2-app-kit = { version = "0.2.0", features = [
# windows:
[target.'cfg(any(target_os = "windows"))'.dependencies]
winapi = { version = "0.3.9", features = ["winuser"] }
windows-sys = { workspace = true, features = ["Win32_Foundation", "Win32_UI_Shell", "Win32_System_Com"] }

# -------------------------------------------
# web:
Expand Down Expand Up @@ -252,3 +253,7 @@ wgpu = { workspace = true, optional = true, features = [
# without having to explicitly opt-in to backends
"webgpu",
] }

# Native dev dependencies for testing
[target.'cfg(not(target_arch = "wasm32"))'.dev-dependencies]
directories = "5"
4 changes: 2 additions & 2 deletions crates/eframe/src/epi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -351,8 +351,8 @@ pub struct NativeOptions {
/// persisted (only if the "persistence" feature is enabled).
pub persist_window: bool,

/// The folder where `eframe` will store the app state. If not set, eframe will get the paths
/// from [directories].
/// The folder where `eframe` will store the app state. If not set, eframe will use a default
/// data storage path for each target system.
pub persistence_path: Option<std::path::PathBuf>,

/// Controls whether to apply dithering to minimize banding artifacts.
Expand Down
95 changes: 91 additions & 4 deletions crates/eframe/src/native/file_storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,80 @@ use std::{
/// [`egui::ViewportBuilder::app_id`] of [`crate::NativeOptions::viewport`]
/// or the title argument to [`crate::run_native`].
///
/// On native the path is picked using [`directories::ProjectDirs::data_dir`](https://docs.rs/directories/5.0.1/directories/struct.ProjectDirs.html#method.data_dir) which is:
/// On native, the path is:
/// * Linux: `/home/UserName/.local/share/APP_ID`
/// * macOS: `/Users/UserName/Library/Application Support/APP_ID`
/// * Windows: `C:\Users\UserName\AppData\Roaming\APP_ID`
/// * Windows: `C:\Users\UserName\AppData\Roaming\APP_ID\data`
YgorSouza marked this conversation as resolved.
Show resolved Hide resolved
pub fn storage_dir(app_id: &str) -> Option<PathBuf> {
directories::ProjectDirs::from("", "", app_id)
.map(|proj_dirs| proj_dirs.data_dir().to_path_buf())
use egui::os::OperatingSystem as OS;
use std::env::var_os;
match OS::from_target_os() {
OS::Nix => var_os("XDG_DATA_HOME")
.map(PathBuf::from)
.filter(|p| p.is_absolute())
.or_else(|| home::home_dir().map(|p| p.join(".local").join("share")))
.map(|p| {
p.join(
app_id
.to_lowercase()
.replace(|c: char| c.is_ascii_whitespace(), ""),
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't seems enough to sanitize the path 😬

https://stackoverflow.com/questions/1976007/what-characters-are-forbidden-in-windows-and-linux-directory-names

I suggest we replace all characters not in [a-zA-Z0-9_-]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually just replicating what the directories crate does.

The Linux impl changes the text to lowercase and removes spaces:

https://github.com/dirs-dev/directories-rs/blob/da65f064a8bb18d790600433fadeed17a54c793a/src/lin.rs#L92-L110

The macOS impl changes the spaces to dashes:

https://github.com/dirs-dev/directories-rs/blob/da65f064a8bb18d790600433fadeed17a54c793a/src/mac.rs#L91-L98

And the Windows impl doesn't change anything:

https://github.com/dirs-dev/directories-rs/blob/da65f064a8bb18d790600433fadeed17a54c793a/src/win.rs#L98-L100

I suppose we could be more strict, but for now I'm just trying to remove the dependency with minimal changes to the functionality.

)
}),
OS::Mac => home::home_dir().map(|p| {
p.join("Library")
.join("Application Support")
.join(app_id.replace(|c: char| c.is_ascii_whitespace(), "-"))
}),
OS::Windows => roaming_appdata().map(|p| p.join(app_id).join("data")),
OS::Unknown | OS::Android | OS::IOS => None,
}
}

// Adapted from
// https://github.com/rust-lang/cargo/blob/6e11c77384989726bb4f412a0e23b59c27222c34/crates/home/src/windows.rs#L19-L37
#[cfg(all(windows, not(target_vendor = "uwp")))]
#[allow(unsafe_code)]
fn roaming_appdata() -> Option<PathBuf> {
use std::ffi::OsString;
use std::os::windows::ffi::OsStringExt;
use std::ptr;
use std::slice;

use windows_sys::Win32::Foundation::S_OK;
use windows_sys::Win32::System::Com::CoTaskMemFree;
use windows_sys::Win32::UI::Shell::{
FOLDERID_RoamingAppData, SHGetKnownFolderPath, KF_FLAG_DONT_VERIFY,
};

extern "C" {
fn wcslen(buf: *const u16) -> usize;
}
unsafe {
let mut path = ptr::null_mut();
match SHGetKnownFolderPath(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could also use the known-folders crate, which has this function and nothing else. But it's still another dependency, which might end up pulling a different version of windows-sys, so I don't know if it's worth it.

&FOLDERID_RoamingAppData,
KF_FLAG_DONT_VERIFY as u32,
0,
&mut path,
) {
S_OK => {
let path_slice = slice::from_raw_parts(path, wcslen(path));
let s = OsString::from_wide(&path_slice);
CoTaskMemFree(path.cast());
Some(PathBuf::from(s))
}
_ => {
// Free any allocated memory even on failure. A null ptr is a no-op for `CoTaskMemFree`.
CoTaskMemFree(path.cast());
None
}
}
}
}

#[cfg(any(not(windows), target_vendor = "uwp"))]
fn roaming_appdata() -> Option<PathBuf> {
None
}

// ----------------------------------------------------------------------------
Expand Down Expand Up @@ -171,3 +238,23 @@ where
}
}
}

#[cfg(test)]
mod tests {
use super::*;

fn directories_storage_dir(app_id: &str) -> Option<PathBuf> {
directories::ProjectDirs::from("", "", app_id)
.map(|proj_dirs| proj_dirs.data_dir().to_path_buf())
}

#[test]
fn storage_path_matches_directories() {
use super::storage_dir;
for app_id in [
"MyApp", "My App", "my_app", "my-app", "My.App", "my/app", "my:app", r"my\app",
] {
assert_eq!(directories_storage_dir(app_id), storage_dir(app_id));
}
}
}
Loading