hexops/mach
1
0
Fork 0
Code Issues 152 Pull requests 18 Releases Activity Actions 3

glfw: Window hints rework #71

Merged
InKryption merged 21 commits from window-hints-rework into main 2021-11-16 01:41:17 +00:00
Conversation 42 Commits 21 Files changed 5 +287 -303
InKryption commented 2021-11-11 17:34:04 +00:00 (Migrated from github.com)
Copy link

I took a different approach to the one used for initialization hints, because I realized that it would probably a lot of overhead to call Window.hint for every possible hint, compared to how status quo allows one to avoid that overhead; as well, it would probably be a lot harder to maintain, given that, unlike InitHint, window initialization hints are of more types than just booleans.
This approach is (quite a bit) more verbose, but avoids the overhead, and probably makes it easier to maintain.

Also, I found that context_revision and context_no_error are listed in Window.Hint, but are not listed as hints in the docs, only as attributes; I commented them out to make what I have pass tests - is this intentional and I'm missing something, or is this the result of some mishap?

Edit: it should also be noted that unlike the change made to initialization hints, I didn't completely re-use the in-place API, because I ran into some issues regarding the type-inference in the Window.hint function, so you'll see I ended up re-implementing the code for Window.hint in HintValue.set (where HintValue is a tagged union, with a tag type of Hint).

Edit: as pointed out by @silversquirl, there isn't actually a lot of overhead involved in the struct approach, so now the union slice is no more. Now just to address the other components of this problem, such as Window.hint, which seems to have some some type inference issues which caused compilation to fail under this new model. Should we try and rework it, or discard it in favor of Hints.set (private function)?

Edit: Closes #65

  • 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.
~~I took a different approach to the one used for initialization hints, because I realized that it would probably a lot of overhead to call `Window.hint` for every possible hint, compared to how status quo allows one to avoid that overhead; as well, it would probably be a lot harder to maintain, given that, unlike `InitHint`, window initialization hints are of more types than just booleans.~~ ~~This approach is (quite a bit) more verbose, but avoids the overhead, and probably makes it easier to maintain.~~ Also, I found that `context_revision` and `context_no_error` are listed in `Window.Hint`, but are not listed as hints in the docs, only as attributes; I commented them out to make what I have pass tests - is this intentional and I'm missing something, or is this the result of some mishap? Edit: ~~it should also be noted that unlike the change made to initialization hints, I didn't completely re-use the in-place API, because I ran into some issues regarding the type-inference in the `Window.hint` function, so you'll see I ended up re-implementing the code for `Window.hint` in `HintValue.set` (where `HintValue` is a tagged union, with a tag type of `Hint`).~~ Edit: as pointed out by @silversquirl, there isn't actually a lot of overhead involved in the struct approach, so now the union slice is no more. ~~Now just to address the other components of this problem, such as `Window.hint`, which seems to have some some type inference issues which caused compilation to fail under this new model. Should we try and rework it, or discard it in favor of `Hints.set` (private function)?~~ Edit: Closes #65 - [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.
silversquirl commented 2021-11-11 23:53:55 +00:00 (Migrated from github.com)
Copy link

it would probably a lot of overhead to call Window.hint for every possible hint

I had this thought initially too, but if you look at the GLFW source, it's probably about the same as calling glfwDefaultWindowHints. I'd say have a hints struct with default values, and just set all of them unconditionally.

If you do want to avoid setting hints that don't need changed though, I'd still recommend going the struct route, and just check if the value is different than the default. This is easy to do by just looping over std.meta.fields, since the typeinfo gives you default values.

A struct results in a nicer API than an array of hints - the latter still feels very much like a C API, not a Zig one.

