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 bug where width padding was applied twice #2

Merged
merged 1 commit into from
Mar 6, 2020

Conversation

Hejsil
Copy link
Contributor

@Hejsil Hejsil commented Nov 27, 2019

Let me be the first to open a PR on this new repo 🎊
Copied from here

Before:
image

After:
image

@dumblob
Copy link
Member

dumblob commented Nov 27, 2019

Let me be the first to open a PR on this new repo 

Congratulations 😉

Before someone will dive into the review - with which backends did you test this? I'm expecting some differences (as I suspect some backends are buggy in this regard).

@dumblob
Copy link
Member

dumblob commented Nov 27, 2019

Btw. are you willing to review PRs of others? If so, I'll invite you to become an organization member with admin rights.

@Hejsil
Copy link
Contributor Author

Hejsil commented Nov 28, 2019

Tested with the x11_xft backend. I'll try other backends when I get time :)

I'm not really interested in doing reviews at the moment. I'm also not too versed in nuklears codebase, so I don't think I'm qualified to review PRs :)

@Nielsbishere
Copy link
Contributor

Looks useful!

@dumblob
Copy link
Member

dumblob commented Dec 5, 2019

Tested with the x11_xft backend. I'll try other backends when I get time :)

The x11_xft backend is quite a good one, so I'd like to merge this. But anyway I'd like to have this tested with at least one more common backend (e.g. glfw) and I don't have time for that right now. Anyone?

I'm also not too versed in nuklears codebase, so I don't think I'm qualified to review PRs :)

Don't be so humble. It's not a rocket science - rather an opportunity to learn how others think about the topic and make the project thrive (as more people have more experience) and second to ensure formalities are met (e.g. version bumps, roughly code style, at least one review of code PRs, C89 compatibility, etc.).

@Hejsil Hejsil force-pushed the fix-double-padding branch from 73fcb94 to 142e203 Compare December 12, 2019 17:08
@Hejsil
Copy link
Contributor Author

Hejsil commented Dec 12, 2019

I have confirmed that this problem and this fix applies to the glfw_opengl3, x11_xft and x11 demo backends.

@Hejsil Hejsil force-pushed the fix-double-padding branch from 142e203 to ebe7599 Compare March 4, 2020 16:56
@Hejsil
Copy link
Contributor Author

Hejsil commented Mar 4, 2020

Here is a before and after from the gui I'm building with Nuklear

Before:
image

After:
image

@Hejsil
Copy link
Contributor Author

Hejsil commented Mar 5, 2020

Mind changed. I wouldn't mind being an organization member. Reviewing and merging smaller scale PR's would help this project along i imagine :)

@dumblob
Copy link
Member

dumblob commented Mar 6, 2020

Great news - invitation sent (if you didn't get any, just ping me). I'd like to encourage the "everybody is an admin, not just member" culture (so you're welcome to send admin invitations yourself at your sole discretion - I myself always try to look at their PR reviews before sending an invitation).

I read your comments on the current PRs and very much thank you for the work!

Hm, I though also we could make the statement from https://github.com/Immediate-Mode-UI a bit more specific in this repo (currently it says We review all PRs before merging. We require backwards compatibility of nuklear.h (unlike of backends/demos/examples), but we also like major version bumps.). Do you think a checklist in README.md of common things to look for when reviewing PRs would make sense? Maybe something like:

Changes to nuklear.h (i.e. src/ directory):

  • try to merge just good stuff (the measure here is code quality, not time before merging)
  • version bumps
  • code style (at least roughly)
  • at least one review of each PR from a different person
  • C89 compatibility
  • has to work with several different backends well enough
  • prefer copy over pointer for small structs (e.g. RGB color) for performance reasons
  • ...

Changes to other stuff in this repo (e.g. backends):

  • none of the above is a requirement
  • use of modern technology slightly preferred (e.g. C11 instead of C89)
  • merge even incomplete or slow or a bit messy stuff, but always comment on these deficiencies in the code directly (or make todo lists at the top of source files) - here the measure is the time (the sooner merged, the better)
  • ...

@Hejsil
Copy link
Contributor Author

Hejsil commented Mar 6, 2020

Yea, these seem like good points that should at least be stated somewhere easily available (the Readme is probably the best place). This information both helps reviewers look for the right things and contributes getting their stuff merged faster.

@dumblob
Copy link
Member

dumblob commented Mar 6, 2020

@Hejsil could you amend then the review "rules" with your ideas and put it to readme?

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.

LGTM and I'm happy to accept this 😉. Please update the version to the current master and let's merge it!

@Hejsil Hejsil force-pushed the fix-double-padding branch from ebe7599 to 14dd68e Compare March 6, 2020 16:30
@Hejsil Hejsil merged commit 82d85e9 into Immediate-Mode-UI:master Mar 6, 2020
@Hejsil Hejsil deleted the fix-double-padding branch March 6, 2020 17:07
@ghost ghost mentioned this pull request Mar 16, 2020
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.

3 participants