Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Style: add ImGuiStyleVar{TabBorderSize,TableAngledHeadersAngle} #7411

Closed
wants to merge 1 commit into from

Conversation

cfillion
Copy link
Contributor

These two are not accessible via the mid-frame safe Push/PopStyleVar API. TabBarBorderSize recently got its own StyleVar so it's strange that the older TabBorderSize doesn't have one. Temporarily overriding the new TableAngledHeadersAngle per-table seems useful.

The lack of StyleVar for some style fields has an impact in my custom bindings: Push/PopStyleVar is the only way users are allowed to edit styles mid-frame as direct memory access is not a thing in scripting languages and I limit it to "safe" APIs, obeying the comments in imgui.h:

// You may modify the ImGui::GetStyle() main instance during initialization and before NewFrame().
// During the frame, use ImGui::PushStyleVar(ImGuiStyleVar_XXXX)/PopStyleVar() to alter the main style values,
// and ImGui::PushStyleColor(ImGuiCol_XXX)/PopStyleColor() for colors.

Speaking of which, and considering that comment, the new Behaviors style fields for tooltips seem a bit out of place in ImGuiStyle with direct memory access mid-frame being the only way to edit them and a comment directly contradicting the previous one:

// (It is possible to modify those fields mid-frame if specific behavior need it, unlike e.g. configuration fields in ImGuiIO)

Yet there is no corresponding comment around ImGuiIO about mid-frame writes being no-go, and the demo happily directly writes to both ImGuiIO and ImGuiStyle mid-frame... What are the actual rules? Should I add a check in my bindings so that ImGuiIO values are only writable before a frame? Some of them are useful mid-frame, like changing InputTextEnterKeepActive per text field.

@ocornut
Copy link
Owner

ocornut commented Mar 26, 2024

Thanks for the PR, merged now.

Speaking of which, and considering that comment, the new Behaviors style fields for tooltips seem a bit out of place in ImGuiStyle with direct memory access mid-frame being the only way to edit them and a comment directly contradicting the previous one:

They were placed here about some back-and-forth because it is expected that user may want to alter them mid-frame.

There's two distinct aspects here:

  • the fact that values are likely to be modified mid-frame by users. Most values in ImGuiIO are not expecting to be often changed. it's likely that things would break if you e.g. toggled some settings mid-frame around specific locations, and we don't expect to support that.
  • the fact that editing values some may need to go via an API. the comment for ImGuiStyle was intended to keep the possibility open that a future style system will mark some data as dirty when modifying colors, and therefore direct edits would be discouraged. There's zero enforcement of that currently, and in reality many people have been doing this, so if we start enforcing that constraint it'll likely lead to some breakage or some careful design on our end.

Yet there is no corresponding comment around ImGuiIO about mid-frame writes being no-go
[...] and the demo happily directly writes to both ImGuiIO and ImGuiStyle mid-frame...

There's no hard rule but most of the time it doesn't seem to make sense and actual support may not be guaranteed.
AFAIK the demo only meaningfully writes to ImGuiIO in the Configuration panels and those paths appears to work.
I don't think it's worth enforcing hard restriction, but if people come with a legit case that doesn't work we can alter behavior or clarify them. (Slightly amended a comment with 5c5ae80)

Some of them are useful mid-frame, like changing InputTextEnterKeepActive per text field.

What else? it hadn't occurred to me that it would make sense to alter that flag, the general idea was that a whole application may want to be consistent here. In which situation are you altering it?

ocornut pushed a commit that referenced this pull request Mar 26, 2024
@ocornut
Copy link
Owner

ocornut commented Mar 26, 2024

I made a small amends with 5c5ae80. Other than that, I don't really see any clear action to do but open to discuss it if you have specific suggestion. In principle I agree it may seem not-defined enough but it doesn't seem overly important. Let me know what you think!

@ocornut ocornut closed this Mar 26, 2024
pull bot pushed a commit to TeamREPENTOGON/imgui that referenced this pull request Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants