chore: reorganize in preparation for setting multiple backgrounds #13
Conversation
|
Thanks for the PR ❤ It's indeed a better design choice! I'll review it in a couple of hours, sorry I've got a full-time job besides open-source :( |
YassinEldeeb
changed the title
No problem! I've got a full-time life besides Open Source, and it gets priority so you never know when it might interrupt. :) |
There was a problem hiding this comment.
Sorry for the wait, It was a very stressful week.
Good Job! Nice work, that's what I like to call idiomatic rust, you've made great usage of Rust's rich type system and traits.
Love the changes ❤
Can you please take a look at my suggestions below and have a discussion about them or apply them if you agree so that we can land this PR into the main branch?
| @@ -64,7 +59,6 @@ pub struct Deadliner<'a> { | |||
| textures: HashMap<&'a str, TextureHandle>, | |||
|
|
|||
| error_msg: String, | |||
| invalid_bg: bool, | |||
There was a problem hiding this comment.
That's so much better architecture, related pieces of data are now tied together in one type.
But you've actually introduced a bug here. The App state needs to store the errors, cause remember Egui uses the update method to render the app and it runs it 60 times per second to achieve 60FPS smooth apps.
Background::FromDisk { location, .. } => {
ui.horizontal(|ui| {
+ let mut is_valid = true;
if ui.button("Open file…").clicked() {
if let Some(path) = rfd::FileDialog::new().pick_file() {
let new_location = path.display().to_string();
let file_name = get_file_name_from_path(&new_location);
let file_ext = file_name.split(".").collect::<Vec<&str>>().pop().unwrap();
let supported_file_ext = ["png", "gif", "jpg", "jpeg"];
if supported_file_ext.contains(&file_ext) {
*location = new_location;
is_valid = true;
} else {
is_valid = false;
}
}
}
+ if !is_valid {
+ ui.colored_label(Color32::from_rgb(255, 48, 48), "Not an Image");
+ } else if !location.is_empty() {
+ ui.colored_label(
+ Color32::from_rgba_unmultiplied(254, 216, 67, 200),
+ get_file_name_from_path(location),
+ );
+ }
+ });
+ }So checking for a background if it was invalid or not in the update function would mean that if you've chosen an invalid file(not an image format) the error would show only for 1 frame in 1/60 second then It forgets about it after the first render.
The only place where we can persist a value across frames rendering is at the app state level.
There was a problem hiding this comment.
Here's a screen recording for illustrating what I mean
2022-06-05.15-20-22.mp4
There was a problem hiding this comment.
Ah, good catch. I just pushed another commit that uses Egui's temporary widget state to persist the validity without polluting the application state. It's a little messier in the widget GUI code, but I think I like it better than putting the extra state in the app.
Still, I could easily go either way with this one.
| pub fn get_file_name_from_path(file_path: &str) -> String { | ||
| let location_paths: Vec<&str> = file_path.split(path::MAIN_SEPARATOR).collect(); | ||
| let file_name = location_paths[location_paths.len() - 1]; | ||
|
|
||
| file_name | ||
| file_name.to_string() | ||
| } |
There was a problem hiding this comment.
btw, why you're returning a String instead of &str here and constructing a string from the string literal.
I see that we don't have any use cases for String type and we always need the return value from get_file_name_from_path to be &str in every place. Also constructing a string is slower than taking a partial value from the passed string literal, so It's also better from a performance perspective.
There was a problem hiding this comment.
I ran into a lifetime issue with the string reference and I was kind of whirlwind fixing things at the moment. I'll look at it again later, and try to see if it was actually necessary.
|
When you've some time for implementing the multiple backgrounds feature, please let me know cause I've enjoyed your code and I'd love to work with you on implementing it cause I'd love to practice Rust more and unfortunately I'm not able to do that with my coworkers at my job(we use TypeScript). P.S: the last time I've written Rust was more than 2 months! So I'd love to brush up my skills again with a nice guy like you :D |
Yeah, sure. :) I really love writing Rust. It's the language so far that's made me happiest out of Java, C#, Python, JavaScript, TypeScript, and Haxe. |
| let mut data = ui.data(); | ||
| let is_valid = data.get_temp_mut_or(ui.id(), IsValid(true)); | ||
|
|
||
| if supported_file_ext.contains(&file_ext) { | ||
| *location = new_location; | ||
| is_valid = true; | ||
|
|
||
| *is_valid = IsValid(true); | ||
| } else { | ||
| is_valid = false; | ||
| *is_valid = IsValid(false); |
There was a problem hiding this comment.
Love it! I was questioning myself for so long if something like this was possible cause I agree with you that it's so messy to store errors, and loading states at the Application level.
There was a problem hiding this comment.
Rust generated docs are so bad in terms of their searching capabilities. If you also agree that it's so hard to find stuff in rust docs, maybe you can pitch this up: rust-lang/rust#95213
There was a problem hiding this comment.
It seems like It's time for a sweet merge.
The workflow in this repository is that I've got a GitHub action workflow that listens for new GitHub releases and on every release I make, It'll build the app for all of the supported platforms, zip them and upload them to the release assets.
I won't be making a new release for this cause no features or bug fixes were added for users, just better-architected code. But in the next thing that we'll hopefully work on is gonna be the multiple wallpapers which is a useful feature that'll need a new release.
YassinEldeeb
changed the title
Hey there, I was just going to experiment with some UI update for the image countdown, and I ran into some code that I felt could benefit from a little bit of re-organization to make it easier to work with the backgrounds.
It essentially just moves all the separate variables used to track the background selection into an enum.
This is just my personal preference on organization, so totally fine if you don't like it!