> it would probably a lot of overhead to call Window.hint for every possible hint I had this thought initially too, but if you look at the [GLFW source](https://github.com/glfw/glfw/blob/master/src/window.c#L255-L426), it's probably about the same as calling `glfwDefaultWindowHints`. I'd say have a hints struct with default values, and just set all of them unconditionally. If you do want to avoid setting hints that don't need changed though, I'd still recommend going the struct route, and just check if the value is different than the default. This is easy to do by just looping over `std.meta.fields`, since the typeinfo gives you default values. A struct results in a nicer API than an array of hints - the latter still feels very much like a C API, not a Zig one.
InKryption commented 2021-11-12 00:05:54 +00:00 (Migrated from github.com)
Copy link

@silversquirl Good point. I don't know why I didn't actually check the source; guess that's what I get for starting this on my laptop at a café. And I guess that's also why I made this a draft.

@silversquirl Good point. I don't know why I didn't actually check the source; guess that's what I get for starting this on my laptop at a café. And I guess that's also why I made this a draft.
InKryption commented 2021-11-12 01:43:39 +00:00 (Migrated from github.com)
Copy link

@silversquirl Do you have any thoughts on what to do with the currently unused Window.hint function?

@silversquirl Do you have any thoughts on what to do with the currently unused `Window.hint` function?
silversquirl commented 2021-11-12 09:34:24 +00:00 (Migrated from github.com)
Copy link

Either move the type conversion logic in set into it, or delete it. Not much point in keeping it if it's not used

Either move the type conversion logic in `set` into it, or delete it. Not much point in keeping it if it's not used
silversquirl (Migrated from github.com) reviewed 2021-11-12 09:34:52 +00:00
glfw/src/Window.zig
@ -258,0 +268,4 @@
OpenGLProfile,
=> c.glfwWindowHint(hint_tag, @enumToInt(hint_value)),
[:0]const u8 => c.glfwWindowHintString(hint_tag, hint_value.ptr),
silversquirl (Migrated from github.com) commented 2021-11-12 09:34:52 +00:00
Copy link

Any particular reason for these parens? Personally I find it more readable without them

Any particular reason for these parens? Personally I find it more readable without them
InKryption (Migrated from github.com) reviewed 2021-11-12 12:55:19 +00:00
glfw/src/Window.zig
@ -258,0 +268,4 @@
OpenGLProfile,
=> c.glfwWindowHint(hint_tag, @enumToInt(hint_value)),
[:0]const u8 => c.glfwWindowHintString(hint_tag, hint_value.ptr),
InKryption (Migrated from github.com) commented 2021-11-12 12:55:19 +00:00
Copy link

My personal reason for it is that I don't like "mixing" whitespace "scopes" (the obligatory space between 'const' and 'u8', and the optional whitespace between 'u8' and '=>'), if that makes any sense. Though, I have no particular attachment to it either, just a sort of typing habit. I can go ahead and change it whenever I get on my laptop.

My personal reason for it is that I don't like "mixing" whitespace "scopes" (the obligatory space between 'const' and 'u8', and the optional whitespace between 'u8' and '=>'), if that makes any sense. Though, I have no particular attachment to it either, just a sort of typing habit. I can go ahead and change it whenever I get on my laptop.
InKryption commented 2021-11-12 14:40:58 +00:00 (Migrated from github.com)
Copy link

Excuse my fumbling with git, trying to make the history not look messy.

Excuse my fumbling with git, trying to make the history not look messy.
silversquirl (Migrated from github.com) reviewed 2021-11-12 15:53:14 +00:00
glfw/src/Window.zig
@ -230,0 +160,4 @@
center_cursor: bool = true,
transparent_framebuffer: bool = false,
focus_on_show: bool = true,
scale_to_monitor: bool = false,
silversquirl (Migrated from github.com) commented 2021-11-12 15:53:14 +00:00
Copy link

The source backs you up here. GLFW_CONTEXT_REVISION is not a hint

The source backs you up here. GLFW_CONTEXT_REVISION is not a hint
InKryption (Migrated from github.com) reviewed 2021-11-12 15:56:52 +00:00
glfw/src/Window.zig
@ -230,0 +160,4 @@
center_cursor: bool = true,
transparent_framebuffer: bool = false,
focus_on_show: bool = true,
scale_to_monitor: bool = false,
InKryption (Migrated from github.com) commented 2021-11-12 15:56:52 +00:00
Copy link

@silversquirl Is that the same case for context_no_error = c.GLFW_CONTEXT_NO_ERROR?

@silversquirl Is that the same case for `context_no_error = c.GLFW_CONTEXT_NO_ERROR`?
silversquirl (Migrated from github.com) reviewed 2021-11-12 15:58:41 +00:00
glfw/src/Window.zig
@ -258,0 +180,4 @@
/// Monitor refresh rate
refresh_rate: c_int = glfw.dont_care,
silversquirl (Migrated from github.com) commented 2021-11-12 15:55:38 +00:00
Copy link

This one is a hint, and is actually listed in the docs

This one is a hint, and is actually [listed in the docs](https://www.glfw.org/docs/latest/window_guide.html#GLFW_CONTEXT_NO_ERROR_hint)
glfw/src/Window.zig
@ -258,0 +207,4 @@
opengl_debug_context: bool = false,
opengl_profile: OpenGLProfile = .opengl_any_profile,
silversquirl (Migrated from github.com) commented 2021-11-12 15:56:45 +00:00
Copy link

Personally I'd remove these constants from consts and inline them here. They don't need to be publicly accessible any more, so there's probably no reason for them to be in consts.

Personally I'd remove these constants from consts and inline them here. They don't need to be publicly accessible any more, so there's probably no reason for them to be in consts.
InKryption (Migrated from github.com) reviewed 2021-11-12 16:07:09 +00:00
glfw/src/Window.zig
@ -258,0 +207,4 @@
opengl_debug_context: bool = false,
opengl_profile: OpenGLProfile = .opengl_any_profile,
InKryption (Migrated from github.com) commented 2021-11-12 16:07:09 +00:00
Copy link

@silversquirl I was thinking the same thing, but was unsure whether to do so being that something like this wasn't done in #56 already.

@silversquirl I was thinking the same thing, but was unsure whether to do so being that something like this wasn't done in #56 already.
InKryption (Migrated from github.com) reviewed 2021-11-12 16:19:00 +00:00
glfw/src/Window.zig
@ -258,0 +180,4 @@
/// Monitor refresh rate
refresh_rate: c_int = glfw.dont_care,
InKryption (Migrated from github.com) commented 2021-11-12 16:18:59 +00:00
Copy link

@silversquirl Right, I didn't see that. I was looking at this list, which lists hints, their default values and possible values. It's not in that list, and I can't see anywhere else that would specify a default value for it; maybe it could default to !std.debug.runtime_safety? (set to true in release modes).

@silversquirl Right, I didn't see that. I was looking [at this list](https://www.glfw.org/docs/latest/window_guide.html#window_hints_values), which lists hints, their default values and possible values. It's not in that list, and I can't see anywhere else that would specify a default value for it; maybe it could default to `!std.debug.runtime_safety`? (set to true in release modes).
silversquirl (Migrated from github.com) reviewed 2021-11-12 16:26:32 +00:00
glfw/src/Window.zig
@ -258,0 +180,4 @@
/// Monitor refresh rate
refresh_rate: c_int = glfw.dont_care,
silversquirl (Migrated from github.com) commented 2021-11-12 16:26:32 +00:00
Copy link

Since we handle errors through Zig error unions, I'd say definitely default it to false. Possibly even remove it from the struct altogether, since it's quite an unsafe option to use - at the very least put a doc comment informing about the dangers.

Since we handle errors through Zig error unions, I'd say definitely default it to false. Possibly even remove it from the struct altogether, since it's quite an unsafe option to use - at the very least put a doc comment informing about the dangers.
InKryption (Migrated from github.com) reviewed 2021-11-12 16:37:00 +00:00
glfw/src/Window.zig
@ -258,0 +180,4 @@
/// Monitor refresh rate
refresh_rate: c_int = glfw.dont_care,
InKryption (Migrated from github.com) commented 2021-11-12 16:37:00 +00:00
Copy link

@silversquirl Alright, I suppose that makes sense. I'll amend it now and leave this as resolved.

@silversquirl Alright, I suppose that makes sense. I'll amend it now and leave this as resolved.
InKryption (Migrated from github.com) reviewed 2021-11-12 17:01:27 +00:00
glfw/src/Window.zig
@ -258,0 +207,4 @@
opengl_debug_context: bool = false,
opengl_profile: OpenGLProfile = .opengl_any_profile,
InKryption (Migrated from github.com) commented 2021-11-12 17:01:26 +00:00
Copy link

@silversquirl I went ahead with the change; the only constant in consts.zig now is dont_care.

@silversquirl I went ahead with the change; the only constant in consts.zig now is `dont_care`.
emidoots commented 2021-11-12 17:22:49 +00:00 (Migrated from github.com)
Copy link

Thanks for all your work on this! It'll probably be a few days before I have a chance to review (at handmade Seattle and then to a wedding) so apologies for the delayed review!

Thanks for all your work on this! It'll probably be a few days before I have a chance to review (at handmade Seattle and then to a wedding) so apologies for the delayed review!
👍 1
InKryption commented 2021-11-12 17:44:23 +00:00 (Migrated from github.com)
Copy link

I had an idea to maybe add a test that compares the attribute values of a window created with Window.from(c.glfwCreateWindow(...)), to the attribute values of a window created with Window.create(...) (in that order), to check that, for any attribute whose value is initialized via Window.Hints.set, its value is the same as what would be set by GLFW itself (maybe excluding context_no_error). This wouldn't be full-coverage, but it would be something. Thoughts?

I had an idea to maybe add a test that compares the attribute values of a window created with `Window.from(c.glfwCreateWindow(...))`, to the attribute values of a window created with `Window.create(...)` (in that order), to check that, for any attribute whose value is initialized via `Window.Hints.set`, its value is the same as what would be set by GLFW itself (maybe excluding context_no_error). This wouldn't be full-coverage, but it would be something. Thoughts?
emidoots (Migrated from github.com) reviewed 2021-11-15 22:40:00 +00:00
glfw/src/Window.zig
@ -258,0 +268,4 @@
OpenGLProfile,
=> c.glfwWindowHint(hint_tag, @enumToInt(hint_value)),
[:0]const u8 => c.glfwWindowHintString(hint_tag, hint_value.ptr),
emidoots (Migrated from github.com) commented 2021-11-15 22:40:00 +00:00
Copy link

Happy to have this as-is and let zig fmt formally sort it out later.

Happy to have this as-is and let `zig fmt` formally sort it out later.
emidoots commented 2021-11-15 22:42:09 +00:00 (Migrated from github.com)
Copy link

@InKryption regarding:

I had an idea to maybe add a test that compares the attribute values of a window

This sounds like an obvious win to me, so yes, please do! I won't block this PR on this, please send another PR for it if I merge before you get to it.

One thing to be mindful of: if we want the test to pass on any OS, be sure to choose hints that work on all OS/platforms.

@InKryption regarding: > I had an idea to maybe add a test that compares the attribute values of a window This sounds like an obvious win to me, so yes, please do! I won't block this PR on this, please send another PR for it if I merge before you get to it. One thing to be mindful of: if we want the test to pass on any OS, be sure to choose hints that work on all OS/platforms.
InKryption commented 2021-11-15 22:43:58 +00:00 (Migrated from github.com)
Copy link

@slimsag Roger roger, I'll set this as ready for merging then, in the absence of any notes you may have.

@slimsag Roger roger, I'll set this as ready for merging then, in the absence of any notes you may have.
emidoots (Migrated from github.com) reviewed 2021-11-15 22:52:08 +00:00
glfw/src/Window.zig
@ -266,3 +283,2 @@
try getError();
}
};
emidoots (Migrated from github.com) commented 2021-11-15 22:52:07 +00:00
Copy link

