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

Fix click #14

Closed
wants to merge 15 commits into from
Closed

Conversation

Nielsbishere
Copy link
Contributor

@Nielsbishere Nielsbishere commented Dec 4, 2019

vurtun/nuklear#803
keharriso's Prevent clicking more than one (overlapping) button per frame.

Problem with existing PR was as follows:

  • Sets click to 0 every start of frame
  • Input was still down for that frame, so it triggered the next event

Now it waits for your button to go back up before resetting the mouse's clicked variable, so as long as your mouse is down, only one item is clicked.

… for vertices (they check correctly now).

Paq.sh now automatically outputs to nuklear.h.
keharriso's Prevent clicking more than one (overlapping) button per frame.
However, this has a couple of other changes that should fix dropdowns from intefering as well.
- Sets click to 0 every start of frame
- Input was still down for that frame, so it triggered the next event
Now it waits for your button to go back up before resetting the clicked variable, so as long as your mouse is down, only one item is clicked.
Copy link
Member

@dumblob dumblob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks. I'll merge it after the conflicts with #13 will get resolved and a version bump will be made (I'm not against merging both PRs into one if it's easier for you).

@@ -78,6 +78,8 @@ nk_button_behavior(nk_flags *state, struct nk_rect r,
#else
nk_input_is_mouse_pressed(i, NK_BUTTON_LEFT);
#endif
if (ret)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: indentation seems off in the whole PR 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 3 tabs on that line, but nuklear.h uses spaces for indentation. At best, we should stick with that instead of introducing inconsistencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, I think.

@@ -108,7 +110,7 @@ nk_draw_button(struct nk_command_buffer *out,
}
NK_LIB int
nk_do_button(nk_flags *state, struct nk_command_buffer *out, struct nk_rect r,
const struct nk_style_button *style, const struct nk_input *in,
const struct nk_style_button *style, struct nk_input *in,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing const here (an other places) is a breaking change in cases where people use -Werror, because users might have written code simular to this:

const struct nk_input *in = ...
...
nk_do_button(state, out, r, style, in, behavior, content)

Which would now throw this error:

error: passing 'const struct nk_input *' to parameter of type 'struct nk_input *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]

Should the major version be incremented because of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree but it's the only straightforward way to fix it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the missing consts are a pity, but I also don't see any more suitable way to solve it. Maybe we should increase the minor part of the version instead of the bugfix part to emphasize this breaking change (but in the context of the usage of Nuklear, I think this does not deserve a major version bump).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Yea bumping major really is overkill, even though it is consistent with the documentation for the changelog. I think minor bump + emphasis is good enough.

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.

None yet

3 participants