-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Conversation
Good idea - this should be documented somewhere for users who will search for it |
I fixed the comment style in the Cargo.toml. Now it should be automatically added to the Feature Flags section in the docs, thanks to the document-features crate. Thank you. |
crates/eframe/Cargo.toml
Outdated
@@ -58,12 +58,15 @@ android-native-activity = ["egui-winit/android-native-activity"] | |||
## If you plan on specifying your own fonts you may disable this feature. | |||
default_fonts = ["egui/default_fonts"] | |||
|
|||
## Enable the directories dependency for a more precise file path for the persistence feature. | |||
## Not needed if you provide your own path, or if the simpler default path resolvers are good enough. | |||
directories = ["dep:directories"] |
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.
Should this be a default feature to avoid breaking changes?
How often does the naive approach fail, and directories
be required?
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.
The problem with making it default is that people using eframe without persistence would have the directories dependency even though it isn't being used for anything, unless they pass --no-default-features
.
This naive function relies on default environment variables, so it will fail on custom environments that don't have those variables. I'm not sure how often that comes up for eframe users.
Another option would be to use the home crate (which eframe already depends on indirectly) to get the paths on Linux and macOS, and a modified version of this code to get the appdata path on Windows. I think that would completely reproduce what directories does, and then it could be removed or left as a dev dependency for the unit test.
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 made the change to just replicate what directories does entirely instead of using the naive approach. If it's too much unsafe/windows-specific code to be in eframe, we could use the etcetera crate instead, as suggested in the issue. Although it is a bit outdated with respect to the home crate at the moment.
p.join( | ||
app_id | ||
.to_lowercase() | ||
.replace(|c: char| c.is_ascii_whitespace(), ""), |
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.
This doesn't seems enough to sanitize the path 😬
I suggest we replace all characters not in [a-zA-Z0-9_-]
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.
This is actually just replicating what the directories crate does.
The Linux impl changes the text to lowercase and removes spaces:
The macOS impl changes the spaces to dashes:
And the Windows impl doesn't change anything:
I suppose we could be more strict, but for now I'm just trying to remove the dependency with minimal changes to the functionality.
It is now possible to enable the persistence feature without depending on the directories crate, for uses who wish to provide their own paths from another source. When the directories feature is not enabled, a simpler naive approximation of its output is used, which covers the most common target systems.
It was wrong in the documentation as well.
8f47322
to
e78a47c
Compare
de3af89
to
8bd8072
Compare
} | ||
unsafe { | ||
let mut path = ptr::null_mut(); | ||
match SHGetKnownFolderPath( |
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.
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.
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.
Thanks!
Ultimately it would be nice if someone published all this as its own crate to reduce the amount of code in eframe
:)
directories
dependency
@emilk which part do you want to be extracted? |
Basically all the code that was added in this PR :) |
Just started a bit |
Issue tracker should be different in this line: |
The question is what to do with the |
Do we really want to make a new crate just for that? I mean, previously eframe was getting all this code from the |
eframe now has its own logic to find the storage_dir to persist the app when the persistence feature is enabled, instead of using the directories crate. The directory should be the same as before (verified with a unit test). * Closes <emilk#4884> * [x] I have followed the instructions in the PR template
eframe now has its own logic to find the storage_dir to persist the app when the persistence feature is enabled, instead of using the directories crate. The directory should be the same as before (verified with a unit test). * Closes <emilk#4884> * [x] I have followed the instructions in the PR template
eframe now has its own logic to find the storage_dir to persist the app when the persistence feature is enabled, instead of using the directories crate. The directory should be the same as before (verified with a unit test).
directories
dependency #4884