Implement sdl2::hint::video_minimize_on_focus_lost #639

phrohdoh · 2017-04-18T03:23:32Z

Let's decide on documentation, namespacing, etc for a single function before diving into a ton of more complex hints.

phrohdoh · 2017-04-18T03:25:25Z

src/sdl2/hint.rs

+///    - `false`: do not minimize the SDL_Window if it loses key focus when in fullscreen mode
+///
+/// [Official SDL documentation](https://wiki.libsdl.org/SDL_HINT_VIDEO_MINIMIZE_ON_FOCUS_LOSS)
+pub fn video_minimize_on_focus_lost(value: bool) -> bool {


Do you want value renamed to something like should_minimize?

As long as you can precisely which value does what, I'm fine with both. If there isn't any ambiguity like "I don't know if true or false actually enables the hint that I want to use because there is 'DISABLE' in the name" (I don't know if there is any actual hint like that, but just as an example). If there is no room for ambiguity, it's fine by me.

phrohdoh · 2017-04-18T03:25:47Z

src/sdl2/hint.rs

+/// Sets the SDL_HINT_VIDEO_MINIMIZE_ON_FOCUS_LOSS hint.
+///
+/// # Default
+/// By default a SDL_Window is minimized if it loses key focus when in fullscreen mode.


Do you want each SDL_Window to be a link to the SDL_Window struct docs?

You don't have to. One link to the official document is enough, users are expected to go look at the official SDL2 documentation if they need additional information.

phrohdoh · 2017-04-18T03:26:03Z

src/sdl2/hint.rs

@@ -3,6 +3,27 @@ use sys::hint as ll;
 use std::ptr;
 use libc::c_char;

+const VIDEO_MINIMIZE_ON_FOCUS_LOST: &'static str = "SDL_VIDEO_MINIMIZE_ON_FOCUS_LOST";
+
+/// Sets the SDL_HINT_VIDEO_MINIMIZE_ON_FOCUS_LOSS hint.


Should this be just SDL_HINT_VIDEO_MINIMIZE_ON_FOCUS_LOSS?

How about A hint that specifies if a SDL_Window is minimized if it loses key focus when in fullscreen mode. (directly from sdl's docs)?

How about A hint that specifies if a SDL_Window is minimized if it loses key focus when in fullscreen mode. (directly from sdl's docs)?

Perfect

phrohdoh · 2017-04-18T03:30:18Z

I've never written a macro but the simple "1"/"0" hints ought to be a good candidate for a set wrapper macro I think.

Opinions?

Cobrand · 2017-04-18T03:37:04Z

I've never written a macro but the simple "1"/"0" hints ought to be a good candidate for a set wrapper macro I think.

Definitely, but make sure the macro cannot only be seen from within sdl2::hint and nowhere else

Cobrand · 2017-04-18T03:44:28Z

src/sdl2/hint.rs

+///
+/// * `value`:
+///    - `true`: do minimize the SDL_Window if it loses key focus when in fullscreen mode
+///    - `false`: do not minimize the SDL_Window if it loses key focus when in fullscreen mode


You don't have to repeat yourself or enumerate every value like that, this would have been enough :

/// * `value`: `true` to enable minimizing of the Window if it loses key focus when in fullscreen mode , `false` to disable this feature. Defaults to X

If it has a default value (and it is explicitly said in the SDL2 documentation), it would be nice to include what is the default value as well.

By the way, you don't have to say SDL_Window instead of Window when the equivalence is this obvious.

phrohdoh · 2017-04-18T12:42:08Z

@Cobrand Please take another look.

phrohdoh · 2017-04-18T13:13:54Z

Thinking about this a little more... do you think people would benefit from us implementing a fn {get,set}_* for each hint (how about fn set_*_with_priority(..))?

Assuming we can make all of the hints typesafe and take the individual get/set approach I don't see any gain in exposing set, set_with_priority, and get.

Cobrand · 2017-04-19T03:57:41Z

Assuming we can make all of the hints typesafe and take the individual get/set approach I don't see any gain in exposing set, set_with_priority, and get.
I think they still need to be exposed, just in case SDL2 adds another Hint in the future and we aren't reactive enough to implement it on time in this crate. However, while it can be exposed, documentation must be very clear that the appropriate function should be used instead of only set and get when it is possible.

phrohdoh · 2017-04-19T22:34:44Z

Updated, please review again.

I'm sorry we're going back and forth over and over -- I believe this revision should be all that is necessary functionally.

The documentation is quite repetitive though so if you want that redone a particular way just let me know.

This patch allows me to change some game code like so:

diff --git a/crates/media/src/renderer.rs b/crates/media/src/renderer.rs
index b7f9bee..6f98b78 100644
--- a/crates/media/src/renderer.rs
+++ b/crates/media/src/renderer.rs
@@ -57,7 +57,7 @@ impl Renderer {
                 #[cfg(target_os="macos")]
                 { builder.position(0, 0); }

-                sdl2::hint::set("SDL_VIDEO_MINIMIZE_ON_FOCUS_LOSS", "0");
+                sdl2::hint::set_video_minimize_on_focus_lost(false);
             } else {
                 builder.resizable();
             }

phrohdoh · 2017-04-19T22:38:56Z

src/sdl2/hint.rs

+pub fn get_video_minimize_on_focus_lost() -> bool {
+    match get(VIDEO_MINIMIZE_ON_FOCUS_LOST) {
+        Some(value) => match &*value {
+            "1" => true,


I deliberately chose to map anything non-"1" to false because a consumer could set a hint to "foobar" if they wanted to for whatever reason but, AFAIK, that will not enable the feature (ignoring the default of being enabled for this particular hint).

phrohdoh · 2017-04-19T22:50:52Z

Build failed because of docs.
I'll fix this shortly.

…OCUS_LOST

Cobrand · 2017-04-19T23:04:19Z

The 'set_with_priority' is kind of verbose, how about merging both 'set_*' into one which takes value and priority: Option<Hint>? When priority is None it would be like no priority at all, and setting to Some(_) will set call the function with the matching priority.

What do you think ?

phrohdoh · 2017-04-19T23:25:43Z

I agree with your observation that the priority-variant is overly verbose.

Reasons I see for not going the Option<Hint> route:

sdl2::hint::set_video_minimize_on_focus_lost(false) would become sdl2::hint::set_video_minimize_on_focus_lost(false, None) to get the same behavior that users would probably expect from the option-less form.
When someone with SDL2 knowledge reads sdl2::hint::set_video_minimize_on_focus_lost(false, None) they will be forced to look up documentation while thinking "What could possibly be none/nullable here? Is this actually optional or was this made optional to create a smaller API surface?" instead of instantly understanding what is happening here.
Instead of using Option<Hint> we should just use Hint because Hint::Default exists.
3a. If we fold the two together and have a single fn set_video_...(value: bool, priority: Hint) then set should be removed and set_with_priority should be renamed to set for consistency (but this would be a breaking change, which isn't acceptable)

Counter points:

Users should be reading the docs already and (eventually) tooling will make this easier (IDE suggestions, etc)
We are already deviating from / adding on to SDL2 by introducing these functions so pre-existing knowledge isn't a good argument.

I do not have a counter point for 3 and because of that I think using Option here as a shortcut for Hint::Default isn't a good idea.

Cobrand · 2017-04-19T23:33:52Z

Good point for Hint::Default. It's still a hassle because you have to look up Hint to know which variant should be used by default while with Option it would instantly make sense if you read the documentation of any of the set_hint function. Anyway you can keep it like this or make every function take value & hint as parameters, it's up to you, I'm fine with either one. Two functions for each is kind of verbose but it can't be helped I guess.

…

On Apr 19, 2017, 16:25, at 16:25, Taryn Hill ***@***.***> wrote: Reasons I see for not going the `Option<Hint>` route: 1) `sdl2::hint::set_video_minimize_on_focus_lost(false)` would become `sdl2::hint::set_video_minimize_on_focus_lost(false, None)` to get the same behavior that users would probably expect from the option-less form. 2) When someone with SDL2 knowledge reads `sdl2::hint::set_video_minimize_on_focus_lost(false, None)` they will be forced to look up documentation while thinking "What could possibly be none/nullable here? Is this actually optional or was this made optional to create a smaller API surface?" instead of instantly understanding what is happening here. 3) Instead of using `Option<Hint>` we should just use `Hint` because `Hint::Default` exists. Counter points: 1) Users should be reading the docs already and (eventually) tooling will make this easier (IDE suggestions, etc) 2) We are already deviating from / adding on to SDL2 by introducing these functions so pre-existing knowledge isn't a good argument. I do not have a counter point for 3 and because of that I think using `Option` here as a shortcut for `Hint::Default` isn't a good idea. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #639 (comment)

phrohdoh · 2017-04-19T23:46:27Z

I'll leave it as-is then and hope this won't become a pain point or irritation for users.

Do you have an opinions regarding the documentation? If not this is ready to merge if desired.

Cobrand · 2017-04-19T23:50:12Z

Merged, thanks again !

phrohdoh · 2017-04-19T23:50:28Z

Awesome, thank you!

phrohdoh commented Apr 18, 2017

View reviewed changes

Cobrand reviewed Apr 18, 2017

View reviewed changes

phrohdoh commented Apr 19, 2017

View reviewed changes

Implement functions in sdl2::hint specific to SDL_VIDEO_MINIMIZE_ON_F…

5d2bfa7

…OCUS_LOST

Cobrand merged commit 1f1d8fa into Rust-SDL2:master Apr 19, 2017

phrohdoh deleted the hints-minimize-on-focus-lost branch April 19, 2017 23:50

phrohdoh mentioned this pull request Apr 21, 2017

Make hints more idiomatic #638

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement sdl2::hint::video_minimize_on_focus_lost #639

Implement sdl2::hint::video_minimize_on_focus_lost #639

phrohdoh commented Apr 18, 2017 •

edited

Loading

phrohdoh Apr 18, 2017

Cobrand Apr 18, 2017

phrohdoh Apr 18, 2017

Cobrand Apr 18, 2017

phrohdoh Apr 18, 2017

phrohdoh Apr 18, 2017

Cobrand Apr 18, 2017

phrohdoh commented Apr 18, 2017

Cobrand commented Apr 18, 2017

Cobrand Apr 18, 2017

phrohdoh commented Apr 18, 2017 •

edited

Loading

phrohdoh commented Apr 18, 2017 •

edited

Loading

Cobrand commented Apr 19, 2017

phrohdoh commented Apr 19, 2017 •

edited

Loading

phrohdoh Apr 19, 2017 •

edited

Loading

phrohdoh commented Apr 19, 2017

Cobrand commented Apr 19, 2017

phrohdoh commented Apr 19, 2017 •

edited

Loading

Cobrand commented Apr 19, 2017 via email

phrohdoh commented Apr 19, 2017

Cobrand commented Apr 19, 2017

phrohdoh commented Apr 19, 2017

Implement sdl2::hint::video_minimize_on_focus_lost #639

Implement sdl2::hint::video_minimize_on_focus_lost #639

Conversation

phrohdoh commented Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phrohdoh commented Apr 18, 2017

Cobrand commented Apr 18, 2017

Choose a reason for hiding this comment

phrohdoh commented Apr 18, 2017 • edited Loading

phrohdoh commented Apr 18, 2017 • edited Loading

Cobrand commented Apr 19, 2017

phrohdoh commented Apr 19, 2017 • edited Loading

phrohdoh Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

phrohdoh commented Apr 19, 2017

Cobrand commented Apr 19, 2017

phrohdoh commented Apr 19, 2017 • edited Loading

Cobrand commented Apr 19, 2017 via email

phrohdoh commented Apr 19, 2017

Cobrand commented Apr 19, 2017

phrohdoh commented Apr 19, 2017

phrohdoh commented Apr 18, 2017 •

edited

Loading

phrohdoh commented Apr 18, 2017 •

edited

Loading

phrohdoh commented Apr 18, 2017 •

edited

Loading

phrohdoh commented Apr 19, 2017 •

edited

Loading

phrohdoh Apr 19, 2017 •

edited

Loading

phrohdoh commented Apr 19, 2017 •

edited

Loading