Added support for 9-slice/9-patch image for various widgets #69
Conversation
|
Thanks, I'll review in the next couple of days hopefully. Could you also bump version in |
…cremented version to 4.01.6.
Done. |
|
Hmm. I think newlines changed |
Hm, if this is the output of |
I use Win 10 with Git for Windows, and have autocrlf enabled. On my system, all nuklear files are using CRLF line endings, as is expected with autocrlf. Commited files should automatically have all line endings changed to LF, although I can see that this is not the case with nuklear.h, for some reason... It seems that the other edited/new source files have proper line endings (LF). nuklear.h is the only one with CRLF. EDIT: When opening the generated nuklear.h in Visual Studio, it reports that the file contains inconsistent line endings. I have confirmed that ALL files in /src/ use CRLF line endings, so something must be happening during the packing process. If I fix the inconsistent line endings, git converts CRLF to LF upon commiting as it should do. |
…thon 2.x on Windows and added setting to .gitattributes to ensure line endings are consistent amongst contributers
|
I have modified paq.bat, paq.sh and build.py to amend the issue caused by Python 2.x on Windows. build.py now outputs to a file instead of directly to stdout, which was causing Windows to mangle line endings. |
All issues and conflicts have been resolved and the PR should be ready to be merged. Would you like me to squash the commits before you merge? |
|
If you bump the minor version instead of patch version and squash the commits, then I approve of these changes 👍 |
Bumped the version to 4.02.0. Apparently squashing can be done by whoever merges the PR: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/about-pull-request-merges |
| @@ -74,7 +74,7 @@ def omit_includes(str, files): | |||
| return str | |||
|
|
|||
| def fix_comments(str): | |||
| return re.sub(r"//(.*)(\n|$)", "/* \\1 */\\2", str) | |||
| return re.sub(r"//(.*)(" + os.linesep + r"|$)", "/* \\1 */\\2", str) | |||
There was a problem hiding this comment.
This doesn't look correct to me (this shouldn't be dependent on the environment in which this script is being executed). I think this should actually be hard coded - i.e. something like
re.sub(r"//(.*)(\r\n|\r|\n|$)", "/* \\1 */\\2", str)There was a problem hiding this comment.
This will, for some bizzare reason, always give priority to \n. This results in each comment being replaced with \r /* comment */ \n
There was a problem hiding this comment.
Apparently I need to learn Python regexes better 😢. I think the issue is with the $ character, but I think the easiest thing would be to rewrite it to simply re.sub( "//([^\r\n]*)\r?\n?", "/* \\1 */\n", s ). Does this work with python 2.x on your system?
Also if we're already at it, we should discourage the use of str as variable label as it's a keyword in Python.
There was a problem hiding this comment.
The problem with that one is that it appends LF instead of the system-specific newline. I think it might be best to just completely remove the comment replacing feature, as // is fully supported in C99 and later, and in C90 if strict mode is not used.
There was a problem hiding this comment.
The problem with that one is that it appends LF instead of the system-specific newline.
That was my goal 😉.
I think it might be best to just completely remove the comment replacing feature, as
//is fully supported in C99 and later, and in C90 if strict mode is not used.
We're trying to be strictly C89 compatible for good reasons (and this presumably won't change during the coming few years). That's why we introduced the "replacing feature".
There was a problem hiding this comment.
But of course, I could write the replacement feature in POSIX shell and assume each Windows dev has git for windows installed allowing them to run paq.sh and remove paq.bat completely. But I'd prefer leaving it up to the build.py script.
There was a problem hiding this comment.
That was my goal 😉.
See my reply below as to why this isn't recommended.
But of course, I could write the replacement feature in POSIX shell and assume each Windows dev has git for windows installed allowing them to run paq.sh and remove paq.bat completely. But I'd prefer leaving it up to the build.py script.
I'm not sure if this would be a good assumption to make. Somehow we need to fix the regex so that it searches for the line-endings in the order that they are specified, instead of always giving precedence to \n.
| @@ -129,39 +135,42 @@ def fix_comments(str): | |||
|
|
|||
| # Print concatenated output | |||
| # ------------------------- | |||
| print("/*") | |||
| sys.stdout.write("/*" + os.linesep) | |||
There was a problem hiding this comment.
I have to admit I still can't grasp using os.linesep as this script should deterministically output stream of bytes independent from the platform it is being executed on.
I understand that we have to fight some corner cases of Python behavior (I also accept patches regarding Python 2.x behavior even if it's already after final EOL), but basically using os.linesep in this script in general doesn't seem like this fighting against Python. I don't run Windows, but I'm considering asking my good friend who runs Windows to take a look at this as this smells kind of fishy 😉.
Thoughts?
There was a problem hiding this comment.
Sorry, I have no idea on this one. Windows and Python oddities are not something i deal with often.
There was a problem hiding this comment.
The problem is with print on Windows with Python 2.x (I have 2.7). For some reason, it always uses LF instead of CRLF. It is silly that we have to work around this kind of thing, which is (apparently) resolved in Python 3.x, but too many programs still rely on 2.x. Would you be willing to make the script only work with Python 3.x? Most developers should have 3.x on their systems nowadays anyway, and it's simple to install if they don't have it.
There was a problem hiding this comment.
For some reason, it always uses LF instead of CRLF.
Hm, but that's exactly the behavior we require, don't we?
Would you be willing to make the script only work with Python 3.x?
Yeah, that's also an option I'm totally OK with.
There was a problem hiding this comment.
@Hejsil sounds plausible - what about just replacing .replace( '\r', '' ) on all opened files with wb (i.e. binary mode). That'd do, right?
There was a problem hiding this comment.
I think that could work. As long as this file is generated with one consistent line ending then I think git will be happy.
There was a problem hiding this comment.
I'm sorry if I wasn't clear enough, but where exactly do the multiple line-endings come from?
print()on Windows in Python 2.x (or at least in 2.7) appends \n to the end of the string you pass it. Git on Windows converts all line-endings to \r\n when you checkout, at least by default.
Hejsil explained it a bit clearer than I did, it seems.
what about just replacing .replace( '\r', '' ) on all opened files with wb (i.e. binary mode). That'd do, right?
I'm not sure we should start doing this kind of thing, but if it works, it works. I still feel that simply requiring that the script be run with Py3 would be a better decision.
There was a problem hiding this comment.
I do agree we can restrict users to Python 3, but I still like the explicit replace (with a comment explaining "why") because that makes it fully explicit.
There was a problem hiding this comment.
Maybe this should be a separate pull request? Then we could get this one merged ;)
| case NK_STYLE_ITEM_COLOR: | ||
| text.background = background->data.color; | ||
| nk_fill_rect(out, header, 0, style->tab.border_color); | ||
| nk_fill_rect(out, nk_shrink_rect(header, style->tab.border), |
There was a problem hiding this comment.
On some places in the patch long lines are wrapped and on some not 😉. I'll merge it anyway because nuklear.h historically didn't wrap lines much/at_all, but I'm just pointing it out as I find any inconsistency interesting.
There was a problem hiding this comment.
Do you have a preference as to how many characters long a line should be? I tried to adhere to the surrounding coding style as much as I could.
There was a problem hiding this comment.
I do (the good old 80 chars rule), but I'm convinced I shouldn't dictate it for this project. Adherence to the general coding style in nuklear.h is what counts. The only thing is, that I'd maybe slightly prefer if PRs themselves were consistent on their own 😉.
There was a problem hiding this comment.
It might help to write up a style guide for future contributors, if it's not too much work. Just something simple like "use x naming scheme, keep lines under y characters long, make sure to use asserts for z".
There was a problem hiding this comment.
Yeah, that makes sense - on the other hand as I wrote, it's not a requirement, just an observation which won't affect whether to merge or not. So we might mention that as "preferred", but not a requirement in https://github.com/Immediate-Mode-UI/Nuklear/#reviewers-guide 😉. Feel free to make a PR to formulate how you'd imagine that.
|
This looks like a derivative of this pull request: vurtun/nuklear#874 with Nuklear is in public domain and open-source and everything, but i'm pretty sure that removing author's name from work is illegal. If memory serves me well, in Russia it's a socially dangerous crime that can not be settled, no matter what you do, state will punish you for that. I don't know how it is in other countries and this isn't legal consultancy, but regardless, IMO it would be nice to credit original author somewhere. For example by rebasing this branch on @carloscm branch so his name remains in Git log. |
Did you look at any of my code before posting this? My PR takes a completely different approach from the one that you linked. carloscm's PR introduces a new draw command type, whereas mine makes use of the already existing image command type. In no way does this even remotely resemble a derivative work and, in fact, was written by myself without knowing about the existence of a similar approach already suggested by another user. I would greatly appreciate it if in future you verified your claim before accusing me of plagiarism. |
|
I actually did, this is why i noticed all those similarities, didn't i? Great minds most definitely think alike, but this looks like a good dozen of coincidences to me:
|
You are joking, right? It's literally just a copy-paste of the nk_image function declarations with |
|
@alekseyt thanks for pointing out there is some prior work. I don't think the screenshots tell much - actually rather the opposite. In both patches the API is built around the Nuklear naming of APIs - it's no surprise it looks similar if the authors want it to be merged in our upstream. Maybe we could ask @carloscm whether he wants to be mentioned as the originator of the idea (but not the code nor API). Until then I think it's perfectly OK to leave it as it is. @carloscm how do you feel about this? |
|
I've hear that russian vodka is great, we can clearly see it's effects work quickly. PS: Don't claim plagiarism just because you see similar or exact same function signatures, it doesn't mean the logic it implements is the same. |
|
@Goldenheaven please stay patient - there might always be something we're missing and any effort to point it out is always appreaciated. |
The idea of 9-patch/9-slice has been around for a long time. I modified my local version of nuklear because I needed this functionality. Neither myself nor carloscm are the originators of this idea; we simply both made PRs for a feature that is used in a lot of GUI libraries and is essential if you want to be able to reuse textures for controls that can be multiple sizes. |
|
@alekseyt I appreciate your concern, really, but in this case @Michael-Kelley is right. The concept of 9-patches has been around forever and given the very well defined nomenclature and API patterns of nuklear any reasonable implementation of them is going to look very similar from the API point of view. Internally my patch and @Michael-Kelley are different, as mentioned. For example look at the code that issues the draw commands in my version: vurtun/nuklear@e14a2f6#diff-a5933f839e9eddaa3f1bd273e0fb3025 And compare with this pull request: 505c0f9#diff-2e64ef485d707609ce7f7124b0e54d19 So I do not believe @Michael-Kelley copied anything from my pull request. It's just an obvious feature to have with a similar named API to fit with the rest of nuklear. @dumblob I don't claim any ownership on this pull request, or even any influence. @Michael-Kelley deserves the full and only credit. |
|
Thanks @carloscm for your explanation and sorry for not having enough time back then to discuss your PR in the old repository. We now have more reviewers, so you're welcome to contribute to this new repository 😉. I think the authorship question is hereby fully solved. (with "originator of the idea" I meant the idea to implement it also in Nuklear, not the general originator of 9-slice images 😉) |
|
@carloscm Thanks for clearing that up. |
|
Is there anything else left for this PR to get merged? |
|
Looked through the code and it looks good. With #144 merged we can remove whatever this PR was trying to do in the |
|
Any way I can help get this PR merged? |
|
It looks like 304 was merged. Does that mean this can be closed? 🥲 |
|
@RobLoach Yep 👍 |
I have added support for 9-slice (also called 9-patch) images for most of the widgets, at least where it made sense.
The following widgets now support nk_style_item_9slice for skinning:
9-slice images can be assigned to styles in much the same way as regular images:
nk_9slice slc = nk_sub9slice_ptr(tex, tex_width, tex_height, nk_rect(rgn_x, rgn_y, rgn_w, rgn_h)); ctx->style.button.normal = nk_style_item_9slice(slc, left_edge, top_edge, right_edge, bottom_edge)); /* Edges are measured from the edge of the image, so bottom_edge = 2 would mean that bottom_edge was 2 pixels from the bottom of the image */Please review this PR and let me know if there are any problems.
closes #68