Collapsing headers & pushID

[petrihakkinen]

In the doc it says "ID are uniquely scoped within windows, tree nodes, etc. so no conflict can happen" so I was under the impression that this also applies to collapsing headers, but it seems that this is not the case. Any reason why? I use a lot of collapsing headers and I just noticed many conflicting IDs.

[ocornut]

They don't because the api doesn't require you to "close" the scope (partly because there's no visible effect for it), so I can't pop an id at the "end" of a collapsing header.

Perhaps this should be clarified. (That specific function is also a prime candidate for 2.0 dismantling because of the horrible bool parameters)

[petrihakkinen]

I figured that out but is there anything other than sprinkling user code with PushID/PopID that we could do? It's a little ugly... Maybe something like, CollapsingHeader pushes the ID and the next CollapsingHeader/End etc. call pops it (don't know the exact list of functions that should pop but you get the idea)?

(yeah, I've shortened the API to CollapsingHeader(label, default_open) in my Lua wrapper)

[petrihakkinen]

To clarify, I'm suggesting that the next call to End(), CollapsingHeader() or such function that clearly ends the scope of the collapsing header would automatically pop the ID.

[Cthutu]

If you had a C++ object for a ImGui context (thinking 2.0 API), you could use scoped based pushing and popping such as:

ImGui::Context ctx;

{ ImGui::ScopedIds(ctx); ... ... }

The constructor of ScopedIds would push, and the destructor would pop by calling ctx.PushID and ctx.PopID.

On Fri, 15 Apr 2016 at 04:33 petrihakkinen notifications@github.com wrote:

To clarify, I'm suggesting that the next call to End(), CollapsingHeader() or such function that clearly ends the scope of the collapsing header would automatically pop the ID.

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/ocornut/imgui/issues/590#issuecomment-210359617

[ocornut]

@petrihakkinen I feel it may be a little too magic/inconsistent. I imagine it'd work for the basic case but if you start messing up with cursor position, indentation, columns, etc. the "next" CollapsingHeader may not be an obvious notion.

We could add a flag (once we switch to a variant taking flags) to automatically keep the ID pushed but then you'd still have to a TreePop() (or perhaps via a CollapsingHeaderPop() calling TreePop()). At this point it may not be worth the extra?

I think I see where you come from, I'm always using TreeNode() to embed sections of content and perhaps you are using CollapsingHeader) the way I'd use TreeNode() so the Push/Pop may be annoying fs your UI are generated from manually authored code.

Following refactoring those functions it could be that CollapsingHeader() is just a flag over TreeNode(), so you could call TreeNodeEx(... TreeNodeFlags_Framed) and it'd give the same visual with an expectation of doing a TreePop()?

[petrihakkinen]

Yep, I agree. Removing CollapsingHeader and adding support for tree nodes with frames sounds like the right way to go.

[ocornut]

Basic CollapsingHeader would become:

bool CollapsingHeader(const char* label, ImGuiTreeNodeFlags flags)
{
    return TreeNodeEx(label, flags | ImGuiTreeNodeFlags_Framed | ImGuiTreeNodeFlags_NoPushIdOnOpen | ImGuiTreeNodeFlags_NoIndentOnOpen | ImGuiTreeNodeFlags_NoAutoOpenOnLog);
}

Which also makes it obvious what you do if you want more custom behavior (aka call TreeNodeEx).

The existing parameter to the old CollapsingHeader() are terrible, probably among the top 3 worst choice of parameters in the original API and I think they were chosen solely to allow TreeNode() to call CollapsingHeader(). Thankfully:

• We may preserve the old API under the #ifndef IMGUI_DISABLE_OBSOLETE_FUNCTIONS block.
• I suspect few users ever used the str_id parameter or set display_frame=false. Likely the last parameter set to default_open=true was used a little more but if we decide to just break this API there will be very little damage and people can call CollapsingHeader(label, ImGuiTreeNodeFlags_DefaultOpen); or SetNextTreeNodeOpened(true, ImGuiSetCond_Once) which can be advertised a little more clearly as functional on CollapsingHeader.

[petrihakkinen]

How about just ImGuiTreeNodeFlags_CollapsingHeader = ImGuiTreeNodeFlags_Framed | ImGuiTreeNodeFlags_NoPushIdOnOpen | ImGuiTreeNodeFlags_NoIndentOnOpen | ImGuiTreeNodeFlags_NoAutoOpenOnLog ? This way you could remove the CollapsingHeader function.

I would also consider dropping ImGuiTreeNodeFlags_NoPushIdOnOpen and just make the collapsing header act like any other tree node (that require explicit pop call). Maybe.

[ocornut]

I would also consider dropping ImGuiTreeNodeFlags_NoPushIdOnOpen and just make the collapsing header act like any other tree node (that require explicit pop call). Maybe.

That would unfortunately break people code commonly and silently, so not a good idea :( Yes to the ImGuiTreeNodeFlags_CollapsingHeader shortcut.

[petrihakkinen]

Maybe I'm missing something but I think it would not break silently old code, because there would be no CollapsingHeader function anymore in the new API? (The deprecated old API would not push the id but once IMGUI_DISABLE_OBSOLETE_FUNCTIONS is defined that functionality would be gone).

[ocornut]

Oh I see. Then we can keep CollapsingHeader() as well, that's not hurting to keep it as a shortcut. I'm still iterating through those api to try to accommodate for various needs, will report. :)

[petrihakkinen]

Idea: could you consider moving the hardcoded ImGuiTreeNodeFlags_NoTreePushOnOpen flag to the function prototype, like this: bool ImGui::CollapsingHeader(const char* label, bool* p_open, ImGuiTreeNodeFlags flags = ImGuiTreeNodeFlags_NoTreePushOnOpen)

This way I could turn off the NoTreePushOnOpen flag cleanly without duplicating the code. (edited: oops, had the concept the wrong way around)

What do you think?

[ocornut]

I agree in principle but the idea here is to remove that function all-together in the API because the aim is to make it a natural fit in user-land (and temporarily a bigger chunk to include in user-land)

[petrihakkinen]

Ok, fair enough!

[ocornut]

Oh wait, no, this only pertain to the bool* version of it. The normal CollapsingHeader() version is of course staying.

Note that normal CollapsingHeader() is more or less only doing:

TreeNodeEx(label, ImGuiTreeNodeFlags_CollapsingHeader | ImGuiTreeNodeFlags_NoTreePushOnOpen);

So you can call that yourself without the undesirable flag?

[petrihakkinen]

Sure, I can :) I have a custom version of CollapsingHeader anyway, because I need the close button feature. So there is absolutely no need to change anything just for my sake.

It's just that from my point of view, a collapsing header is not that different from any other tree node, so I could argue that the API for both collapsing headers and "normal" tree nodes should follow the same begin/end pattern and thus push/pop the id automatically. It just feels backwards the way it is now. Feel free to disagree :)

[ocornut]

I'd agree in principle but it's been like that for 18 months and we can't break the API this way. However the new TreeNodeFlags + reorganization + comments makes it much more obvious that you can make your own one-liner variation if you want behavior like TreeNode().