glfw: window hint default values parity test with attributes #81

2021-11-18T22:03:39Z

InKryption commented

2021-11-18 22:03:39 +00:00

(Migrated from github.com)

Tests pass on my machine right now, but also unsure whether this is as comprehensive as it could be, hence the mess of comments. Any feedback or insights would be greatly appreciated.

By selecting this checkbox, I agree to license my contributions to this project under the license(s) described in the LICENSE file, and I have the right to do so or have received permission to do so by an employer or client I am producing work for whom has this right.

Tests pass on my machine right now, but also unsure whether this is as comprehensive as it could be, hence the mess of comments. Any feedback or insights would be greatly appreciated. - [x] By selecting this checkbox, I agree to license my contributions to this project under the license(s) described in the LICENSE file, and I have the right to do so or have received permission to do so by an employer or client I am producing work for whom has this right.

emidoots commented

2021-11-20 03:10:21 +00:00

(Migrated from github.com)

Linux: confirmed this passed (did not log an error) in CI
Windows: I haven't tested, CI for Windows is currently headless so I'll need to test there manually
Mac: Haven't tested, our x86_64 CI is headless

* Linux: confirmed this passed (did not log an error) in CI * Windows: I haven't tested, CI for Windows is currently headless so I'll need to test there manually * Mac: Haven't tested, our x86_64 CI is headless

emidoots commented

2021-11-20 03:10:42 +00:00

(Migrated from github.com)

Can you clarify which OS your machine is?

emidoots (Migrated from github.com) reviewed

2021-11-20 03:14:05 +00:00

glfw/src/Window.zig

thought: maybe using a global var testing_set_no_hints and comptime if inside Window.create to disable the hinting calls would be simpler than all of this logic?

thought: maybe using a global var `testing_set_no_hints` and `comptime if` inside `Window.create` to disable the hinting calls would be simpler than all of this logic?

emidoots (Migrated from github.com) reviewed

2021-11-20 03:14:41 +00:00

glfw/src/Window.zig

All else equal, I'd prefer to keep tests a bit isolated (and consistent with eachother) so maybe ditch the failedToCreateWindow(err) in this case

All else equal, I'd prefer to keep tests a bit isolated (and consistent with eachother) so maybe ditch the `failedToCreateWindow(err)` in this case

emidoots (Migrated from github.com) reviewed

2021-11-20 03:16:55 +00:00

glfw/src/Window.zig

What do you mean by this? "as we've chosen a different default value"?

emidoots (Migrated from github.com) reviewed

2021-11-20 03:18:34 +00:00

glfw/src/Window.zig

not worried about these

emidoots (Migrated from github.com) reviewed

2021-11-20 03:19:20 +00:00

glfw/src/Window.zig

in recent GLFW versions, doublebuffer can also only be queried not set (which is super reasonable)

I'd say it's fine if we do not test all of these.

in recent GLFW versions, doublebuffer can also only be queried not set (which is super reasonable) I'd say it's fine if we do not test all of these.

emidoots commented

2021-11-20 03:20:02 +00:00

(Migrated from github.com)

I left some thoughts inline, generally this looks great to me and it's OK if we don't have perfect coverage of every possible hint IMO. Just having something in place here for general sanity works for me.

InKryption commented

2021-11-20 11:28:57 +00:00

(Migrated from github.com)

@slimsag Sorry, it's been a rough couple of days for me. I'll try and get to your comments. And also, my primary OS is currently Windows 10, so tests passed there. Should have clarified that.

InKryption (Migrated from github.com) reviewed

2021-11-20 11:42:59 +00:00

glfw/src/Window.zig

That's fair. I made it because I was running into some weird issues with block return and 'catching' errors from it, but I eventually figured it out, just didn't think to remove it.

InKryption (Migrated from github.com) reviewed

2021-11-20 11:46:33 +00:00

glfw/src/Window.zig

Are you implying we use a global comptime var? Because I don't think that's possible. Or do you mean something like const testing_set_no_hints = std.meta.globalOption("glfw_testing_set_no_hints") orelse false? I don't know if that would work, since I don't think (?) tests are treated as "root", though I'll have to check that.

Are you implying we use a global `comptime var`? Because I don't think that's possible. Or do you mean something like `const testing_set_no_hints = std.meta.globalOption("glfw_testing_set_no_hints") orelse false`? I don't know if that would work, since I don't think (?) tests are treated as "root", though I'll have to check that.

InKryption (Migrated from github.com) reviewed

2021-11-20 12:05:00 +00:00

glfw/src/Window.zig

In the discussion here in my window hints rework pr, me and @silversquirl came to the accord that, seeing as GLFW doesn't specify a default value for context_no_error in their docs in this list, it would be most sane to default it to false (seeing as, in zig, we almost always want to handle errors with error codes, and avoid UB).

