sdl.cr icon indicating copy to clipboard operation
sdl.cr copied to clipboard

Add 3 functions for getting the display mode

Open bastienleonard opened this issue 3 years ago • 3 comments

  • get_desktop_display_mode()
  • get_current_display_mode()
  • Window.display_index

(Maybe the first two functions should go in a new video file? SDL's wiki lists them under the Video category.)

bastienleonard avatar Aug 23 '22 21:08 bastienleonard

Reworked the commit and MR to include Window.display_index, which is necessary to use the other two functions.

bastienleonard avatar Aug 25 '22 00:08 bastienleonard

I made the changes that you requested. I was hoping that GitHub would update the diffs, but that doesn't to be the case.

What about dropping the get_ prefixes, so that we'd have SDL.desktop_display_mode and SDL.current_display_mode?

Personally I generally prefer that functions don't have names that would be typical of attributes. But it depends on the convention you're using for the project, so it's your call.

I'm wondering whether there could be a single function and a DisplayMode enum (Desktop, Current), so we'd call SDL.display_mode(:current, 0) and SDL.display_mode(:desktop, 0) for example, but that's probably overkill.

Personally I find that SDL2 is well designed overall for a C library and I prefer when bindings just adapt to the target language's idions. For example, in Rust I prefer to use the sys library that is just an unsafe wrapper around the C API, rather the full-fledged API which changes everything for no apparent reason.

bastienleonard avatar Sep 01 '22 00:09 bastienleonard

I'm following Ruby/Crystal practice of avoiding the get/set/is/has prefixes when possible:

  • get_name -> name
  • set_name -> name=
  • has_name -> name?

For example window.cr is full of these.

ysbaddaden avatar Sep 01 '22 09:09 ysbaddaden

If the goal is to make the method look like an attribute, then that seems in conflict with the fact that it takes a parameter (the display index).

I have added a commit with an alternative API. Typical usage would become SDL.desktop_display_modes[window.display_index]. One downside is that it makes the API slightly magical, which will confuse users if the abstraction leaks (e.g. in case of SDL error, users will have to realize it's not an actual array).

Update: added a size() method.

bastienleonard avatar Sep 03 '22 05:09 bastienleonard

Now that sounds more complicated than necessary. I preferred the simple calls, just skipping the get_ prefix was enough. I like the addition of SDL_GetNumVideoDisplays. Just the following methods would be great; they're close enough to SDL:

SDL.desktop_display_mode(display_index: Int32) : SDL::DisplayMode
SDL.current_display_mode(display_index: Int32) : SDL::DisplayMode
SDL.num_video_displays() : Int32
SDL::Window#display_index() : Int32

Alternatively, for something that feels closer to Crystal, I'd introduce a struct SDL::Display with an @index attribute, and methods to access the display functions (e.g. #name, #bounds, #desktop_mode, #current_mode, ...). Add the following methods, and it would be very nice:

SDL::Window#display : SDL::Display
SDL.displays : Array(SDL::Display)

ysbaddaden avatar Sep 05 '22 12:09 ysbaddaden

Sounds good, I removed the proposal commit and added a new one where I started working on the new API.

bastienleonard avatar Sep 07 '22 21:09 bastienleonard

Can you give me feedback on the last commit?

bastienleonard avatar Oct 10 '22 22:10 bastienleonard

Thanks for your patience!

Let's go with these :)

ysbaddaden avatar Oct 11 '22 07:10 ysbaddaden