Add nk_col_pick and nk_col_picker

[aganm]

All widget functions that have a color param take nk_color, except for nk_color_picker/pick that take nk_colorf instead.

This adds nk_col_pick and nk_col_picker wrappers which take a nk_color param and does the conversion to nk_colorf.

[RobLoach]

Seems appropriate.

[RobLoach]

It's unfortunate we couldn't have the functions named by what color struct they take...

nk_colorf_picker
nk_colorf_pick
nk_color_picker
nk_color_pick

But I can understand keeping backward compatibility.

[dumblob]

Should we also mark the nk_color_picker/pick functions as deprecated and add to comment a very approximate estimation when they'll be removed (e.g. will be removed any time after 2023-12-14 i.e. in about 2 years from now)?

There is e.g. NK_UNUSED and alike so we could introduce NK_DEPRECATED which would make it clear it's not advised to be used for new code (it should ideally produce a warning during compilation also - at least in the biggest compilers like clang & gcc & msvc). Together with the estimated time of removal in the comment (and thus in the doc) this should prepare a good ground for future.

Thoughts @Hejsil and others?

[Hejsil]

Instead of a timeframe, maybe remove it at next major version bump? So the comment would be will be removed in v6.0.0 or something like that.

[RobLoach]

I think having both is fine, but would be nice to flip the names to match what argument they return. nk_colorf_picker picks a nk_colorf, while nk_color_picker picks a nk_color

NK_API struct nk_colorf nk_colorf_picker(struct nk_context*, struct nk_colorf, enum nk_color_format);
NK_API nk_bool nk_colorf_pick(struct nk_context*, struct nk_colorf*, enum nk_color_format);
NK_API struct nk_color nk_color_picker(struct nk_context*, struct nk_color, enum nk_color_format);
NK_API nk_bool nk_color_pick(struct nk_context*, struct nk_color*, enum nk_color_format);

To make the API switch, they would just add an f to nk_color_picker.

[dumblob]

Instead of a timeframe, maybe remove it at next major version bump? So the comment would be will be removed in v6.0.0 or something like that.

Yep, that's easier. Would you also like to see NK_DEPRECATED or just leave it up to the vigility of programmers to follow changes to the doc without affecting their compilation pipelines?

I think having both is fine,

Forever? I.e. even in new major versions you'd prefer to have both?

but would be nice to flip the names to match what argument they return.

Yep, in the next major version, this could be the default recommended way to name the API.

[Hejsil]

Instead of a timeframe, maybe remove it at next major version bump? So the comment would be will be removed in v6.0.0 or something like that.

Yep, that's easier. Would you also like to see NK_DEPRECATED or just leave it up to the vigility of programmers to follow changes to the doc without affecting their compilation pipelines?

Hmm good question. I'm not sure how -Werror effects deprecation warnings. We don't wonna break people who use -Werror but it would be nice if people who don't got a warning about the deprecation before the functions removal.

[dumblob]

Hmm good question. I'm not sure how -Werror effects deprecation warnings. We don't wonna break people who use -Werror but it would be nice if people who don't got a warning about the deprecation before the functions removal.

I wouldn't try to make it a compiler warning but rather an ordinary compiler message. Didn't look up any side effects but I'd first investigate #pragma message "my compile-time message\n".

[RobLoach]

I would recommend against even a #pragma message , as it could get quite annoying for builds that are not affected by the deprecated code.

How would it affect them? I mean, old code won't update nuklear.h to this newer version which would have the #pragma. Or what exactly did you have in mind?

I don't think #pragma message would affect any builds in any way. It'll merely add some more messages to the build output.

[RobLoach]

This seems fine to me. While there could be some API improvements in terms of naming, I'd rather not break backward compatibility for it.

[RobLoach]

@aganm @dumblob @Hejsil Is this good to merge? While there are some good suggestions for breaking changes, I think this is the best intermediate change that won't break anything. Consider the breaking/deprecating changes in a future issue.

[aganm]

