Refactoring to address PR comments, eliminate gimp_palette crate depe…

…ndency

src/cmd/palette/command.rs

@@ -24,26 +24,46 @@ pub struct PaletteImportArgs {
 	pub input_file: String,
 }
 
+#[derive(Debug)]
+/// Supported palette file types
+enum PaletteFileType {
+	PNG,
+	GPL,
+}
+
+fn vec_compare(va: &[u8], vb: &[u8]) -> bool {
+	(va.len() >= vb.len()) && va.iter().zip(vb).all(|(a, b)| a == b)
+}
+
+fn determine_file_type(data: &Vec<u8>) -> Result<PaletteFileType, Error> {
+	// 8-byte PNG file header as described here: https://en.wikipedia.org/wiki/Portable_Network_Graphics#File_header
+	const PNG_BYTES: [u8; 8] = [0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a];
+	// First line of Gimp gpl file excluding line ending: "GIMP Palette"
+	const GPL_BYTES: [u8; 12] = [71, 73, 77, 80, 32, 80, 97, 108, 101, 116, 116, 101];
+
+	if vec_compare(data, &PNG_BYTES) {
+		return Ok(PaletteFileType::PNG);
+	}
+	if vec_compare(data, &GPL_BYTES) {
+		return Ok(PaletteFileType::GPL);
+	}
+	return Err(ErrorKind::ArgumentError("Invalid palette file".to_string()).into());
+}
+
 /// Palette import command
 pub fn palette_import(g_args: &GlobalArgs, args: &PaletteImportArgs) -> Result<(), Error> {
-	let is_gpl = args.input_file.ends_with("gpl") || args.input_file.ends_with("GPL");
-	let palette = match is_gpl {
-		true => {
-			let pal_config = VeraPaletteLoadConfig {
-				direct_load: true,
-				include_defaults: false,
-				sort: false,
-			};
-			VeraPalette::derive_from_gpl(&args.id, &args.input_file, &pal_config)?
-		}
-		false => {
-			let png_bytes = common::read_file_bin(&args.input_file)?;
-			let pal_config = VeraPaletteLoadConfig {
-				direct_load: true,
-				include_defaults: false,
-				sort: false,
-			};
-			VeraPalette::derive_from_png(&args.id, png_bytes, &pal_config)?
+	let pal_bytes = common::read_file_bin(&args.input_file)?;
+	let pal_config = VeraPaletteLoadConfig {
+		direct_load: true,
+		include_defaults: false,
+		sort: false,
+	};
+	let file_type = determine_file_type(&pal_bytes);
+	let palette = match file_type {
+		Ok(PaletteFileType::GPL) => VeraPalette::derive_from_gpl(&args.id, pal_bytes, &pal_config)?,
+		Ok(PaletteFileType::PNG) => VeraPalette::derive_from_png(&args.id, pal_bytes, &pal_config)?,
+		Err(s) => {
+			return Err(s);
 		}
 	};
 	// load up the project json

util/src/data/palette-gimp.gpl

@@ -0,0 +1,21 @@
+GIMP Palette
+Name: TestPalette
+Columns: 0
+#
+  0   0   0 Untitled
+ 71  71  71 Untitled
+255 255 255 Untitled
+ 34  68  17 Untitled
+136 102  17 Untitled
+ 79  16  16 Untitled
+ 79  16 176 Untitled
+ 34  34  34 Untitled
+255 255 128 Untitled
+144 255  80 Untitled
+153 153 153 Untitled
+  0   0 112 Untitled
+ 69  36  19 Untitled
+246  25  25 Untitled
+ 25 246 246 Untitled
+233  25 246 Untitled
+

util/src/gpl.rs

@@ -0,0 +1,132 @@
+// Copyright 2020 Revcore Technologies Ltd.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! Implements basic support for reading Gimp .gpl files.
+
+/// Parses a Gimp gpl file provided as a vector of bytes into a vector of (r,g,b) tuples.
+pub fn parse_gpl_from_bytes(gpl_data: Vec<u8>) -> Result<Vec<(u8, u8, u8)>, &'static str> {
+	let gpl_string = match String::from_utf8(gpl_data) {
+		Ok(s) => s,
+		_ => {
+			return Err("Invalid gpl file content");
+		}
+	};
+
+	fn is_comment(s: &str) -> bool {
+		s.chars().skip_while(|c| c.is_whitespace()).next() == Some('#')
+	}
+
+	fn validate_line_1(s: &str) -> Result<(), &'static str> {
+		if s != "GIMP Palette" {
+			return Err("Invalid gpl file line 1");
+		}
+		return Ok(());
+	}
+
+	fn validate_line_2(s: &str) -> Result<(), &'static str> {
+		if !s.starts_with("Name:") {
+			return Err("Invalid gpl file line 2");
+		}
+		return Ok(());
+	}
+
+	fn validate_line_3(s: &str) -> Result<(), &'static str> {
+		if !s.starts_with("Columns:") {
+			return Err("Invalid gpl file line 3");
+		}
+		return Ok(());
+	}
+
+	fn parse_rgb_value(s: &str) -> Result<u8, &'static str> {
+		match s.parse::<u8>() {
+			Ok(n) => Ok(n),
+			_ => Err("Failed to parse rgb value"),
+		}
+	}
+
+	let mut colors = vec![];
+	let mut line_num = 0;
+
+	for line in gpl_string.lines() {
+		line_num += 1;
+		if is_comment(&line) || line.trim().len() == 0 {
+			continue;
+		}
+		match line_num {
+			1 => {
+				validate_line_1(line)?;
+			}
+			2 => {
+				validate_line_2(line)?;
+			}
+			3 => {
+				validate_line_3(line)?;
+			}
+			_ => {
+				let mut split = line.split_whitespace();
+				match (split.next(), split.next(), split.next()) {
+					(Some(r_str), Some(g_str), Some(b_str)) => {
+						let r = parse_rgb_value(r_str)?;
+						let g = parse_rgb_value(g_str)?;
+						let b = parse_rgb_value(b_str)?;
+						colors.push((r, g, b));
+					}
+					_ => {
+						return Err("Invalid gpl file");
+					}
+				}
+			}
+		}
+	}
+
+	Ok(colors)
+}
+
+#[cfg(test)]
+mod test {
+	use super::*;
+	#[test]
+	fn test_parse_gpl_from_bytes() {
+		let test_gpl = include_bytes!("data/palette-gimp.gpl");
+
+		match parse_gpl_from_bytes(test_gpl.to_vec()) {
+			Ok(gpl) => {
+				// Note: these values aren't yet scaled down to 4-bit values
+				assert_eq!(gpl[0], (0, 0, 0));
+				assert_eq!(gpl[2], (255, 255, 255));
+				assert_eq!(gpl[4], (136, 102, 17));
+				assert_eq!(gpl[10], (153, 153, 153));
+				assert_eq!(gpl.len(), 16);
+				println!("Read colors from palette:{:?}", gpl);
+				println!("parse_gpl_from_bytes() succeeded on valid gpl file");
+			}
+			_ => {
+				println!("parse_gpl_from_bytes() failed");
+				assert_eq!(false, true)
+			}
+		}
+
+		let invalid_gpl = include_bytes!("data/create.sh");
+
+		match parse_gpl_from_bytes(invalid_gpl.to_vec()) {
+			Err(_) => {
+				println!("parse_gpl_from_bytes() failed as expected");
+			}
+			_ => {
+				println!("parse_gpl_from_bytes() unexpectedly succeeded");
+				assert_eq!(!false, true);
+			}
+		}
+	}
+}

