Make nk_combo Header Symbols Optional.

[zgcrowno]

The changes made ensure that when an nk_combo has no symbol associated with it, the symbol is neither drawn nor reserved any space in the header.

[dumblob]

@zgcrowno thanks for tracking it down. Before we'll dive into review, please check general review guidelines and make appropriate PR changes 😉.

Btw. have you tested this with few different backends?

[zgcrowno]

@dumblob, thanks for the review guidelines link; I hadn't seen that! I think I've updated the PR sufficiently, but please let me know if I've missed anything.

As for different backends, I've only tested it in OpenGL, but it seems to work just fine on that. My apologies, as I'm new to this sort of testing, but how do you generally go about testing across different render backends? Is there an especially efficient means of doing that? Thanks so much!

[Hejsil]

Have pulled down and tested this code on the x11 backend and it seems to mostly work. There is this one padding inconsistency where the left padding is smaller than the right one:

I understand that letters pop out of view when reaching the edge, but this is the closest I could have l to the egde without it disappearing.

There is also a warning we needs to fix:

warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
28275 |         int draw_button_symbol = sym != NK_SYMBOL_NONE;

Lastly, you forgot to run the pag script as instructed in the reviewers guide.

This looks good otherwise, and once these have been looked into we can merge 👍

[zgcrowno]

Thanks so much, @Hejsil. I'll see if I can't rectify these issues today.

[zgcrowno]

@Hejsil, I couldn't find anything about running a script in the reviewers guide, but I did see paq.bat and paq.sh scripts in the src folder. Is this what I should run?

[Hejsil]

@zgcrowno Aaah, you're right. It is not mentioned in the reviewers guide but in the Readme.md in src. We should probably link to it in the reviewers guide 👍

[zgcrowno]

Alright, @Hejsil, I ran the batch file, and I believe I've remedied the other issues as well. Just let me know if you require anything else. Thanks again!

[Hejsil]

Urrh not this again:

It seems like the newlines changed in nuklear.h. We've had this issue before but I guess we never got around to merging the PR that tried to fix it. I'm not really a windows development expert here so I'm not sure how we can easily fix this.

[zgcrowno]

Hmm, that's unfortunate. Feel free to let me know if I can help in any way.

[Hejsil]

@zgcrowno I tried to fix the issue in a VM #144. I'd be nice if you could test if this work.

[zgcrowno]

@Hejsil, the git diff is showing no difference!

[Hejsil]

@zgcrowno Alright, we got #144 merged. If you could rebase on master (and maybe squash your commits) then this PR should be good to merge 👍

[zgcrowno]

Sorry, @Hejsil, this weekend has been pretty busy for me. I'll try to get this taken care of today.

[zgcrowno]

I've actually realized that the issue I fixed for nk_combo still exists for nk_combo_begin_image, so I think I'll add that fix to this PR as well, since it's basically the same thing.

[zgcrowno]

Alright, @Hejsil, I added the optional header symbol behavior to nk_combo_begin_color, nk_combo_begin_image and nk_combo_begin_image_text. If everything looks alright to you, I can go ahead and squash my commits.

[Hejsil]

Just tested the code again, and everything looks good! I still see the newline issue, but maybe if you rebase and remove the nuklear.h, then regenerate it, it will go away 🤷

[zgcrowno]

I'm not sure why, but I can't seem to rebase because of unstaged changes to nuklear.h that I'm inexplicably unable to stash/reset/etc. Would you recommend I just revert the commit that introduced the newline issue, @Hejsil?

[Hejsil]

Idk, maybe:

git reset upstream/master  # This will give you all your changes from master unstaged
git checkout nuklear.h # Reset nuklear.h
cd src
paq.bat # Regenrate
cd ..
git add . # Add all changes
git commit -m "Message" # Commit
git push -f # Force push

This should be very similar to a rebase

[zgcrowno]

Alright, @Hejsil, I think we're all good! Thanks so much for your help.

[Hejsil]

Looks good! If @dumblob agrees then it'll get merged. Sorry about all the newline nonsense.

[dumblob]

@zgcrowno it's all good, I just had a bad day.