Rust-SDL2 / rust-sdl2

SDL2 bindings for Rust
MIT License
2.72k stars 467 forks source link

Reasoning for WindowBuilder methods taking '&mut self' while CanvasBuilder methods take 'mut self'? #1167

Open BeefaloKing opened 2 years ago

BeefaloKing commented 2 years ago

I'm quite new to Rust, so maybe there's something going on I don't understand, but it seems awkward that the window and canvas builders behave differently.

Relevant snippets: https://github.com/Rust-SDL2/rust-sdl2/blob/fe411c6b8765d53654b24886524ee2f3d94892b2/src/sdl2/render.rs#L741-L744 https://github.com/Rust-SDL2/rust-sdl2/blob/fe411c6b8765d53654b24886524ee2f3d94892b2/src/sdl2/video.rs#L1145-L1149

If you set your configuration and build all at once, then you don't notice the difference.

let window = video.window(name, width, height)
    .position_centered()
    /* etc. */
    .build()
    .unwrap();

let canvas = window.into_canvas()
    .present_vsync()
    /* etc. */
    .build()
    .unwrap();

However, if for some reason you wanted to split things into discrete steps (say to allow for different options to be selected at runtime based on a user provided config), then they no longer behave the same.

let mut window_builder = video.window(name, width, height);
window_builder.position_centered();
/* etc. */
let window = window_builder.build().unwrap();

let mut canvas_builder = window.into_canvas();
canvas_builder = canvas_builder.present_vsync();
/* etc */
let canvas = canvas_builder.build().unwrap();

I don't really have any strong arguments for doing things one way or the other and don't know which is "more correct," and I guess it's kind of a non-issue since the differences will most often be hidden, but it just seems awkward that it's inconsistent. Changing it could potentially be breaking though, so maybe it's stuck. Thoughts?

Cobrand commented 2 years ago

Changing it could potentially be breaking though, so maybe it's stuck. Thoughts?

That's the whole issue. Usually builder patterns take mut self instead of &mut self, that's why we did it for Canvas, but applying that change to WindowBuilder would be a (not important) breaking change, and it would probably be unwelcome as well.

BeefaloKing commented 2 years ago

I guess that's good to know. Thanks for the reply.

Like I said, it does seem like a non-issue. If the change would be too breaking to be worth the consistency, it's probably safe to close this.