Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Release 0.18.0-rc #577

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

fintelia
Copy link
Contributor

@fintelia fintelia commented Feb 28, 2025

One open question is whether we should take this chance to remove Decoder::{next_row, next_interlaced_row} in favor of Decoder::read_row (added in #493)

@anforowicz
Copy link
Contributor

My thoughts on removing Decoder::{next_row, next_interlaced_row}:

  • I think this shouldn't block the 0.18 release
  • If we want to remove those APIs, then I think the first step would be to mark them as #[deprecated]. I think it doesn't matter that much if we do it in 0.18.0 vs 0.18.1 or later. And then we can remove them in 0.19.
  • I don't have a strong opinion about desirability of removing those APIs:
    • The old APIs are sometimes convenient (I guess mostly because buffer management on Rust side is easier than buffer management on C++ side). Current Chromium implementation uses those old APIs (see here; I now realize that the TODO/bug link should be updated), although using the new API would force an improvement for some scenarios (avoiding an extra memcpy here)
    • On the other hand, the old APIs are kind of redundant (i.e. removing them will result in smaller, simpler-to-understand API)

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