util/src/lib.rs

@@ -38,3 +38,6 @@ pub mod logger;
 pub use crate::logger::{init_logger, init_test_logger};
 
 pub mod hex;
+
+// Parsing Gimp .gpl files
+pub mod gpl;

vera/Cargo.toml

@@ -17,5 +17,4 @@ serde = "1"
 serde_derive = "1"
 png = "0.15"
 permutate = "0.3"
-aloevera_util = { path = "../util", version = "0.2.2" }
-gimp_palette = { version = "0.1.1" }
+aloevera_util = { path = "../util", version = "0.2.2" }

vera/src/palette.rs

@@ -13,11 +13,9 @@
 
 //! Vera Palette definition
 
-extern crate gimp_palette;
-
 use crate::{Assemblable, AssembledPrimitive};
 use crate::{Error, ErrorKind};
-use gimp_palette::{NewPaletteError, Palette};
+use aloevera_util::gpl::parse_gpl_from_bytes;
 use std::fmt;
 
 const PALETTE_SIZE: usize = 256;
@@ -170,42 +168,29 @@ impl VeraPalette {
 	/// Derives a palette from the given Gimp gpl file
 	pub fn derive_from_gpl(
 		id: &str,
-		gpl_file: &str,
+		gpl_data: Vec<u8>,
 		config: &VeraPaletteLoadConfig,
 	) -> Result<Self, Error> {
-		let gimp_palette = match Palette::read_from_file(&gpl_file) {
+		let gpl_palette = match parse_gpl_from_bytes(gpl_data) {
 			Ok(p) => p,
-			Err(e) => match e {
-				NewPaletteError::NoColors => {
-					return Err(ErrorKind::GenericError(format!("No colors loaded")).into())
-				}
-				NewPaletteError::InvalidData { line_num, val } => {
-					return Err(ErrorKind::GenericError(format!(
-						"Line {} has invalid data: {}",
-						line_num, val
-					))
-					.into())
-				}
-				NewPaletteError::IoErr(io_err) => {
-					return Err(ErrorKind::GenericError(format!("I/O error {}", io_err)).into())
-				}
-			},
+			Err(s) => {
+				return Err(ErrorKind::GenericError(format!("Error: {}", s)).into());
+			}
 		};
 		debug!(
-			"Palette load: Gimp palette {} with {} colors",
-			gimp_palette.get_name(),
-			gimp_palette.get_colors().len()
+			"Palette load: Gimp palette with {} colors",
+			gpl_palette.len()
 		);
 		let mut palette = match config.include_defaults {
 			true => VeraPalette::blank_with_defaults(id),
 			false => VeraPalette::blank(id),
 		};
-		for color in gimp_palette.get_colors() {
+		for color in gpl_palette.iter() {
 			palette.add_entry(
 				config.direct_load,
-				color.r as u8,
-				color.g as u8,
-				color.b as u8,
+				color.0 as u8,
+				color.1 as u8,
+				color.2 as u8,
 			)?;
 		}
 		if config.sort {

vera/tests/palette.rs

@@ -84,17 +84,15 @@ fn palette_8bpp_indexed() -> Result<(), Error> {
 }
 
 #[test]
-fn palette_gimp() -> Result<(), Error> {
+fn palette_gpl() -> Result<(), Error> {
 	init_test_logger();
-	// Unclear why the path needs to be different for this test compared to
-	// the other tests which use include_bytes().
-	let test_gpl = "tests/data/palette/palette-gimp.gpl";
+	let test_gpl = include_bytes!("data/palette/palette-gimp.gpl");
 	let pal_config = VeraPaletteLoadConfig {
 		direct_load: true,
 		include_defaults: false,
 		sort: false,
 	};
-	let palette = VeraPalette::derive_from_gpl("pal", &test_gpl, &pal_config)?;
+	let palette = VeraPalette::derive_from_gpl("pal", test_gpl.to_vec(), &pal_config)?;
 	assert_eq!(
 		palette.value_at_index(0)?,
 		VeraPaletteEntry { r: 0, g: 0, b: 0 }