ImGuiMod_Shortcut is not "portable"

[oprypin]

Version/Branch of Dear ImGui:

Version: 1.89.1
Branch: master

My Issue/Question:

See here:

imgui/imgui.h

Lines 1456 to 1460 in a8df192

	#if defined(__APPLE__)
	ImGuiMod_Shortcut = ImGuiMod_Super,
	#else
	ImGuiMod_Shortcut = ImGuiMod_Ctrl,
	#endif

In its header file, as part of the core API, imgui now has a value that depends on the system that it's being compiled on.

---

The first issue I see with this is that this might not be representable in bindings to other languages. Or if it is, the binding generators can be not smart enough to discern this and instead just paste the value of the system on which the generator happens to be running.

cimgui already straight up defines it as the non-Mac value, presumably because that's where it was generated.
https://github.com/cimgui/cimgui/blob/68483775b3d7594eb14d296ec002ac11683882c9/cimgui.h#L643
Maybe this is an oversight that can be fixed, so cc @sonoro1234. But still there's no good way to expose it in the JSON info files of cimgui.

Based on this, in crystal-imgui I intend to also ignore the Mac value for the foreseeable future. Though I could manually code the conditional into my generator.

---

The 2nd concern again comes from the fact that this is in a header file, so even in C++, the value gets baked into the destination executable, not into imgui (built as a shared library). Or well, it probably also gets baked into imgui, which is even less good.

---

Either way, because I believe this is the first ever occurrence of a platform-specific API value in a header, maybe it warrants some discussion or reconsideration.

Perhaps turning ImGuiMod_Shortcut into its own unique value (but then handling the difference on .cpp side) would be the approach that's less prone to cause issues down the line. Although... that's not possible to apply to equality checks against the value :/

[sonoro1234]

Perhaps turning ImGuiMod_Shortcut into its own unique value (but then handling the difference on .cpp side) would be the approach that's less prone to cause issues down the line.

1+

[sonoro1234]

But still there's no good way to expose it in the JSON info files of cimgui.

{
                    "name": "ImGuiMod_Shortcut",
                    "value": "ImGuiMod_Super",
                    "conditionals": [
                        {
                            "condition": "if",
                            "expression": "defined(__APPLE__)"
                        }
                    ]
                },
                {
                    "name": "ImGuiMod_Shortcut",
                    "value": "ImGuiMod_Ctrl",
                    "conditionals": [
                        {
                            "condition": "ifnot",
                            "expression": "defined(__APPLE__)"
                        }
                    ]
                },

It seems that dear_bindings is able to do it. The main reason is that does preprocessing itself with lex and yacc while cimgui takes the preprocessed output. That make things extraordinary more complex and prone to failures but is the way to go if you want to handle things like that. (That way still has to prove some things as imgui_internal, implot, etc parsing).
Initial cimgui_auto took less than a month to develop while dear_bindings seemed to take 18 months!!
And another question would be: Is it easy to use for a binding generator? (There are features in cimgui that seem too complicated for some binding generators so that not everybody use them)

The way to go for cimgui would be to make generation and compilation in the same machine. That is the same situation as with freetype, wchar32, obsolete functions or other preprocesor options (even options in imconfig.h)

[sonoro1234]

Just found https://github.com/ocornut/imgui/blob/master/imgui.h#L1928 where ConfigMacOSXBehaviors seemed to be the previous way of Dear ImGui handling MacOS differences.

[ocornut]

Perhaps turning ImGuiMod_Shortcut into its own unique value (but then handling the difference on .cpp side) would be the approach that's less prone to cause issues down the line.

I will look into the possibility of making that change. I am not entirely sure it is easily .
I agree it can create complication for bindings users and generator so if we can simplify it it'd be better.

ConfigMacOSXBehaviors seemed to be the previous way of Dear ImGui handling MacOS differences.

Yes. This specific occurrence was more difficult to do at runtime. I agree it is inconsistent to use the define it was just much simpler. I realized that we almost never take advantage of the dynamic nature ConfigMacOSXBehaviors (for local or automated testing) so I thought I could take that shortcut.

[ocornut]

I experimented with various approach, and decided toward implementing a redirect without storage:

• Committed 1a497c2
• It seemed simpler and less error-prone (in term of e.g. comparing mods flags).
• io.KeyMods (which is technically commented as "internal") will never contains ImGuiMod_Shortcut. So code wishing to do a single check of io.KeyMods and support ImGuiMod_Shortcut would need to call ConvertShortcutMod(). Because ImGuiMod_Shortcut is new there's likely no regression.

The other approach, which I have also implemented and tried is to store the alias, it felt slightly more complicated and the only value of doing it was to make it easier to expose e.g. io.KeyMods with the 5-bits, but that can break some code. Perhaps exposing two variants of io.KeyMods with/without alias may be useful but now the first approach seems simpler.

(Also committed cc3a220 which is internal refactor only, designed to make it easy to switch between both approach)

Let me know if it works!

EDIT: Interestingly if we decide to store a variant of KeyMods with ImGuiMod_Shortcut inside, we can actually still do it with first approach anyway.