@RobLoach It's good to merge.

[dumblob]

I would recommend against even a #pragma message , as it could get quite annoying for builds that are not affected by the deprecated code.

What do you mean? Builds not affected by the deprecated code wouldn't produce such message, would they?

How would it affect them? I mean, old code won't update nuklear.h to this newer version which would have the #pragma. Or what exactly did you have in mind?

I see 3 cases:

1. I'm already using Nuklear (i.e. its previous/older version). But I don't want to update Nuklear at all and thus I'm totally unaffected by anything.
2. I'm already using Nuklear (i.e. its previous/older version). And I do want to update Nuklear (because there are some bug fixes, new features, etc.). Then I certainly welcome that Nuklear tells me with 0 effort (i.e. during build) that I should also migrate my code to new API without breaking the build itself.
3. I'm starting a new app with Nuklear. Thus I won't use any deprecated API and use only the new one. Therefore I'll not see any messages about deprecated APIs during build.

Does this make sense?

I don't think #pragma message would affect any builds in any way. It'll merely add some more messages to the build output.

That's the point - it shouldn't affect the build itself, but the programmer (i.e. the output of the build which is the only communication channel - apart from exit code - which the compiler has and thus it's guaranteed the programmer will want to read the output).

[RobLoach]

Builds not affected by the deprecated code wouldn't produce such message, would they?

I thought #pragma message just displayed the message whether or not they are using the old methods. I may not be quite understanding how it would be used though. I could be wrong on that though. I've seen @deprecated be used in code comments before.

[dumblob]

@RobLoach perhaps the function would be a macro...

@deprecated is fine - but it's only for documentation (and not for build), so not enough on its own.

[RobLoach]

Having it as a macro is a great idea.

[RobLoach]

My only worry here is the naming...

nk_color_picker
nk_color_pick
nk_col_picker
nk_col_pick

None of these are really descriptive. col doesn't quite help.

[Copilot]

Pull request overview

This PR adds nk_col_pick and nk_col_picker API wrappers so callers can use struct nk_color (byte RGBA) with the existing color picker, which currently operates on struct nk_colorf (float RGBA).

Changes:

• Add nk_col_picker(ctx, nk_color, fmt) wrapper that converts nk_color ⇄ nk_colorf around nk_color_picker.
• Add nk_col_pick(ctx, nk_color*, fmt) wrapper that converts nk_color ⇄ nk_colorf around nk_color_pick.
• Expose the new APIs in both the split header (src/nuklear.h) and the single-header amalgamation (nuklear.h), including the inline implementation in nuklear.h.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/nuklear_color_picker.c	Implements the new nk_color-based wrapper functions for split-compilation builds.
src/nuklear.h	Adds public declarations for nk_col_pick / nk_col_picker.
nuklear.h	Adds public declarations and single-header implementations for nk_col_pick / nk_col_picker.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

nk_col_pick dereferences color before validating it, and it always writes back *color even when nk_color_pick returns false. This can crash on NULL input and can also mutate *color despite returning 0 (e.g., due to float<->byte round-trip). Add NK_ASSERT(color) / an early if (!ctx || !color) return 0;, and only assign back to *color when ret is true.

Suggested change

	struct nk_colorf cf = nk_color_cf(*color);
	ret = nk_color_pick(ctx, &cf, fmt);
	*color = nk_rgba_cf(cf);
	struct nk_colorf cf;
	NK_ASSERT(ctx);
	NK_ASSERT(color);
	if (!ctx || !color)
	return 0;
	cf = nk_color_cf(*color);
	ret = nk_color_pick(ctx, &cf, fmt);
	if (ret)
	*color = nk_rgba_cf(cf);

[Copilot]

nk_col_pick dereferences color without a NULL check and always writes back to *color even when nk_color_pick returns false. This can crash on NULL input and can also modify the output while returning 0 (float<->byte round-trip). Add an early if (!ctx || !color) return 0; (plus NK_ASSERT(color)), and only assign back when ret is true.