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

Added bunch of SDL_Window* functions #278

Closed
wants to merge 2 commits into from

Conversation

Tairesh
Copy link
Contributor

@Tairesh Tairesh commented Aug 29, 2021

I don't know if it is necessary for anyone else, but I needed some SDL functions like SDL_SetWindowPosition that Tetra currently doesn't provide, so I added some of them. I'm a very inexperienced Rust programmer so I may have just made some mess and nonsense, but if it would be useful I'd be glad to share this. Also sorry for my English.

@17cupsofcoffee
Copy link
Owner

I'm definitely in favour of adding these! I just need to give some thought to the naming/API consistency - will try to give a proper review in the next day or two.

17cupsofcoffee added a commit that referenced this pull request Sep 4, 2021
@17cupsofcoffee
Copy link
Owner

Okay, I finally got round to reviewing this, and made the following tweaks:

  • I renamed raise to focus, to make it clearer that calling it will also steal input focus. I also added a disclaimer about this to the docs, as it's quite a disruptive action to take without the user being aware it's going to happen.
  • I changed all the functions dealing with window sizes to take i32 instead of u32 - I'm not sure if this is correct, but it's consistent with the rest of the window API. If this does turn out to be the wrong choice, we can always change everything to u32 in 0.7 or something like that.
    • For what it's worth, SDL itself takes ints for these functions, it's the sdl2-rs bindings that are adding validation on top, which I think is a bit overkill.
  • I removed sdl2::video::WindowPos from the public API and replaced it with tetra::window::WindowPosition, for two reasons:
    • Stuff from sdl2 or glow should never be exposed in Tetra's API - the idea behind the platform module is to abstract over these crates so that they can be switched out later (e.g. if I wanted to port to consoles or the web).
    • WindowPos doesn't expose SDL2's functionality for centering a window on a second monitor - I was able to bring this back via some slightly hacky code.
  • I removed get_border_size entirely, as I don't think that's something that other windowing libraries expose (it seems more common to have seperate functions for getting the client area and the outer size of the window), so it wouldn't be portable.

I've merged this PR manually with my tweaks included (b2a1022), so closing this now :)

@Tairesh Tairesh deleted the more_sdl_functions branch September 4, 2021 13:11
@sumibi-yakitori
Copy link
Contributor

@Tairesh @17cupsofcoffee
I needed a set of these functions as well, but I was hesitant to send a PR because I didn't think I should publish functions that could be highly dependent on SDL. Thanks for adding them.

@17cupsofcoffee
Copy link
Owner

@sumibi-yakitori: If you (or anyone else) are ever considering a PR but aren't sure if it'd get accepted, please open an issue or a discussion thread - I'm always happy to do a bit of research and let you know my thoughts 🙂

I'm usually happy to expose SDL2 functionality as long as there's a good use case for it and the API isn't miles away from what other windowing libraries have available - for example, I don't think all other windowing libraries have built-in 'center the window' functionality, but it'd not be too difficult to build it on top of their functions for getting the monitor size/setting the window position.

@sumibi-yakitori
Copy link
Contributor

sumibi-yakitori commented Sep 21, 2021

@17cupsofcoffee

OK, I'll do that next time!
Do you have any plans to make tetra compatible with WASM?
I don't really need it at the moment, but if you do, it will affect the design of APIs around Window.

@17cupsofcoffee
Copy link
Owner

@sumibi-yakitori: I would like to make Tetra WASM-compatible in the future (I actually have a prototype branch of it!), but desktop is probably going to remain the main focus, and I wouldn't be opposed to having stuff that's desktop-only as long as it's clearly documented.

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.

3 participants