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

Prevent users from creating Sdl2TtfContext out of nothing and use SDL's reference counting init/quit #1437

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ when upgrading from a version of rust-sdl2 to another.

### Next

[PR #1437](https://github.com/Rust-SDL2/rust-sdl2/pull/1437) **BREAKING CHANGE** Prevent users from creating `Sdl2TtfContext` out of nothing and use SDL's reference counting init/quit

[PR #1416](https://github.com/Rust-SDL2/rust-sdl2/pull/1416) Apply clippy fixes, fix deprecations and other code quality improvements.

[PR #1408](https://github.com/Rust-SDL2/rust-sdl2/pull/1408) Allow comparing `Version`s, add constant with the version the bindings were compiled with.
Expand Down
65 changes: 22 additions & 43 deletions src/sdl2/ttf/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ use get_error;
use rwops::RWops;
use std::error;
use std::fmt;
use std::io;
use std::os::raw::{c_int, c_long};
use std::path::Path;
use sys::ttf;
Expand All @@ -14,7 +13,14 @@ use super::font::{

/// A context manager for `SDL2_TTF` to manage C code initialization and clean-up.
#[must_use]
pub struct Sdl2TtfContext;
pub struct Sdl2TtfContext(());

impl Clone for Sdl2TtfContext {
fn clone(&self) -> Self {
// This should not return an error because SDL_ttf is already initialized
init().unwrap()
}
}

// Clean up the context once it goes out of scope
impl Drop for Sdl2TtfContext {
Expand Down Expand Up @@ -54,7 +60,7 @@ impl Sdl2TtfContext {
point_size: u16,
) -> Result<Font<'ttf, 'r>, String> {
let raw = unsafe { ttf::TTF_OpenFontRW(rwops.raw(), 0, point_size as c_int) };
if (raw as *mut ()).is_null() {
if raw.is_null() {
Err(get_error())
} else {
Ok(internal_load_font_from_ll(raw, Some(rwops)))
Expand All @@ -72,7 +78,7 @@ impl Sdl2TtfContext {
let raw = unsafe {
ttf::TTF_OpenFontIndexRW(rwops.raw(), 0, point_size as c_int, index as c_long)
};
if (raw as *mut ()).is_null() {
if raw.is_null() {
Err(get_error())
} else {
Ok(internal_load_font_from_ll(raw, Some(rwops)))
Expand All @@ -82,52 +88,25 @@ impl Sdl2TtfContext {

/// Returns the version of the dynamically linked `SDL_TTF` library
pub fn get_linked_version() -> Version {
unsafe { Version::from_ll(*ttf::TTF_Linked_Version()) }
}

/// An error for when `sdl2_ttf` is attempted initialized twice
/// Necessary for context management, unless we find a way to have a singleton
#[derive(Debug)]
pub enum InitError {
InitializationError(io::Error),
AlreadyInitializedError,
}

impl error::Error for InitError {
fn source(&self) -> Option<&(dyn error::Error + 'static)> {
match *self {
InitError::AlreadyInitializedError => None,
InitError::InitializationError(ref error) => Some(error),
}
}
}

impl fmt::Display for InitError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Self::AlreadyInitializedError => {
write!(f, "SDL2_TTF has already been initialized")
}
Self::InitializationError(error) => write!(f, "SDL2_TTF initialization error: {error}"),
}
}
Version::from_ll(unsafe { *ttf::TTF_Linked_Version() })
}

/// Initializes the truetype font API and returns a context manager which will
/// clean up the library once it goes out of scope.
pub fn init() -> Result<Sdl2TtfContext, InitError> {
unsafe {
if ttf::TTF_WasInit() == 1 {
Err(InitError::AlreadyInitializedError)
} else if ttf::TTF_Init() == 0 {
Ok(Sdl2TtfContext)
} else {
Err(InitError::InitializationError(io::Error::last_os_error()))
}
#[doc(alias = "TTF_Init")]
pub fn init() -> Result<Sdl2TtfContext, String> {
Copy link
Contributor Author

@antonilol antonilol Oct 8, 2024

Choose a reason for hiding this comment

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

I changed the error type to String here (like in other functions in this library that only return an error from sdl directly), but this specifically will break in an annoying way because now the error does not implement std::error::Error anymore (relevant: #942, #1053, #1424).

I feel like introducing a struct that wraps String (like pub struct SDLError(String);) and implements std::error::Error and Display. This will (in the future) also allow other enums in this crate that can contain an SDL error string to return it from std::error::Error::source.

if unsafe { ttf::TTF_Init() } == 0 {
Ok(Sdl2TtfContext(()))
} else {
Err(get_error())
}
}

/// Returns whether library has been initialized already.
pub fn has_been_initialized() -> bool {
unsafe { ttf::TTF_WasInit() == 1 }
amount_of_times_initialized() != 0
}

fn amount_of_times_initialized() -> c_int {
unsafe { ttf::TTF_WasInit() }
}
11 changes: 7 additions & 4 deletions src/sdl2/ttf/font.rs
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ impl<'ttf, 'r> Drop for Font<'ttf, 'r> {
}

/// Internally used to load a font (for internal visibility).
pub fn internal_load_font<'ttf, P: AsRef<Path>>(
pub(super) fn internal_load_font<'ttf, P: AsRef<Path>>(
path: P,
ptsize: u16,
) -> Result<Font<'ttf, 'static>, String> {
Expand All @@ -271,7 +271,10 @@ pub fn internal_load_font<'ttf, P: AsRef<Path>>(
}

/// Internally used to load a font (for internal visibility).
pub fn internal_load_font_from_ll<'ttf, 'r, R>(raw: *mut ttf::TTF_Font, rwops: R) -> Font<'ttf, 'r>
pub(super) fn internal_load_font_from_ll<'ttf, 'r, R>(
raw: *mut ttf::TTF_Font,
rwops: R,
) -> Font<'ttf, 'r>
where
R: Into<Option<RWops<'r>>>,
{
Expand All @@ -283,7 +286,7 @@ where
}

/// Internally used to load a font (for internal visibility).
pub fn internal_load_font_at_index<'ttf, P: AsRef<Path>>(
pub(super) fn internal_load_font_at_index<'ttf, P: AsRef<Path>>(
path: P,
index: u32,
ptsize: u16,
Expand All @@ -308,7 +311,7 @@ impl<'ttf, 'r> Font<'ttf, 'r> {
// this can prevent introducing UB until
// https://github.com/rust-lang/rust-clippy/issues/5953 is fixed
#[allow(clippy::trivially_copy_pass_by_ref)]
unsafe fn raw(&self) -> *mut ttf::TTF_Font {
fn raw(&self) -> *mut ttf::TTF_Font {
self.raw
}

Expand Down
4 changes: 1 addition & 3 deletions src/sdl2/ttf/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,7 @@
mod context;
mod font;

pub use self::context::{
get_linked_version, has_been_initialized, init, InitError, Sdl2TtfContext,
};
pub use self::context::{get_linked_version, has_been_initialized, init, Sdl2TtfContext};
pub use self::font::{
Font, FontError, FontResult, FontStyle, GlyphMetrics, Hinting, PartialRendering,
};
Loading