The reason I had this here was that it is listed in the source as being both a hint and attribute:

github.com/glfw/glfw@fb0f2f92a3/include/GLFW/glfw3.h (L1031-L1036)

However, indeed, I can confirm from looking at the GLFW source that comment in their header seems incorrect. I will send a PR upstream to fix the comment.

The reason I had this here was that it is listed in the source as being both a hint and attribute: https://github.com/glfw/glfw/blob/fb0f2f92a38c1d6a776ffeb253329f8d1c65694c/include/GLFW/glfw3.h#L1031-L1036 However, indeed, I can confirm from looking at the GLFW source that comment in their header seems incorrect. I will send a PR upstream to fix the comment.
emidoots (Migrated from github.com) reviewed 2021-11-15 22:53:15 +00:00
glfw/src/Window.zig
@ -266,3 +283,2 @@
try getError();
}
};
emidoots (Migrated from github.com) commented 2021-11-15 22:53:15 +00:00
Copy link

Sent https://github.com/glfw/glfw/pull/1992

Sent https://github.com/glfw/glfw/pull/1992
emidoots (Migrated from github.com) reviewed 2021-11-15 22:57:32 +00:00
glfw/src/Window.zig
emidoots (Migrated from github.com) commented 2021-11-15 22:57:32 +00:00
Copy link
    // Note: The defaults here are directly from the GLFW source of the glfwDefaultWindowHints function
    resizable: bool = true,
