-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add support for importing palette based on Gimp .gpl files #18
Conversation
vera/src/palette.rs
Outdated
/// Derives a palette from the given Gimp gpl file | ||
pub fn derive_from_gpl( | ||
id: &str, | ||
gpl_file: &str, |
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.
For the sake of consistency with other similar functions, it might be better to accept a vec here similar to the derive_from_png
function, even if it's a little more fiddly to read a text file as bytes, send it to this function than reinterpret as text. To better support multiple file types (which I think would need to happen as soon as we add a third type), we'd ultimately want this function to be factored out into a trait which accepts data and an enum denoting what kind of input it represents and outputs a palette. So keeping the calls consistent would help toward this.
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.
In its current form the gimp_palette
crate doesn't support parsing the palette from a string or array of bytes. I was a little torn between using this crate and rolling my own; since the crate hasn't been updated since 2017 the latter is probably a better choice anyway and would allow for the approach you're describing here and below. If I were to roll my own would it be better to do as a separate crate or as a lib project on the same level as the util
and library
projects?
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.
It's such a simple file format that I think creating a function to read the bytes directly and return a vec of hex tuples right in the util
crate would be a good way to start, then if you think other people would get use out of it, break it out into its own crate later (at least that's the approach I'd take)
vera/tests/palette.rs
Outdated
#[test] | ||
fn palette_gimp() -> Result<(), Error> { | ||
init_test_logger(); | ||
// Unclear why the path needs to be different for this test compared to |
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.
include_bytes
interprets the data in a file as a byte array at compilation time, with the path would be relative to the source file. Here you seem to be loading the file during runtime, so the path is relative to the executable. Tests can be run from different directories, so loading files at runtime can lead to more configuration needs. If you change the derive_from_gpl
to accept a byte array and do the actual loading outside, this can be made consistent with the rest of the tests.
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 for the clarification!
src/cmd/palette/command.rs
Outdated
include_defaults: false, | ||
sort: false, | ||
}; | ||
VeraPalette::derive_from_gpl(&args.id, &args.input_file, &pal_config).expect("Error") |
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.
Just return the error here as originally. The ?
operator at the end of something that returns a Result either unwraps the result or returns from the function with an error if it fails, which will work so long as the error type is understood by the enclosing function. Unfortunately rust's error handling is a bit all over the place, and you need to ensure error handling is consistent throughout or you start getting unhelpful panics and difficulty tracking down errors.
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.
Fixed.
src/cmd/palette/command.rs
Outdated
direct_load: true, | ||
include_defaults: false, | ||
sort: false, | ||
let is_gpl = args.input_file.ends_with("gpl") || args.input_file.ends_with("GPL"); |
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.
Just wondering if there's a better way to do this rather than relying on file extension. I believe PNGs have magic bytes that can be used to identify them from raw data (first 8 bytes might be the same). A better workflow might be:
- Create an enum with
PNG
andGPL
variants - Load file as bytes
- Check first few bytes for png identifier or
Gimp Palette
(if that's consistent) - Call a factored out
derive
function that takes the bytes and enum type and decides where to go from there
I might be getting ahead of myself a bit here, this is an easy enough refactor to do at a later date.
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.
Per my first comment, rolling a replacement for the gimp_palette
crate is a first step toward enabling this sort of refactoring.
PNG files have an 8 byte header that could be used for identification (https://en.wikipedia.org/wiki/Portable_Network_Graphics#File_header).
Thanks again for another useful PR! Overall it looks very good. A few comments above that you might want to consider, mostly to do with consistency with the existing functions and how I might restructure a few things bit differently for future needs, but nothing major (and apologies if I'm over-explaining any rust concepts) |
Here are some updates to eliminate the dependency on the gimp_palette crate and to refactor things per your previous comments. |
All looks great, thanks very much for doing this! Going to merge then tag a new release. Perhaps even another announce on the FB page. |
This PR uses the gimp_palette crate to support importing palette based on a .gpl file.
Sample usage:
The
aloevera palette import
command now looks at the suffix of the palette filename in order to handle either .gpl files or .png files.