search

dearimgui / dear_bindings

C header (and language binding metadata) generator for Dear ImGui
MIT License
221 stars 12 forks source link

Some enums are missing "is_flags_enum" field #42

Closed Ldash4 closed 9 months ago

Ldash4 commented 9 months ago

The following enums are missing the "is_flags_enum" field:

ImGuiTableBgTarget_
ImGuiDataType_
ImGuiDir_
ImGuiSortDirection_
ImGuiKey
ImGuiNavInput
ImGuiCol_
ImGuiStyleVar_
ImGuiMouseButton_
ImGuiMouseCursor_
ImGuiMouseSource
ImGuiCond_

It's easy to work around, but I could remove a few lines if they all just said false

ShironekoBen commented 9 months ago

Ah, that's a very interesting point. So in theory the current behavior is "correct" - is_flags_enum is assumed to default to false, so to avoid cluttering things up the metadata generator only actually emits it if it is true.

So you can safely assume that if it's missing then that means false... although now you've mentioned it I'm also suddenly aware that elsewhere we use "key not present" to mean "not applicable" or "don't know", so maybe that isn't such a great idea.

Just to double-check here before I fiddle with things, I'm right in saying that the underlying data (i.e. which enums are tagged as being flags) is correct, but from your perspective an explicit is_flags_enum: false is better than the implicit false we have right now? (having pondered it a bit in the wider scheme of things I'm very much inclined to agree with that, but I just wanted to make sure I'm understanding correctly!)

Ldash4 commented 9 months ago

Yeah you are right. But now that I think about it, I suppose that it is more of a broader style question. If it is decided to not have implicit defaults here, then it would make sense to do so everywhere, which obviously is a bigger change.

ShironekoBen commented 9 months ago

So having pondered this for a while, I've come to the conclusion that you're right and the minor readability cost of the JSON being more verbose is probably not worth the confusion implicit defaults could cause, especially in light of the "omitted elements means we don't know" behavior elsewhere.

So I've pushed a small update that makes all the applicable values get included all the time - hopefully this makes it clearer for everyone.

Thanks again for the feedback, and just shout if you spot any other problems with this.

Ldash4 commented 9 months ago

Great! I've been able to remove a bit of code now.