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

Fix iced compatibility - replace fold with pick_list for choosing theme #26

Merged
merged 3 commits into from
Feb 13, 2024

Conversation

krshrimali
Copy link
Owner

@krshrimali krshrimali commented Feb 11, 2024

  • Now using pick_list for choosing theme buttons
  • Compiles successfully with versions mentioned in Cargo.toml file

Please note that all features haven't been tested, I'm right now on wayland and some issues around opening dialogs. Will land this PR once testing is successful (manual for now).

Part of the fix for #24, tests etc. will be added in another PR.

@krshrimali
Copy link
Owner Author

Tested this on my Mac system, very curious why it didn't work on Wayland - will have to check.

There are issues around "text" feature when marking an image as incorrect, and the "end" functionality has been disabled for now - need to bring that in and also for each image in case anyone wants to exit first. Can fix that in separate PR though.

cc: @eirnym - wondering if you'd be willing to help test this on your system as well? The definition for test for me over here is just ensuring that you can launch the binary, select a folder and scroll through images.

src/render_image.rs Show resolved Hide resolved
@eirnym
Copy link
Contributor

eirnym commented Feb 11, 2024

Please cleanup unused imports.

It's totally fine if you prefer use direct naming.

@eirnym
Copy link
Contributor

eirnym commented Feb 11, 2024

I'll check it out as soon as I'll have an access to a computer

@eirnym
Copy link
Contributor

eirnym commented Feb 11, 2024

I've tested on macOS M2. before testing I've run cargo clean to cleanup up old caches. It could be a reason why wayland test failed.

I found following experience:

  • I selected a folder, and pressed "next".
  • This folder contained some other files and the application tried them to open up as images (there were no defined in code filters in code, so I don't expect it working)

Later I'll try to run in on Windows when I have an access to.

@krshrimali
Copy link
Owner Author

I've tested on macOS M2. before testing I've run cargo clean to cleanup up old caches. It could be a reason why wayland test failed.

Naah, I'm pretty sure I did cargo clean before trying to run it. 🤔 Will debug this further.

I found following experience:

  • I selected a folder, and pressed "next".
  • This folder contained some other files and the application tried them to open up as images (there were no defined in code filters in code, so I don't expect it working)

^^ This is interesting, IIRC I had a check for invalid files (definition of invalid: anything that is not an image) to ensure that they aren't being loaded as images, and some different message will be shown to the users.

Here is the relevant part of the code that just returns the error with a message: "Invalid file, please check ..."

pub fn fetch_image(all_images: Vec<PathBuf>, curr_idx: &usize) -> Result<Handle, std::io::Error> {
// TODO: Set a default image to show that we are waiting for an image...// folder is empty
let formatted_string = format!("Invalid index: {}", curr_idx);
let path: PathBuf = all_images
.get(*curr_idx)
.unwrap_or_else(|| panic!("{}", formatted_string.to_string()))
.to_owned();
let img_validation_result = imghdr::from_file(path.clone());
match img_validation_result {
Ok(if_none) => match if_none {
Some(_) => Ok(Handle::from_path(path)),
None => Err(std::io::Error::new(
std::io::ErrorKind::InvalidData,
"Invalid file, please check if it is a valid image file!",
)),
},
Err(e) => Err(e),
}
}

But thank you for flagging this here, I'll check on my system (Mac) to see what's going wrong there.

Later I'll try to run in on Windows when I have an access to.

If you get time and a chance, that would be great, but if not - I can try testing this tomorrow on Windows as well.

Thank you so much for your help btw ❤️ 🚀 @eirnym

@krshrimali
Copy link
Owner Author

Screenshot 2024-02-13 at 2 21 26 PM

Just checked on my Mac system (M1) - seems to be working fine, however I'm wondering if it would make more sense to just not show these invalid files on the screen (/skip them) instead of showing this screen which is probably not useful to the user at all. Ideal discussion to have on a separate issue, will create one this week and start discussions! :)

For now, deciding to push this PR to main and keep the main clean for any new users! Thank you again for testing this and your contributions @eirnym! 🚀

@krshrimali krshrimali merged commit 3b853b4 into main Feb 13, 2024
3 checks passed
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.

2 participants