```suggestion // Note: The defaults here are directly from the GLFW source of the glfwDefaultWindowHints function resizable: bool = true, ```
emidoots (Migrated from github.com) reviewed 2021-11-15 22:57:55 +00:00
glfw/src/Window.zig
emidoots (Migrated from github.com) commented 2021-11-15 22:57:55 +00:00
Copy link

Adding this note so that others will know how to verify if these values are correct or not and how to update them in the future

Adding this note so that others will know how to verify if these values are correct or not and how to update them in the future
emidoots (Migrated from github.com) reviewed 2021-11-15 23:01:12 +00:00
glfw/src/Window.zig
emidoots (Migrated from github.com) commented 2021-11-15 23:01:12 +00:00
Copy link

Because these are the only public hints exposed by the API, I think that we need to move all the docstrings from the current const Hint = enum(c_int) { into the fields here. Otherwise, once a Zig documentation generator exists (or even if you're just looking at this code for usage) you would not find any docstrings on these fields.

Because these are the only public hints exposed by the API, I think that we need to move all the docstrings from the current `const Hint = enum(c_int) {` into the fields here. Otherwise, once a Zig documentation generator exists (or even if you're just looking at this code for usage) you would not find any docstrings on these fields.
emidoots (Migrated from github.com) reviewed 2021-11-15 23:03:25 +00:00
glfw/src/Window.zig
@ -258,0 +200,4 @@
context_release_behavior: ContextReleaseBehavior = .any_release_behavior,
/// Note: disables the context creating errors,
/// instead turning them into undefined behavior.
emidoots (Migrated from github.com) commented 2021-11-15 23:03:25 +00:00
Copy link
    opengl_profile: OpenGLProfile = .opengl_any_profile,

From https://ziglang.org/documentation/master/#Names

These are general rules of thumb; if it makes sense to do something different, do what makes sense. For example, if there is an established convention such as ENOENT, follow the established convention.

I would guess OpenGLProfile is more correct than OpenGlProfile

```suggestion opengl_profile: OpenGLProfile = .opengl_any_profile, ``` From https://ziglang.org/documentation/master/#Names > These are general rules of thumb; if it makes sense to do something different, do what makes sense. For example, if there is an established convention such as ENOENT, follow the established convention. I would guess `OpenGLProfile` is more correct than `OpenGlProfile`
emidoots (Migrated from github.com) reviewed 2021-11-15 23:03:55 +00:00
glfw/src/Window.zig
@ -258,0 +200,4 @@
context_release_behavior: ContextReleaseBehavior = .any_release_behavior,
/// Note: disables the context creating errors,
/// instead turning them into undefined behavior.
emidoots (Migrated from github.com) commented 2021-11-15 23:03:54 +00:00
Copy link
    pub const OpenGLProfile = enum(c_int) {
```suggestion pub const OpenGLProfile = enum(c_int) { ```
emidoots (Migrated from github.com) reviewed 2021-11-15 23:04:23 +00:00
glfw/src/Window.zig
@ -258,0 +200,4 @@
context_release_behavior: ContextReleaseBehavior = .any_release_behavior,
/// Note: disables the context creating errors,
/// instead turning them into undefined behavior.
emidoots (Migrated from github.com) commented 2021-11-15 23:04:23 +00:00
Copy link

And same thing for ClientApi and ContextCreationApi I think (use API)

And same thing for `ClientApi` and `ContextCreationApi` I think (use `API`)
emidoots (Migrated from github.com) reviewed 2021-11-15 23:06:24 +00:00
glfw/src/Window.zig
emidoots (Migrated from github.com) commented 2021-11-15 23:06:24 +00:00
Copy link

I believe that this is wrong? It would set the user-specified hints, then overwrite them with the defaults?

I believe that this is wrong? It would set the user-specified hints, then overwrite them with the defaults?
emidoots (Migrated from github.com) reviewed 2021-11-15 23:08:27 +00:00
glfw/src/consts.zig
emidoots (Migrated from github.com) commented 2021-11-15 23:08:27 +00:00
Copy link

Maybe delete this whole file now and just move this constant into main.zig, since we already do a usingnamespace import of this file: github.com/hexops/mach@d5913c3e49/glfw/src/main.zig (L8)

Maybe delete this whole file now and just move this constant into `main.zig`, since we already do a `usingnamespace` import of this file: https://github.com/hexops/mach/blob/d5913c3e4961b1d4f2562900b26b5ef2b7280ed6/glfw/src/main.zig#L8
emidoots (Migrated from github.com) approved these changes 2021-11-15 23:12:03 +00:00
emidoots (Migrated from github.com) left a comment
Copy link

Looks really great, thanks for sending this. Once my comments are addressed and tests pass I'll merge. Feel free to send the test in this PR or a separate one, whichever you prefer.

Looks really great, thanks for sending this. Once my comments are addressed and tests pass I'll merge. Feel free to send the test in this PR or a separate one, whichever you prefer.
InKryption (Migrated from github.com) reviewed 2021-11-15 23:12:09 +00:00
glfw/src/Window.zig
InKryption (Migrated from github.com) commented 2021-11-15 23:12:09 +00:00
Copy link

You are correct, that code is wrong. That probably explains why I was getting some strange results prototyping those hint-attribute tests I mentioned.

You are correct, that code is wrong. That probably explains why I was getting some strange results prototyping those hint-attribute tests I mentioned.
emidoots (Migrated from github.com) reviewed 2021-11-15 23:29:14 +00:00
glfw/src/Window.zig
emidoots (Migrated from github.com) commented 2021-11-15 23:29:14 +00:00
Copy link

Ah, I see your intent now is to "cleanup" the hints so-to-speak by changing them back to the default. This makes sense.

However, if window creation fails we would not clean up. Maybe instead defer and ignore the error? I think defer _ = Window.defaultHints(); would do the trick (but I might have the exact syntax wrong)

Ah, I see your intent now is to "cleanup" the hints so-to-speak by changing them back to the default. This makes sense. However, if window creation fails we would not clean up. Maybe instead `defer` and ignore the error? I think `defer _ = Window.defaultHints();` would do the trick (but I might have the exact syntax wrong)
InKryption (Migrated from github.com) reviewed 2021-11-15 23:41:12 +00:00
glfw/src/Window.zig
InKryption (Migrated from github.com) commented 2021-11-15 23:41:11 +00:00
Copy link

In my original thinking, a defer wouldn't have worked, as you can't try in a defer statement; but looking at it closer, the only possible error for Window.defaultHints is Error.NotInitialized, which should already have been caught in Hints.set, so an error should never be thrown there, a la catch unreachable.

In my original thinking, a `defer` wouldn't have worked, as you can't `try` in a `defer` statement; but looking at it closer, the only possible error for `Window.defaultHints` is `Error.NotInitialized`, which should already have been caught in `Hints.set`, so an error should never be thrown there, a la `catch unreachable`.
InKryption (Migrated from github.com) reviewed 2021-11-15 23:50:57 +00:00
glfw/src/Window.zig
InKryption (Migrated from github.com) commented 2021-11-15 23:50:56 +00:00
Copy link

@slimsag Here, I'm actually unsure if all of the comments in Hint are totally necessary. They about double the length of the struct's declaration, and don't (in my opinion) add that much information. All of them follow the pattern:

    /// Window {name} hint
    {name},

Save for a few, like center_cursor and focus_on_show, and the framebuffer bits hints, which follow their own pattern.
But if you feel strongly about it, I can add the changes.

Edit: gentle ping

@slimsag Here, I'm actually unsure if all of the comments in `Hint` are totally necessary. They about double the length of the struct's declaration, and don't (in my opinion) add that much information. All of them follow the pattern: ```zig /// Window {name} hint {name}, ``` Save for a few, like `center_cursor` and `focus_on_show`, and the framebuffer bits hints, which follow their own pattern. But if you feel strongly about it, I can add the changes. Edit: gentle ping
emidoots (Migrated from github.com) reviewed 2021-11-16 00:40:07 +00:00
glfw/src/Window.zig
emidoots (Migrated from github.com) commented 2021-11-16 00:40:07 +00:00
Copy link

Makes sense to me! I agree having comments just for the sake of it is bad practice, we should remove all such instances. They probably just ended up there as I didn't notice it. I should've checked the comments were actually useful before asking you to do this, sorry 😛

Makes sense to me! I agree having comments just for the sake of it is bad practice, we should remove all such instances. They probably just ended up there as I didn't notice it. I should've checked the comments were actually useful before asking you to do this, sorry 😛
InKryption (Migrated from github.com) reviewed 2021-11-16 00:44:31 +00:00
glfw/src/Window.zig
InKryption (Migrated from github.com) commented 2021-11-16 00:44:31 +00:00
Copy link

All's good. I'll sort through them for a bit, and grab the ones that are meaningful, and polish them up.

All's good. I'll sort through them for a bit, and grab the ones that are meaningful, and polish them up.
emidoots (Migrated from github.com) reviewed 2021-11-16 00:46:14 +00:00
glfw/src/Window.zig
@ -68,126 +69,67 @@ pub const InternalUserPointer = struct {
/// @thread_safety This function must only be called from the main thread.
emidoots (Migrated from github.com) commented 2021-11-16 00:46:13 +00:00
Copy link

I think this became public again by accident / due to merge conflict

I think this became public again by accident / due to merge conflict
emidoots commented 2021-11-16 00:47:54 +00:00 (Migrated from github.com)
Copy link

Looks good to me, let me know when you're finished and I'll merge. I will need to squash merge due to non-linear history, unless you want to rebase everything (I'm happy either way)

Looks good to me, let me know when you're finished and I'll merge. I will need to squash merge due to non-linear history, unless you want to rebase everything (I'm happy either way)
👍 1
InKryption (Migrated from github.com) reviewed 2021-11-16 01:15:06 +00:00
glfw/src/Window.zig
InKryption (Migrated from github.com) commented 2021-11-16 01:15:06 +00:00
Copy link

I've done the sorting; give it a quick look and see if there's anything amiss. If that looks good, then I feel confident about saying this is locked and loaded. I'll look at making a PR for that test/those tests concerning hints/attributes at some point in the near-ish future.

Edit: give me a second to figure out the rebasing, it actually doesn't seem as complicated as I thought.

I've done the sorting; give it a quick look and see if there's anything amiss. If that looks good, then I feel confident about saying this is locked and loaded. I'll look at making a PR for that test/those tests concerning hints/attributes at some point in the near-ish future. Edit: ~~give me a second to figure out the rebasing, it actually doesn't seem as complicated as I thought.~~
emidoots (Migrated from github.com) approved these changes 2021-11-16 01:40:02 +00:00
emidoots (Migrated from github.com) left a comment
Copy link

LGTM, thanks a ton again!

LGTM, thanks a ton again!
InKryption commented 2021-11-16 01:45:34 +00:00 (Migrated from github.com)
Copy link

Closes #65

Closes #65
Sign in to join this conversation.
Reviewers
No reviewers
emidoots
Labels
Clear labels   
CI

   
all

   
basisu

   
blog

   
bug

Something isn't working

  
build

   
contributor-friendly

Good issue for contributors to take on

  
core

   
correctness

   
deferred

   
dev

   
direct3d-headers

   
docs

Improvements or additions to documentation

  
driver-os-issue

Issues that are very likely to be graphics driver / OS configuration issues

  
duplicate

This issue or pull request already exists

  
dxcompiler

   
editor

Graphical editor and Mach CLI

  
examples

   
experiment

   
feature-idea

New feature or request

  
feedback

   
flac

   
freetype

   
gamemode

   
gkurve

   
glfw

   
gpu

   
gpu-dawn

   
harfbuzz

   
help welcome

   
in-progress

   
infrastructure

   
invalid

This doesn't seem right

  
libmach

   
linux-audio-headers

   
long-term

   
mach

   
mach.gfx

   
mach.math

   
mach.physics

   
mach.testing

   
model3d

   
needs-triage

Issues that need triaging

  
object

object system

  
opengl-headers

   
opus

   
os/linux

   
os/macos

   
os/wasm

WebAssembly support

  
os/windows

   
package-manager

   
priority

   
proposal

   
proposal-accepted

   
question

Further information is requested

  
roadmap

   
slipped

   
stability

   
sysaudio

   
sysgpu

   
sysjs

   
validating-fix

   
vulkan-zig-generated

   
wayland-headers

   
website

   
wontfix

This will not be worked on

  
wrench

   
www

   
x11-headers

   
xcode-frameworks

   
zig-update
No labels
CI
all
basisu
blog
bug
build
contributor-friendly
core
correctness
deferred
dev
direct3d-headers
docs
driver-os-issue
duplicate
dxcompiler
editor
examples
experiment
feature-idea
feedback
flac
freetype
gamemode
gkurve
glfw
gpu
gpu-dawn
harfbuzz
help welcome
in-progress
infrastructure
invalid
libmach
linux-audio-headers
long-term
mach
mach.gfx
mach.math
mach.physics
mach.testing
model3d
needs-triage
object
opengl-headers
opus
os/linux
os/macos
os/wasm
os/windows
package-manager
priority
proposal
proposal-accepted
question
roadmap
slipped
stability
sysaudio
sysgpu
sysjs
validating-fix
vulkan-zig-generated
wayland-headers
website
wontfix
wrench
www
x11-headers
xcode-frameworks
zig-update
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
hexops/mach!71
No description provided.