src/book/mod.rs

@@ -3,10 +3,7 @@ use std::cmp::max;
 use anyhow::{anyhow, Result};
 use ironcalc::{
     base::{
-        expressions::types::Area,
+        expressions::types::Area, types::{Border, Col, Fill, Font, Row, SheetData, Style, Worksheet}, worksheet::WorksheetDimension, Model, UserModel
-        types::{SheetData, Style, Worksheet},
-        worksheet::WorksheetDimension,
-        Model, UserModel,
     },
     export::save_xlsx_to_writer,
     import::load_from_xlsx,
@@ -17,13 +14,7 @@ use crate::ui::Address;
 #[cfg(test)]
 mod test;
 
-pub(crate) const COL_PIXELS: f64 = 5.0;
+const COL_PIXELS: f64 = 5.0;
-// TODO(zaphar): Move this to the book module.
-// NOTE(zaphar): This is stolen from ironcalc but ironcalc doesn't expose it
-// publically.
-pub(crate) const LAST_COLUMN: i32 = 16_384;
-pub(crate) const LAST_ROW: i32 = 1_048_576;
-
 
 #[derive(Debug, Clone)]
 pub struct AddressRange<'book> {
@@ -150,9 +141,7 @@ impl Book {
     }
 
     pub fn set_sheet_name(&mut self, idx: u32, sheet_name: &str) -> Result<()> {
-        self.model
+        self.model.rename_sheet(idx, sheet_name).map_err(|e| anyhow!(e))?;
-            .rename_sheet(idx, sheet_name)
-            .map_err(|e| anyhow!(e))?;
         Ok(())
     }
 
@@ -162,9 +151,7 @@ impl Book {
         if let Some(name) = sheet_name {
             self.set_sheet_name(idx, name)?;
         }
-        self.model
+        self.model.set_selected_sheet(self.current_sheet).map_err(|e| anyhow!(e))?;
-            .set_selected_sheet(self.current_sheet)
-            .map_err(|e| anyhow!(e))?;
         Ok(())
     }
 
@@ -262,8 +249,7 @@ impl Book {
 
     pub fn clear_cell_range_all(&mut self, sheet: u32, start: Address, end: Address) -> Result<()> {
         let area = calculate_area(sheet, start, end);
-        self.model
+        self.model.range_clear_all(&area)
-            .range_clear_all(&area)
             .map_err(|s| anyhow!("Unable to clear cell contents {}", s))?;
         Ok(())
     }
@@ -277,73 +263,107 @@ impl Book {
         // TODO(jwall): This is modeled a little weird. We should probably record
         // the error *somewhere* but for the user there is nothing to be done except
         // not use a style.
-        match self
+        match self.model.get_model().get_style_for_cell(sheet, cell.row as i32, cell.col as i32)
-            .model
-            .get_model()
-            .get_style_for_cell(sheet, cell.row as i32, cell.col as i32)
         {
             Err(_) => None,
             Ok(s) => Some(s),
         }
     }
 
+    fn get_column(&self, sheet: u32, col: usize) -> Result<Option<&Col>> {
+        Ok(self.model.get_model().workbook.worksheet(sheet)
+            .map_err(|e| anyhow!("{}", e))?.cols.get(col))
+    }
+
+    fn get_row(&self, sheet: u32, col: usize) -> Result<Option<&Row>> {
+        Ok(self.model.get_model().workbook.worksheet(sheet)
+            .map_err(|e| anyhow!("{}", e))?.rows.get(col))
+    }
+
+    pub fn get_column_style(&self, sheet: u32, col: usize) -> Result<Option<Style>> {
+        // TODO(jwall): This is modeled a little weird. We should probably record
+        // the error *somewhere* but for the user there is nothing to be done except
+        // not use a style.
+        if let Some(col) = self.get_column(sheet, col)? {
+            if let Some(style_idx) = col.style.map(|idx| idx as usize) {
+                let styles = &self.model.get_model().workbook.styles;
+                if styles.cell_style_xfs.len() <= style_idx {
+                    return Ok(Some(Style {
+                        alignment: None,
+                        num_fmt: styles.num_fmts[style_idx].format_code.clone(),
+                        fill: styles.fills[style_idx].clone(),
+                        font: styles.fonts[style_idx].clone(),
+                        border: styles.borders[style_idx].clone(),
+                        quote_prefix: false,
+                    }));
+                }
+            }
+        }
+        return Ok(None);
+    }
+
+    pub fn get_row_style(&self, sheet: u32, row: usize) -> Result<Option<Style>> {
+        // TODO(jwall): This is modeled a little weird. We should probably record
+        // the error *somewhere* but for the user there is nothing to be done except
+        // not use a style.
+        if let Some(row) = self.get_row(sheet, row)? {
+            let style_idx = row.s as usize;
+            let styles = &self.model.get_model().workbook.styles;
+            if styles.cell_style_xfs.len() <= style_idx {
+                return Ok(Some(Style {
+                    alignment: None,
+                    num_fmt: styles.num_fmts[style_idx].format_code.clone(),
+                    fill: styles.fills[style_idx].clone(),
+                    font: styles.fonts[style_idx].clone(),
+                    border: styles.borders[style_idx].clone(),
+                    quote_prefix: false,
+                }));
+            }
+        }
+        return Ok(None);
+    }
+
+    pub fn create_style(&mut self) -> Style {
+        Style {
+            alignment: None,
+            num_fmt: String::new(),
+            fill: Fill::default(),
+            font: Font::default(),
+            border: Border::default(),
+            quote_prefix: false,
+        }
+    }
+
     pub fn set_cell_style(&mut self, style: &[(&str, &str)], area: &Area) -> Result<()> {
         for (path, val) in style {
-            self.model
+            self.model.update_range_style(area, path, val)
-                .update_range_style(area, path, val)
                 .map_err(|s| anyhow!("Unable to format cell {}", s))?;
         }
         Ok(())
     }
 
-    fn get_col_range(&self, sheet: u32, col_idx: usize) -> Area {
+    pub fn set_col_style(&mut self, style: &[(&str, &str)], sheet: u32, col: usize) -> Result<()> {
-        Area {
+        todo!()
-            sheet,
+        //let idx = self.create_or_get_style_idx(style);
-            row: 1,
+        //let sheet = self.model.workbook.worksheet_mut(sheet)
-            column: col_idx as i32,
+        //    .map_err(|e| anyhow!("{}", e))?;
-            width: 1,
+        //let width = sheet.get_column_width(col as i32)
-            height: LAST_ROW,
+        //    .map_err(|e| anyhow!("{}", e))?;
-        }
+        //sheet.set_column_style(col as i32, idx)
+        //    .map_err(|e| anyhow!("{}", e))?;
+        //sheet.set_column_width(col as i32, width)
+        //    .map_err(|e| anyhow!("{}", e))?;
+        //Ok(())
     }
 
-    fn get_row_range(&self, sheet: u32, row_idx: usize) -> Area {
+    pub fn set_row_style(&mut self, style: &[(&str, &str)], sheet: u32, row: usize) -> Result<()> {
-        Area {
+        todo!()
-            sheet,
+        //let idx = self.create_or_get_style_idx(style);
-            row: row_idx as i32,
+        //self.model.workbook.worksheet_mut(sheet)
-            column: 1,
+        //    .map_err(|e| anyhow!("{}", e))?
-            width: LAST_COLUMN,
+        //    .set_row_style(row as i32, idx)
-            height: 1,
+        //    .map_err(|e| anyhow!("{}", e))?;
-        }
+        //Ok(())
-    }
-
-    pub fn set_col_style(
-        &mut self,
-        style: &[(&str, &str)],
-        sheet: u32,
-        col_idx: usize,
-    ) -> Result<()> {
-        // TODO(jeremy): This is a little hacky and the underlying model
-        // supports a better mechanism but UserModel doesn't support it yet.
-        // https://github.com/ironcalc/IronCalc/issues/273
-        // NOTE(jwall): Because of the number of cells required to modify
-        // this is crazy slow
-        let area = self.get_col_range(sheet, col_idx);
-        self.set_cell_style(style, &area)?;
-        Ok(())
-    }
-
-    pub fn set_row_style(
-        &mut self,
-        style: &[(&str, &str)],
-        sheet: u32,
-        row_idx: usize,
-    ) -> Result<()> {
-        // TODO(jeremy): This is a little hacky and the underlying model
-        // supports a better mechanism but UserModel doesn't support it yet.
-        // https://github.com/ironcalc/IronCalc/issues/273
-        let area = self.get_row_range(sheet, row_idx);
-        self.set_cell_style(style, &area)?;
-        Ok(())
     }
 
     /// Get a cells rendered content for display.
@@ -438,14 +458,8 @@ impl Book {
         self.get_column_size_for_sheet(self.current_sheet, idx)
     }
 
-    pub fn get_column_size_for_sheet(
+    pub fn get_column_size_for_sheet(&self, sheet: u32, idx: usize) -> std::result::Result<usize, anyhow::Error> {
-        &self,
+        Ok((self.model.get_column_width(sheet, idx as i32)
-        sheet: u32,
-        idx: usize,
-    ) -> std::result::Result<usize, anyhow::Error> {
-        Ok((self
-            .model
-            .get_column_width(sheet, idx as i32)
             .map_err(|e| anyhow!("Error getting column width: {:?}", e))?
             / COL_PIXELS) as usize)
     }
@@ -454,14 +468,8 @@ impl Book {
         self.set_column_size_for_sheet(self.current_sheet, col, width)
     }
 
-    pub fn set_column_size_for_sheet(
+    pub fn set_column_size_for_sheet(&mut self, sheet: u32, col: usize, width: usize) -> std::result::Result<(), anyhow::Error> {
-        &mut self,
+        self.model.set_column_width(sheet, col as i32, width as f64 * COL_PIXELS)
-        sheet: u32,
-        col: usize,
-        width: usize,
-    ) -> std::result::Result<(), anyhow::Error> {
-        self.model
-            .set_column_width(sheet, col as i32, width as f64 * COL_PIXELS)
             .map_err(|e| anyhow!("Error setting column width: {:?}", e))?;
         Ok(())
     }
@@ -483,8 +491,7 @@ impl Book {
     /// Select a sheet by name.
     pub fn select_sheet_by_name(&mut self, name: &str) -> bool {
         if let Some((idx, _sheet)) = self
-            .model
+            .model.get_model()
-            .get_model()
             .workbook
             .worksheets
             .iter()
@@ -508,9 +515,7 @@ impl Book {
         if next == len {
             next = 0;
         }
-        self.model
+        self.model.set_selected_sheet(next).expect("Unexpected error selecting sheet");
-            .set_selected_sheet(next)
-            .expect("Unexpected error selecting sheet");
         self.current_sheet = next;
     }
 
@@ -521,26 +526,21 @@ impl Book {
         } else {
             self.current_sheet - 1
         };
-        self.model
+        self.model.set_selected_sheet(next).expect("Unexpected error selecting sheet");
-            .set_selected_sheet(next)
-            .expect("Unexpected error selecting sheet");
         self.current_sheet = next;
     }
 
     /// Select a sheet by id.
     pub fn select_sheet_by_id(&mut self, id: u32) -> bool {
         if let Some((idx, _sheet)) = self
-            .model
+            .model.get_model()
-            .get_model()
             .workbook
             .worksheets
             .iter()
             .enumerate()
             .find(|(_idx, sheet)| sheet.sheet_id == id)
         {
-            self.model
+            self.model.set_selected_sheet(idx as u32).expect("Unexpected error selecting sheet");
-                .set_selected_sheet(idx as u32)
-                .expect("Unexpected error selecting sheet");
             self.current_sheet = idx as u32;
             return true;
         }
@@ -553,8 +553,7 @@ impl Book {
         // Looks like it should be done with:
         // https://docs.rs/ironcalc_base/latest/ironcalc_base/struct.UserModel.html#method.get_worksheets_properties
         Ok(self
-            .model
+            .model.get_model()
-            .get_model()
             .workbook
             .worksheet(self.current_sheet)
             .map_err(|s| anyhow!("Invalid Worksheet id: {}: error: {}", self.current_sheet, s))?)
@@ -565,8 +564,7 @@ impl Book {
         // Looks like it should be done with:
         // https://docs.rs/ironcalc_base/latest/ironcalc_base/struct.UserModel.html#method.get_worksheets_properties
         Ok(&self
-            .model
+            .model.get_model()
-            .get_model()
             .workbook
             .worksheet(idx as u32)
             .map_err(|s| anyhow!("Invalid Worksheet: {}", s))?

src/ui/mod.rs

@@ -1,7 +1,7 @@
 //! Ui rendering logic
 use std::{path::PathBuf, process::ExitCode};
 
-use crate::book::{self, AddressRange, Book};
+use crate::book::{AddressRange, Book};
 
 use anyhow::{anyhow, Result};
 use crossterm::event::{self, Event, KeyCode, KeyEventKind, KeyModifiers};
@@ -235,7 +235,7 @@ impl<'ws> Workspace<'ws> {
     /// Move a row down in the current sheet.
     pub fn move_down(&mut self) -> Result<()> {
         let mut loc = self.book.location.clone();
-        if loc.row < (book::LAST_ROW as usize) {
+        if loc.row < render::viewport::LAST_ROW {
             loc.row += 1;
             self.book.move_to(&loc)?;
         }
@@ -244,10 +244,7 @@ impl<'ws> Workspace<'ws> {
 
     /// Move to the top row without changing columns
     pub fn move_to_top(&mut self) -> Result<()> {
-        self.book.move_to(&Address {
+        self.book.move_to(&Address { row: 1, col: self.book.location.col })?;
-            row: 1,
-            col: self.book.location.col,
-        })?;
         Ok(())
     }
     
@@ -274,7 +271,7 @@ impl<'ws> Workspace<'ws> {
     /// Move a column to the left in the current sheet.
     pub fn move_right(&mut self) -> Result<()> {
         let mut loc = self.book.location.clone();
-        if loc.col < (book::LAST_COLUMN as usize) {
+        if loc.col < render::viewport::LAST_COLUMN {
             loc.col += 1;
             self.book.move_to(&loc)?;
         }
@@ -323,8 +320,7 @@ impl<'ws> Workspace<'ws> {
                 "Edit Mode:".to_string(),
                 "* ENTER/RETURN: Exit edit mode and save changes".to_string(),
                 "* Ctrl-r: Enter Range Selection mode".to_string(),
-                "* v: Enter Range Selection mode with the start of the range already selected"
+                "* v: Enter Range Selection mode with the start of the range already selected".to_string(),
-                    .to_string(),
                 "* ESC: Exit edit mode and discard changes".to_string(),
                 "Otherwise edit as normal".to_string(),
             ],
@@ -452,7 +448,8 @@ impl<'ws> Workspace<'ws> {
                         self.book.set_sheet_name(idx as u32, name)?;
                     }
                     _ => {
-                        self.book.set_sheet_name(self.book.current_sheet, name)?;
+                        self.book
+                            .set_sheet_name(self.book.current_sheet, name)?;
                     }
                 }
                 Ok(None)
@@ -465,28 +462,22 @@ impl<'ws> Workspace<'ws> {
                 self.book.select_sheet_by_name(name);
                 Ok(None)
             }
-            Ok(Some(Cmd::Quit)) => Ok(Some(ExitCode::SUCCESS)),
+            Ok(Some(Cmd::Quit)) => {
-            Ok(Some(Cmd::ColorRows(count, color))) => {
+                Ok(Some(ExitCode::SUCCESS))
-                let row_count = count.unwrap_or(1);
+            }
+            Ok(Some(Cmd::ColorRows(_count, color))) => {
+                let row_count = _count.unwrap_or(1);
                 let row = self.book.location.row;
-                for r in row..(row + row_count) {
+                for r in row..(row+row_count) {
-                    self.book.set_row_style(
+                    self.book.set_row_style(&[("fill.bg_color", &color)], self.book.current_sheet, r)?;
-                        &[("fill.bg_color", &color)],
-                        self.book.current_sheet,
-                        r,
-                    )?;
                 }
                 Ok(None)
             }
-            Ok(Some(Cmd::ColorColumns(count, color))) => {
+            Ok(Some(Cmd::ColorColumns(_count, color))) => {
-                let col_count = count.unwrap_or(1);
+                let col_count = _count.unwrap_or(1);
                 let col = self.book.location.col;
-                for c in col..(col + col_count) {
+                for c in col..(col+col_count) {
-                    self.book.set_col_style(
+                    self.book.set_col_style(&[("fill.bg_color", &color)], self.book.current_sheet, c)?;
-                        &[("fill.bg_color", &color)],
-                        self.book.current_sheet,
-                        c,
-                    )?;
                 }
                 Ok(None)
             }
@@ -498,7 +489,7 @@ impl<'ws> Workspace<'ws> {
                         row: start.row as i32,
                         column: start.col as i32,
                         width: (end.col - start.col + 1) as i32,
-                        height: (end.row - start.row + 1) as i32,
+                        height: (end.row - start.row + 1) as i32
                     }
                 } else {
                     let address = self.book.location.clone();
@@ -507,11 +498,10 @@ impl<'ws> Workspace<'ws> {
                         row: address.row as i32,
                         column: address.col as i32,
                         width: 1,
-                        height: 1,
+                        height: 1
                     }
                 };
-                self.book
+                self.book.set_cell_style(&[("fill.bg_color", &color)], &area)?;
-                    .set_cell_style(&[("fill.bg_color", &color)], &area)?;
                 Ok(None)
             }
             Ok(None) => {
@@ -621,7 +611,11 @@ impl<'ws> Workspace<'ws> {
                     })?;
                     self.state.range_select.sheet = Some(self.book.current_sheet);
                 }
-                KeyCode::Char('C') if key.modifiers.contains(KeyModifiers::CONTROL) => {
+                KeyCode::Char('C')
+                    if key
+                        .modifiers
+                        .contains(KeyModifiers::CONTROL) =>
+                {
                     self.copy_range(true)?;
                     self.exit_range_select_mode()?;
                 }
@@ -638,10 +632,7 @@ impl<'ws> Workspace<'ws> {
                     self.exit_range_select_mode()?;
                 }
                 KeyCode::Char('x') => {
-                    if let (Some(from), Some(to)) = (
+                    if let (Some(from), Some(to)) = (self.state.range_select.start.as_ref(), self.state.range_select.end.as_ref()) {
-                        self.state.range_select.start.as_ref(),
-                        self.state.range_select.end.as_ref(),
-                    ) {
                         self.book.extend_to(from, to)?;
                     }
                     self.exit_range_select_mode()?;
@@ -660,15 +651,20 @@ impl<'ws> Workspace<'ws> {
     fn copy_range(&mut self, formatted: bool) -> Result<(), anyhow::Error> {
         self.update_range_selection()?;
         match &self.state.range_select.get_range() {
-            Some((start, end)) => {
+            Some((
+                start,
+                end,
+            )) => {
                 let mut rows = Vec::new();
-                for row in (AddressRange { start, end }).as_rows() {
+                for row in (AddressRange { start, end, }).as_rows() {
                     let mut cols = Vec::new();
                     for cell in row {
                         cols.push(if formatted {
-                            self.book.get_cell_addr_rendered(&cell)?
+                            self.book
+                                .get_cell_addr_rendered(&cell)?
                         } else {
-                            self.book.get_cell_addr_contents(&cell)?
+                            self.book
+                                .get_cell_addr_contents(&cell)?
                         });
                     }
                     rows.push(cols);
@@ -677,9 +673,11 @@ impl<'ws> Workspace<'ws> {
             }
             None => {
                 self.state.clipboard = Some(ClipboardContents::Cell(if formatted {
-                    self.book.get_current_cell_rendered()?
+                    self.book
+                        .get_current_cell_rendered()?
                 } else {
-                    self.book.get_current_cell_contents()?
+                    self.book
+                        .get_current_cell_contents()?
                 }));
             }
         }
@@ -745,7 +743,11 @@ impl<'ws> Workspace<'ws> {
                         self.book.get_current_cell_rendered()?,
                     ));
                 }
-                KeyCode::Char('C') if key.modifiers.contains(KeyModifiers::CONTROL) => {
+                KeyCode::Char('C')
+                    if key
+                        .modifiers
+                        .contains(KeyModifiers::CONTROL) =>
+                {
                     self.state.clipboard = Some(ClipboardContents::Cell(
                         self.book.get_current_cell_rendered()?,
                     ));
@@ -859,13 +861,7 @@ impl<'ws> Workspace<'ws> {
                 }
                 KeyCode::Char('g') => {
                     // TODO(zaphar): This really needs a better state machine.
-                    if self
+                    if self.state.char_queue.first().map(|c| *c == 'g').unwrap_or(false) {
-                        .state
-                        .char_queue
-                        .first()
-                        .map(|c| *c == 'g')
-                        .unwrap_or(false)
-                    {
                         self.state.char_queue.pop();
                         self.move_to_top()?;
                     } else {

src/ui/render/test.rs

@@ -14,8 +14,8 @@ fn test_viewport_get_visible_columns() {
     let default_size = book.get_col_size(1).expect("Failed to get column size");
     let width = dbg!(dbg!(default_size) * 12 / 2);
     let app_state = AppState::default();
-    let viewport = Viewport::new(&book, Some(&app_state.range_select))
+    let viewport =
-        .with_selected(Address { row: 1, col: 17 });
+        Viewport::new(&book, Some(&app_state.range_select)).with_selected(Address { row: 1, col: 17 });
     let cols = viewport
         .get_visible_columns((width + 5) as u16, &mut state)
         .expect("Failed to get visible columns");
@@ -31,8 +31,8 @@ fn test_viewport_get_visible_rows() {
     );
     let height = 6;
     let app_state = AppState::default();
-    let viewport = Viewport::new(&book, Some(&app_state.range_select))
+    let viewport =
-        .with_selected(Address { row: 17, col: 1 });
+        Viewport::new(&book, Some(&app_state.range_select)).with_selected(Address { row: 17, col: 1 });
     let rows = dbg!(viewport.get_visible_rows(height as u16, &mut state));
     assert_eq!(height - 1, rows.len());
     assert_eq!(
@@ -65,8 +65,8 @@ fn test_viewport_visible_columns_after_length_change() {
         .expect("Failed to set column size");
     {
         let app_state = AppState::default();
-        let viewport = Viewport::new(&book, Some(&app_state.range_select))
+        let viewport =
-            .with_selected(Address { row: 1, col: 1 });
+            Viewport::new(&book, Some(&app_state.range_select)).with_selected(Address { row: 1, col: 1 });
         let cols = viewport
             .get_visible_columns((width + 5) as u16, &mut state)
             .expect("Failed to get visible columns");
@@ -97,9 +97,7 @@ fn test_color_mapping() {
         ("darkgrey", Color::DarkGray),
         ("darkgray", Color::DarkGray),
         ("#35f15b", Color::Rgb(53, 241, 91)),
-    ]
+    ].map(|(s, c)| (Some(s.to_string()), c)) {
-    .map(|(s, c)| (Some(s.to_string()), c))
-    {
         assert_eq!(super::viewport::map_color(s.as_ref(), Color::Gray), c);
     }
 }

src/ui/render/viewport.rs

@@ -7,9 +7,14 @@ use ratatui::{
     widgets::{Block, Cell, Row, StatefulWidget, Table, Widget},
 };
 
-use crate::book;
 use super::{Address, Book, RangeSelection};
 
+// TODO(zaphar): Move this to the book module.
+// NOTE(zaphar): This is stolen from ironcalc but ironcalc doesn't expose it
+// publically.
+pub(crate) const LAST_COLUMN: usize = 16_384;
+pub(crate) const LAST_ROW: usize = 1_048_576;
+
 /// A visible column to show in our Viewport.
 #[derive(Clone, Debug)]
 pub struct VisibleColumn {
@@ -63,7 +68,7 @@ impl<'ws> Viewport<'ws> {
         let start_row = std::cmp::min(self.selected.row, state.prev_corner.row);
         let mut start = start_row;
         let mut end = start_row;
-        for row_idx in start_row..=(book::LAST_ROW as usize) {
+        for row_idx in start_row..=LAST_ROW {
             let updated_length = length + 1;
             if updated_length <= height {
                 length = updated_length;
@@ -90,7 +95,7 @@ impl<'ws> Viewport<'ws> {
         // We start out with a length of 5 already reserved
         let mut length = 5;
         let start_idx = std::cmp::min(self.selected.col, state.prev_corner.col);
-        for idx in start_idx..=(book::LAST_COLUMN as usize) {
+        for idx in start_idx..=LAST_COLUMN {
             let size = self.book.get_col_size(idx)? as u16;
             let updated_length = length + size;
             let col = VisibleColumn { idx, length: size };

src/ui/test.rs

@@ -2,7 +2,6 @@ use std::process::ExitCode;
 
 use crossterm::event::{Event, KeyCode, KeyEvent, KeyModifiers};
 
-use crate::book;
 use crate::ui::cmd::parse_color;
 use crate::ui::{Address, Modality};
 
@@ -305,6 +304,7 @@ fn test_cmd_color_columns_with_idx_and_color() {
     assert_eq!(cmd, Cmd::ColorColumns(Some(1), parse_color("red").unwrap()));
 }
 
+
 #[test]
 fn test_input_navitation_enter_key() {
     let mut ws = new_workspace();
@@ -1004,7 +1004,8 @@ macro_rules! assert_range_clear {
             .run(&mut ws)
             .expect("Failed to handle script");
         assert_eq!(Some(&Modality::RangeSelect), ws.state.modality_stack.last());
-        $script.run(&mut ws).expect("Failed to handle script");
+        $script.run(&mut ws)
+            .expect("Failed to handle script");
         assert_eq!(
             "".to_string(),
             ws.book
@@ -1022,21 +1023,18 @@ macro_rules! assert_range_clear {
 
 #[test]
 fn test_range_select_clear_upper_d() {
-    assert_range_clear!(script().char('j').char('l').char('D'));
+    assert_range_clear!(script()
+        .char('j')
+        .char('l')
+        .char('D'));
 }
 
 #[test]
 fn test_range_select_movement() {
     let mut ws = new_workspace();
-    ws.book
+    ws.book.new_sheet(Some("s2")).expect("Unable create s2 sheet");
-        .new_sheet(Some("s2"))
+    ws.book.new_sheet(Some("s3")).expect("Unable create s3 sheet");
-        .expect("Unable create s2 sheet");
+    script().ctrl('r').run(&mut ws)
-    ws.book
-        .new_sheet(Some("s3"))
-        .expect("Unable create s3 sheet");
-    script()
-        .ctrl('r')
-        .run(&mut ws)
         .expect("failed to run script");
     assert_eq!(Some(&Modality::RangeSelect), ws.state.modality_stack.last());
     script()
@@ -1066,7 +1064,10 @@ fn test_range_select_movement() {
 
 #[test]
 fn test_range_select_clear_lower_d() {
-    assert_range_clear!(script().char('j').char('l').char('d'));
+    assert_range_clear!(script()
+        .char('j')
+        .char('l')
+        .char('d'));
 }
 
 macro_rules! assert_range_copy {
@@ -1074,12 +1075,8 @@ macro_rules! assert_range_copy {
         let mut ws = new_workspace();
         let top_left_addr = Address { row: 2, col: 2 };
         let bot_right_addr = Address { row: 4, col: 4 };
-        ws.book
+        ws.book.update_cell(&top_left_addr, "top_left").expect("Failed to update top left");
-            .update_cell(&top_left_addr, "top_left")
+        ws.book.update_cell(&bot_right_addr, "bot_right").expect("Failed to update top left");
-            .expect("Failed to update top left");
-        ws.book
-            .update_cell(&bot_right_addr, "bot_right")
-            .expect("Failed to update top left");
         assert!(ws.state.clipboard.is_none());
         script()
             .ctrl('r')
@@ -1088,14 +1085,7 @@ macro_rules! assert_range_copy {
             .char(' ')
             .run(&mut ws)
             .expect("failed to run script");
-        assert_eq!(
+        assert_eq!(&top_left_addr, ws.state.range_select.start.as_ref().expect("Didn't find a start of range"));
-            &top_left_addr,
-            ws.state
-                .range_select
-                .start
-                .as_ref()
-                .expect("Didn't find a start of range")
-        );
         script()
             .char('2')
             .char('j')
@@ -1103,53 +1093,27 @@ macro_rules! assert_range_copy {
             .char('l')
             .run(&mut ws)
             .expect("failed to run script");
-        assert_eq!(
+        assert_eq!(&bot_right_addr, ws.state.range_select.end.as_ref().expect("Didn't find a start of range"));
-            &bot_right_addr,
+        assert_eq!(&Address { row: 1, col: 1}, ws.state.range_select.original_location
-            ws.state
+            .as_ref().expect("Expected an original location"));
-                .range_select
+        assert_eq!(0, ws.state.range_select.original_sheet.
-                .end
+            expect("Expected an original sheet"));
-                .as_ref()
+        assert_eq!(Some(&Modality::RangeSelect), ws.state.modality_stack.iter().last());
-                .expect("Didn't find a start of range")
-        );
-        assert_eq!(
-            &Address { row: 1, col: 1 },
-            ws.state
-                .range_select
-                .original_location
-                .as_ref()
-                .expect("Expected an original location")
-        );
-        assert_eq!(
-            0,
-            ws.state
-                .range_select
-                .original_sheet
-                .expect("Expected an original sheet")
-        );
-        assert_eq!(
-            Some(&Modality::RangeSelect),
-            ws.state.modality_stack.iter().last()
-        );
         dbg!(ws.state.range_select.get_range());
-        $script.run(&mut ws).expect("failed to run script");
+        $script.run(&mut ws)
+            .expect("failed to run script");
         assert!(ws.state.clipboard.is_some());
         match ws.state.clipboard.unwrap() {
             crate::ui::ClipboardContents::Cell(_) => assert!(false, "Not rows in Clipboard"),
             crate::ui::ClipboardContents::Range(rows) => {
-                assert_eq!(
+                assert_eq!(vec![
-                    vec![
                     vec!["top_left".to_string(), "".to_string(), "".to_string()],
                     vec!["".to_string(), "".to_string(), "".to_string()],
                     vec!["".to_string(), "".to_string(), "bot_right".to_string()],
-                    ],
+                ], rows);
-                    rows
+            },
-                );
         }
-        }
+        assert_eq!(Some(&Modality::Navigate), ws.state.modality_stack.iter().last());
-        assert_eq!(
-            Some(&Modality::Navigate),
-            ws.state.modality_stack.iter().last()
-        );
     }};
 }
 
@@ -1176,9 +1140,7 @@ fn test_range_select_copy_capital_c() {
 #[test]
 fn test_extend_to_range() {
     let mut ws = new_workspace();
-    ws.book
+    ws.book.edit_current_cell("=B1+1").expect("Failed to edit cell");
-        .edit_current_cell("=B1+1")
-        .expect("Failed to edit cell");
     ws.book.evaluate();
     script()
         .char('v')
@@ -1186,9 +1148,7 @@ fn test_extend_to_range() {
         .char('x')
         .run(&mut ws)
         .expect("Unable to run script");
-    let extended_cell = ws
+    let extended_cell = ws.book.get_cell_addr_contents(&Address { row: 2, col: 1 })
-        .book
-        .get_cell_addr_contents(&Address { row: 2, col: 1 })
         .expect("Failed to get cell contents");
     assert_eq!("=B2+1".to_string(), extended_cell);
 }
@@ -1206,73 +1166,12 @@ fn test_color_cells() {
         .expect("Unable to run script");
     for ri in 1..=3 {
         for ci in 1..=3 {
-            let style = ws
+            let style = ws.book.get_cell_style(ws.book.current_sheet, &Address { row: ri, col: ci }).expect("failed to get style");
-                .book
+            assert_eq!("#800000", style.fill.bg_color.expect(&format!("No background color set for {}:{}", ri, ci)).as_str());
-                .get_cell_style(ws.book.current_sheet, &Address { row: ri, col: ci })
-                .expect("failed to get style");
-            assert_eq!(
-                "#800000",
-                style
-                    .fill
-                    .bg_color
-                    .expect(&format!("No background color set for {}:{}", ri, ci))
-                    .as_str()
-            );
         }
     }
 }
 
-#[test]
-fn test_color_row() {
-    let mut ws = new_workspace();
-    script()
-        .char(':')
-        .chars("color-rows red")
-        .enter()
-        .run(&mut ws)
-        .expect("Unable to run script");
-    for ci in [1, book::LAST_COLUMN] {
-        let style = ws
-            .book
-            .get_cell_style(ws.book.current_sheet, &Address { row: 1, col: ci as usize })
-            .expect("failed to get style");
-        assert_eq!(
-            "#800000",
-            style
-                .fill
-                .bg_color
-                .expect(&format!("No background color set for {}:{}", 1, ci))
-                .as_str()
-        );
-    }
-}
-
-#[test]
-fn test_color_col() {
-    let mut ws = new_workspace();
-    script()
-        .char(':')
-        .chars("color-columns red")
-        .enter()
-        .run(&mut ws)
-        .expect("Unable to run script");
-    for ri in [1, book::LAST_ROW] {
-        let style = ws
-            .book
-            .get_cell_style(ws.book.current_sheet, &Address { row: ri as usize, col: 1 })
-            .expect("failed to get style");
-        assert_eq!(
-            "#800000",
-            style
-                .fill
-                .bg_color
-                .expect(&format!("No background color set for {}:{}", ri, 1))
-                .as_str()
-        );
-    }
-}
-
-
 fn new_workspace<'a>() -> Workspace<'a> {
     Workspace::new_empty("en", "America/New_York").expect("Failed to get empty workbook")
 }