TreeNodes with context menus

[pdoane]

I'm curious if anyone has a better pattern for attaching context menus to tree nodes. The context menu needs a unique id that works correctly when the TreeNode is opened, but it needs an extra push/pop when closed:

bool opened = ImGui::TreeNodeEx(label, flags);
if (!opened)
    ImGui::PushID(label);

if (ImGui::BeginPopupContextItem("Context Menu"))
{
    ...
}

if (opened)
{
    ....
    ImGui::TreePop();
}
else
    ImGui::PopID();

If TreeNode always pushed an ID and TreePop was always required, I think the code could be simplified like this, which is also more similar to how Begin/End work with windows.

if (ImGui::TreeNodeEx(label, flags))
{
    ....
}
if (ImGui::BeginPopupContextItem("Context Menu"))
{
    ...
}
ImGui::TreePop();

[ocornut]

This is a good question.
I think maybe it doesn't matter if the popup uses the same id when the treenode is open or closed, since you can't activate the treenode while popup is active?

So regardless of the tree node being open or not you could just do that perhaps?

bool opened = ImGui::TreeNodeEx(label, flags);
ImGui::PushID(label);
if (ImGui::BeginPopupContextItem("Context Menu"))
{
}
ImGui::PopID();

Note that you can also enclose the treenode between the PushID/PopID block, it wouldn't matter and you may already be doing that if you have items alongside your tree node.

As a convenience, I think we need to be able to pass actual ID directly to the Popup functions or have more convenient way of passing ID. In case ideally you would want to just want to avoid the PushID/PopID alltogether and include in the same BeginPopup call.

If OpenPopup/BeginPopup also took explicite ImGuiID it would mean you could manage the scope of that ID yourself, which is useful for various cases (see #331).
And maybe ImGui::GetId could be modified to chain multiple id components together, regardless of them being strings or pointers or indices.

If TreeNode always pushed an ID and TreePop was always required, I think the code could be simplified like this, which is also more similar to how Begin/End work with windows.

That would be a high-breaking change, not acceptable.
Note the ImGuiTreeNodeFlags_NoTreePushOnOpen to avoid doing a TreePush (= PushID() followed by Indent()).

Actually Begin/End is pretty much the inconsistent exception there, not TreeNode. I regret this but it's not really something we can easily change without breaking code (tho not impossible, since typically codebase have less Begin than TreeNode). I often think I would want to make it illegal to call End() if begin returned false. It would only be an annoyance for adding quick logging/debugging code, since user would be required to add a if block.

[pdoane]

Your suggestion looks like the right way to go for ease of use. The id stack is one element bigger than it needs to be but that isn't a problem.

FWIW, I would prefer API breaking changes periodically when that leads to a better and more consistent design.

[ocornut]

Cool. I'm still keeping note of the idea noted here I think they are valid and I would like to implement them at some point.

I am doing API breaking changes (they are usually well documented) but some are more acceptable than others. Making TreePop() required would break a lot of code in many places so that's unlikely.

[ebkgne]

Hey @ocornut , sorry for this but I really dont understand how popup work with treenodes, Im trying to add a simple right click for each node on a hierarchy. I couldnt find any explanation, did I miss something ? anyway here is the most basic snippet I tried every order , if somebody can point me in the right direction in a simple way i'd be very gratefull. thx :)

at best I get a popup n1 when I right click node1 but I get n1 n2 in node2 popup

bool node1 = ImGui::TreeNode("node1");

if (node1) {
    bool node2 = ImGui::TreeNode("node2");
    if (node2) { }
    if (BeginPopupContextItem()) {
        Text("n2");
        EndPopup();
    }
    if (node2) { TreePop(); }
}

if (BeginPopupContextItem()) {
    Text("n1");
    EndPopup();
}

if (node1) { TreePop(); }

[ocornut]

The BeginPopupContextItem() call needs to come right after the TreeNode() call.
Otherwise whatever is eg in your “if (node2) { }” block is going to interfere by submitting more items.

The way your code is structured means that Node 2 effectively has 2 contexts menu items.

[ebkgne]

ok thank you for your fast answer.. I guess I was right to feel sorry I though I had tryed this...

I spent lot of time close but not good enough before finding this post and apparently didnt try or understand correctly so here is a copy paste full exemple in case anybody like me ^^

bool node1 = ImGui::TreeNode("node1");
if (BeginPopupContextItem()) {
    Text("n1");
    EndPopup();
}
if (node1) {
    bool node2 = ImGui::TreeNode("node2");
    if (BeginPopupContextItem()) {
        Text("n2");
        EndPopup();
    }
    if (node2) {  }
    if (node2) {TreePop();}
}
if (node1) { TreePop(); }

[ocornut]

It’s a frequent mistake. Because dear imgui use so many stacks its easy to visualize this in that manner. But we as we don’t really store any item data aside from the last item, queries on items usually come right after the item.