Implement styles by ddimaria · Pull Request #538 · tafia/calamine

ddimaria · 2025-07-24T21:32:14Z

Add style support for Calamine.

Basic api for getting styles

fn test_worksheet_style_iter() {
    let mut xlsx: Xlsx<_> = wb("styles.xlsx");
    let styles = xlsx.worksheet_style("Sheet 1").unwrap();

    for (row, styles) in styles.rows().enumerate() {
        for (col, style) in styles.iter().enumerate() {
            println!("row: {}, col: {}, style: {:?}", row, col, style);
        }
    }
}

Here is the relevant structs so far:

struct Style {
    font: Option<Font>,
    fill: Option<Fill>,
    borders: Option<Borders>,
    alignment: Option<Alignment>,
    number_format: Option<NumberFormat>,
    protection: Option<Protection>,
    style_id: Option<u32>,
}

struct Color {
    alpha: u8,
    red: u8,
    green: u8,
    blue: u8,
}

struct Borders {
    left: Border,
    right: Border,
    top: Border,
    bottom: Border,
    diagonal_down: Border,
    diagonal_up: Border,
}

struct Font {
    name: Option<String>,
    size: Option<f64>,
    weight: FontWeight,
    style: FontStyle,
    underline: UnderlineStyle,
    strikethrough: bool,
    color: Option<Color>,
    family: Option<String>,
}

struct Alignment {
    horizontal: HorizontalAlignment,
    vertical: VerticalAlignment,
    text_rotation: TextRotation,
    wrap_text: bool,
    indent: Option<u8>,
    shrink_to_fit: bool,
}

struct Fill {
    pattern: FillPattern,
    foreground_color: Option<Color>,
    background_color: Option<Color>,
}

struct NumberFormat {
    format_code: String,
    format_id: Option<u32>,
}

struct Protection {
    locked: bool,
    hidden: bool,
}

jmcnamara · 2025-07-26T10:51:49Z

Just a quick note to say that overall this looks good and I think it is going in the right direction.

I'll give more detailed feedback early next week.

For code comments you are already using correct sentence-case but could you also add a full stop/period at the end of each line. This makes it easier to change comments into documentation. It isn't a rule that has been adhered to in the existing code but I would like to improve that going forward.

azarmadr · 2025-07-29T13:07:55Z

Man, i wish i discovered this pull early, i wanted to know if a cell is stricken or not. Sat whole day and edited the code myself. Came till adding if stricken or not from the styles.

Learnt a lot from my experience though.
i'll test this pull against my code.

azarmadr · 2025-07-30T16:44:22Z

Is the data and styles coming together?
If not, then would we have to read the excel twice to get data, once and then style?

ddimaria · 2025-07-30T16:53:24Z

Is the data and styles coming together? If not, then would we have to read the excel twice to get data, once and then style?

Data and Formats have always been processed separately (e.g. worksheet_range() and worksheet_formula()). This PR follows the format pattern (worksheet_style() for styles and worksheet_layout() for column widths and row heights).

When I finish the base work, I'm going to look into optimizations:

More optimal storage (Vec vs Run-Length Encoding) for styles
Looping (to your point, combining read operations to reduce memory/cpu usage)

azarmadr · 2025-07-31T05:09:02Z

i got it, even formulae were being used similarly. Was able to satisfy my requirement with worksheet_sytle over a range.

Another doubt, why every item in the style is an option?
All the fonts, borders, and others are available as ids in cellxfs?

hope i am not bothering you.😊

ddimaria · 2025-08-13T17:39:31Z

Another doubt, why every item in the style is an option?

@azarmadr this is because the particular style may not exist for a given cell.

ddimaria · 2025-08-13T17:41:31Z

@jmcnamara We're currently using this fork in production and it's working well. What do I need to do to get this merged with Calamine? Some things to discuss:

Is the API agreeable?
What type of documentation is needed?
Add more testing?
Should this be a compiler feature flag?
This is only implemented for xlsx, is that OK?

jmcnamara · 2025-08-14T16:14:52Z

What do I need to do to get this merged with Calamine?

You should create a non-draft PR with all the WIP files and code removed. I will try to do a review on the current code.

Is the API agreeable?

Overall I think it is okay but I may have specific comments during review.

What type of documentation is needed?

Copy the style of the Xlsx documentation:

One line description.
One-two paragraph explanation.
Parameters section.
Errors section if the method Result<(), XlsxError>.
Examples section.

https://docs.rs/calamine/latest/calamine/struct.Xlsx.html#method.merged_regions_by_sheet

Add more testing?

That is always good. :-)

Should this be a compiler feature flag?

I think not.

This is only implemented for xlsx, is that OK?

Probably not but lets try get it right for Xlsx first.

jmcnamara · 2025-08-14T16:16:19Z

@@ -0,0 +1,253 @@
+# Style Support in Calamine


Remove this files from the final PR or convert it to documentation.

jmcnamara · 2025-08-14T17:16:35Z

+    /// The cell value
+    pub value: Data,
+    /// The cell style
+    pub style: Option<Style>,


Strictly speaking the style is never optional in the Excel file formats. There is always an implicit 0 style that is the default and is defined in the theme.xml file. Note, I'm not necessarily saying that it shouldn't be an Option<>.

jmcnamara · 2025-08-14T17:18:37Z

+        self.style = None;
+    }
+
+    /// Check if the cell has style information


Again, an Excel cell will always have style information even if it is implicit for size reasons.

jmcnamara · 2025-08-14T17:21:59Z

+                    let mut style = None;
+
+                    // Extract style ID if present
+                    if let Ok(Some(style_id_str)) =


Again, the style id is 0 and not None if not present.

This is done, please review.

jmcnamara · 2025-08-14T18:16:59Z

+    }
+
+    /// Get effective column width (custom or default)
+    pub fn get_effective_column_width(&self, column: u32) -> f64 {


This effective width is probably not the width that the user would expect and also may be dependent on the default font for the file.

See the following for the calculation in the other direction: https://github.com/jmcnamara/rust_xlsxwriter/blob/main/src/worksheet.rs#L19340-L19368

However, there may not be any simple way to return the correct value in all cases.

jmcnamara · 2025-08-14T18:17:31Z

+    }
+
+    /// Get effective row height (custom or default)
+    pub fn get_effective_row_height(&self, row: u32) -> f64 {


Same comment for row height although the height calculation is simpler.

jmcnamara · 2025-08-14T18:29:38Z

 use crate::style::*;
 use crate::XlsxError;

+/// Get default Excel 2007+ theme color scheme (Office theme)


Avoid using /// for non-public non-doc comments. Also this is an example of where proper sentence punctuation should be applied.

jmcnamara · 2025-08-14T18:36:47Z

First review done. Some general comments:

Please squash and or rebase WIP code and reverts into a small number of logical commits or one commit.
Fix the clippy and or format warnings the are breaking the CI.
Add a full stop/period at the end of all comments.
Avoid /// comments for non-public non-doc comments. Use // instead.
Post an explanation here for the setting functions in CellData and Style.

jmcnamara · 2025-09-04T09:48:55Z

@ddimaria Any update on this?

ddimaria · 2026-01-27T20:31:08Z

Finally back on this 😄

ddimaria · 2026-01-27T22:31:37Z

I've merged main into this branch, resolving conflicts and fixing tests. I also have a 1M cell style file being used in new benchmark tests (used criterion). I'm moving on to flamegraph and will start improvements based on the results.

ddimaria · 2026-01-28T00:26:14Z

I just implemented RLE and other optimizations. I'll close this PR and open a new one tomorrow, and will squash all of my commits. The perf is a lot better now, and there is less of a memory footprint.

ddimaria · 2026-01-30T21:52:40Z

I added support for Rich Text, which handles various formats within a single cell. This is how excel stores it:

<si>
  <r>
    <rPr>
      <b/>  <!-- bold -->
      <sz val="11"/>
      <color rgb="FF0000FF"/>
      <rFont val="Calibri"/>
    </rPr>
    <t>Bold blue text</t>
  </r>
  <r>
    <rPr>
      <i/>  <!-- italic -->
      <sz val="11"/>
      <color rgb="FFFF0000"/>
    </rPr>
    <t> italic red text</t>
  </r>
</si>

Resolves merge conflicts in Cargo.toml (rstest version) and tests/test.rs (pivot table tests vs style tests — keep both). This merge adds: - Style struct with Font/Fill/Borders/Alignment/NumberFormat/Protection - styles.xml parser (src/xlsx/style_parser.rs) - worksheet_style() API returning row×col style grid - RLE-compressed StyleRange for memory efficiency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ddimaria · 2026-02-24T16:39:35Z

I noticed a bug in colors where tints weren't being handled. This is now implemented.

ddimaria · 2026-03-02T14:23:14Z

@jmcnamara I need to implement conditional formatting and validations, is it OK to add them to this PR?

jmcnamara · 2026-03-02T14:57:02Z

@ddimaria This PR is already very large. It would be best to get this stabilized first (is it suitable for review now?).

You can create other PRs that are dependent on this on. Conditional formatting and Data Validation should be separate PRs.

ddimaria · 2026-03-02T15:20:04Z

@ddimaria This PR is already very large. It would be best to get this stabilized first (is it suitable for review now?).

You can create other PRs that are dependent on this on. Conditional formatting and Data Validation should be separate PRs.

That works for me. I just rebased my fork's commits onto upstream/master and squashed into a single commit.
I'll do a final read through the code this morning and mark this ready for review.

Erik1000 · 2026-03-17T11:51:03Z

@ddimaria This PR is already very large. It would be best to get this stabilized first (is it suitable for review now?).
You can create other PRs that are dependent on this on. Conditional formatting and Data Validation should be separate PRs.

That works for me. I just rebased my fork's commits onto upstream/master and squashed into a single commit. I'll do a final read through the code this morning and mark this ready for review.

is this PR now ready for review and can be merge? 👀

ddimaria · 2026-03-17T13:01:11Z

@ddimaria This PR is already very large. It would be best to get this stabilized first (is it suitable for review now?).
You can create other PRs that are dependent on this on. Conditional formatting and Data Validation should be separate PRs.

That works for me. I just rebased my fork's commits onto upstream/master and squashed into a single commit. I'll do a final read through the code this morning and mark this ready for review.

is this PR now ready for review and can be merge? 👀

@Erik1000 I believe it is, though some conflicts just popped up. Let me review those and CI and it will be ready to review/merge.

ddimaria · 2026-03-19T16:59:15Z

@jmcnamara this PR is ready for review. I'm staring conditional formatting in another branch.

jmcnamara · 2026-03-19T17:39:16Z

this PR is ready for review. I'm staring conditional formatting in another branch.

Ok. Fix the cargo fmt issues and if present, any clippy warnings. I think it would be best to resubmit this as a new PR. There are a lot of out of date comments on this PR.

Erik1000 · 2026-05-05T14:14:37Z

@ddimaria will you submit a new PR or is there already an existing one?

ddimaria · 2026-05-11T13:54:03Z

I'm back on this today. Will resolve conflicts and start a new PR

ddimaria · 2026-05-12T18:15:19Z

PR moved to #653, it OK to close this one.

ddimaria mentioned this pull request Jul 24, 2025

Extract more information at the cell level #513

Closed