Skip to content

feat(parser): expose supplemental picture data - #44

Open
emsi wants to merge 2 commits into
msiemens:masterfrom
emsi:feature/image-fallback-icons
Open

feat(parser): expose supplemental picture data#44
emsi wants to merge 2 commits into
msiemens:masterfrom
emsi:feature/image-fallback-icons

Conversation

@emsi

@emsi emsi commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Closes #43.

Summary

  • Add a public Picture value that exposes lazily backed picture data, size, availability, and stored extension.
  • Parse and expose WebPictureContainer14 through Image::web_picture().
  • Parse and expose an embedded file's PictureContainer through EmbeddedFile::icon().
  • Keep failures in either optional picture non-fatal: the parser records a warning and preserves the primary image or embedded file.

Rationale

OneNote objects can carry supplemental picture representations independently of their primary payload. Keeping these references in the parsed model lets renderers use the alternate image representation when appropriate and display the icon stored for an embedded file. The new Picture type follows the parser's existing lazy FileBlob behavior, so parsing does not copy or eagerly materialize binary data.

The existing Image and EmbeddedFile payload accessors are unchanged. The additions are optional accessors, so consumers can adopt them without changing their current primary-data handling.

Validation

  • cargo test
  • cargo clippy
  • cargo fmt --all -- --check
  • git diff --check upstream/master...HEAD

The updated snapshots use the repository's existing public fixtures and verify that stored embedded-file icons are retained.

@emsi
emsi marked this pull request as ready for review August 8, 2026 13:47
@msiemens

Copy link
Copy Markdown
Owner

Thanks for implementing this, @emsi! I noticed you put the Debug trait impl behind a custom impl block that only adds the new fields when they are not None. I'd assume this would be to keep the insta snapshot churn down, is that correct? That would also imply that for new fields, we need to remember to manually add them to the Debug impl for insta tests to pick them up. If that's the case, I'd prefer having the insta snapshot churn over the possibility of having new fields not being picked up by accident.

@emsi

emsi commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for implementing this, @emsi! I noticed you put the Debug trait impl behind a custom impl block that only adds the new fields when they are not None. I'd assume this would be to keep the insta snapshot churn down, is that correct? That would also imply that for new fields, we need to remember to manually add them to the Debug impl for insta tests to pick them up. If that's the case, I'd prefer having the insta snapshot churn over the possibility of having new fields not being picked up by accident.

Yes, that was the motivation: I wanted to avoid snapshot churn from optional fields being None. On reconsideration, I agree that the manual implementations create a maintenance risk and weaken the snapshots because future fields could be silently omitted. I’ll restore derived Debug implementations for Image and EmbeddedFile and update the snapshots to include the optional fields even when absent. Thanks for catching this.

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.

Expose supplemental pictures for images and embedded files

2 participants