Skip to content

Fix doc_id type in begin_tile#182

Open
messense wants to merge 1 commit intomainfrom
fix-doc-id
Open

Fix doc_id type in begin_tile#182
messense wants to merge 1 commit intomainfrom
fix-doc-id

Conversation

@messense
Copy link
Owner

No description provided.

Copilot AI review requested due to automatic review settings December 22, 2025 13:30
@messense messense mentioned this pull request Dec 22, 2025
@messense messense requested a review from ginnyTheCat December 22, 2025 13:31
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes the type of the doc_id parameter in the begin_tile method from Option<NonZero<i32>> to i32 to match the FFI layer's expectations.

  • Changed the doc_id parameter type from Option<NonZero<i32>> to i32 in all begin_tile method signatures
  • Updated call sites to pass doc_id as a plain i32 instead of wrapping it with NonZero::new() or using .map_or()

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/device/native.rs Updated begin_tile method signature in the NativeDevice trait and its implementations for Box<T> and Rc<RefCell<T>>, and updated the FFI bridge to pass doc_id as plain i32
src/device.rs Updated begin_tile method signature and removed the map_or() call when passing doc_id to the FFI function

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ginnyTheCat
Copy link
Collaborator

ginnyTheCat commented Dec 22, 2025

Hm, I'm not quite sure what the intention is. I think they would want doc_ids to start at 1 since the JS docs mention that doc_ids of zero are used to indicate no caching (like it is for id). If that's the case than using NonZero would be correct already. But currently it seems to me like the first document would not be cached.

Maybe it's worth opening an issue against mupdf to ask about this oddity?

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.

2 participants