Though, thinking about it now, and looking at the source code, it actually is defaulted to false (albeit not directly, it's done through a call to memset), so there's no actual difference in behavior here. Though, maybe this calls for a pr upstream to add its default value to the aforementioned list in their docs? Then I could amend this "patch" in this pr.

In the discussion [here](https://github.com/hexops/mach/pull/71#discussion_r748416222) in my window hints rework pr, me and @silversquirl came to the accord that, seeing as GLFW doesn't specify a default value for `context_no_error` in their docs in [this list](https://www.glfw.org/docs/latest/window_guide.html#window_hints_values), it would be most sane to default it to false (seeing as, in zig, we almost always want to handle errors with error codes, and avoid UB). Though, thinking about it now, and looking at the source code, it actually is defaulted to false (albeit not directly, it's done through a call to `memset`), so there's no actual difference in behavior here. Though, maybe this calls for a pr upstream to add its default value to the aforementioned list in their docs? Then I could amend this "patch" in this pr.

InKryption (Migrated from github.com) reviewed

2021-11-20 12:28:34 +00:00

glfw/src/Window.zig

A different solution occurred to me. Make a normal global variable, but with this declaration:

var testing_ignore_hints_struct = if (@import("builtin").is_test) false else @as(void, {});

Which would change the code inside create to:

const ignore_hints_struct = if (comptime @import("builtin").is_test) testing_ignore_hints_struct else false;
if (!ignore_hints_struct) {
    try hints.set();
    defer defaultHints() catch unreachable; // this should be unreachable, being that this should be caught in the previous call to `Hints.set`.
}

A different solution occurred to me. Make a normal global variable, but with this declaration: ```zig var testing_ignore_hints_struct = if (@import("builtin").is_test) false else @as(void, {}); ``` Which would change the code inside `create` to: ```zig const ignore_hints_struct = if (comptime @import("builtin").is_test) testing_ignore_hints_struct else false; if (!ignore_hints_struct) { try hints.set(); defer defaultHints() catch unreachable; // this should be unreachable, being that this should be caught in the previous call to `Hints.set`. } ```

InKryption (Migrated from github.com) reviewed

2021-11-20 12:43:49 +00:00

glfw/src/Window.zig

I've committed the changes. I'll leave this as unresolved in case you have anything you want to clarify or suggest.

emidoots (Migrated from github.com) reviewed

2021-11-20 18:39:38 +00:00

glfw/src/Window.zig

Ahh okay, I see. I was aware of the fact that it was implicitly set to false in GLFW through memset (I double checked the defaults when I reviewed, which was why I was surprised to see this TODO)

In that case, I am happy with this as-is and think we can just remove the TODO mention here.

Ahh okay, I see. I was aware of the fact that it was implicitly set to false in GLFW through `memset` (I double checked the defaults when I reviewed, which was why I was surprised to see this TODO) In that case, I am happy with this as-is and think we can just remove the TODO mention here.

emidoots commented

2021-11-20 18:41:05 +00:00

(Migrated from github.com)

@InKryption thanks a ton! And sorry to hear that :) FWIW your PRs have made my day for the past week or so, I really appreciate what you're doing, If I can buy you a coffee or two, let me know.

❤️ 1

emidoots (Migrated from github.com) reviewed

2021-11-20 18:43:08 +00:00

glfw/src/Window.zig

					
				@ -3221,3 +3224,103 @@ test "setScrollCallback" {

				        }

				    }).callback);

				}

        // Future: we could consider hint values that can't be retrieved via attributes:

```suggestion // Future: we could consider hint values that can't be retrieved via attributes: ```

emidoots (Migrated from github.com) reviewed

2021-11-20 18:43:28 +00:00

glfw/src/Window.zig

					
				@ -3221,3 +3224,103 @@ test "setScrollCallback" {

				        }

				    }).callback);

				}

```suggestion ```

emidoots commented

2021-11-20 18:44:08 +00:00

(Migrated from github.com)

Pretty much LGTM!

emidoots (Migrated from github.com) reviewed

2021-11-20 18:44:51 +00:00

glfw/src/Window.zig

Smart approach - I wouldn't have thought of that. Nice work.

emidoots (Migrated from github.com) approved these changes

2021-11-20 19:39:40 +00:00

InKryption commented

2021-11-20 19:42:57 +00:00

(Migrated from github.com)

@slimsag Appreciate the thought; and no worries, I really like what you have here, and thought I ought to try and help make it better, both for my own purposes, and as an added bonus, for others. As well, seemed like a friendly enough place to try making some more significant contributions for the first time.

❤️ 1

Sign in to join this conversation.

No reviewers

emidoots