Collapsing header optional close button

[petrihakkinen]

Would it be possible to have an optional close button on collapsing headers? I'm using a lot of collapsing headers and they would be really useful for properties panels that are needed infrequently.

Alternatively could you expose CloseWindowButton in the public API so that I can place a close button on top of the header (not sure though if the layout would allow this)?

[ocornut]

Seems like a good idea.
Also linking to related issues:
#581
#590
#579
#324
#190

I would say we can head toward something like
bool CollapsingHeaderEx(const char* label, bool* p_open = NULL, int flags = 0)
bool TreeNodeEx(const char* label, int flags = 0)

With all TreeNode variations, etc. And CollapsingHeaderEx() can have format string variations. Bit of a combinatory explosion. It may be more acceptable to replace CollapsingHeader which is weirder and less frequently used than TreeNode (but we can't replace/breakTreeNode).

I used int flags here but they should be typed enums, the question being: can we use the same set of enums (preferable) and what should the name be.

This also connects to on #579 - we may want to clarify how the SetxxOpened functions can apply to collapsing headers as well.

So all of those things are loosely connected and it's a case where we should aim to solve all of them together in a nice new api. Just throwing all those info out here so I can think about it.

Immediate minor step:

• Turn CloseWindowButton() into an internal helper e.g. CloseButton() declared in imgui_internal.h that takes position as parameter?

[ocornut]

I added a first draft helper CloseButton() that we will be able to use once all CollapsingHeader() code is refactored into something sane.

As a hacky test adding that line after the ItemAdd() block in CollapsingHeader()

    if (display_frame)
    {
        CloseButton(id+1, interact_bb.GetTR()+ImVec2(-g.Style.FramePadding.x - g.FontSize*0.5f, +g.Style.FramePadding.y + g.FontSize*0.5f), g.FontSize*0.5f);
    }

So that's the easy part, the hard part now is to refactor TreeNode/CollapsingHeader to take account of all the things above. It's mostly an API design issue. But closing so many bug # on the same time makes it an exciting prospect :)

[petrihakkinen]

Excellent! Can't wait to start using this new feature. :-)

[ocornut]

I've got that working locally with a new TreeNodeEx/CollapsingHeader API but not super api with
a) either adding an additional bool* in a CollapsingHeader API or passing the value through return flags that I am trying to remove (in favor of using functions in the same nature as IsItemSelected()).
b) the fact that is hard-coded.
So instead I will look into making this a more generic thing so that you can eventually add your own button after the fact. Needs some subtle tweaking of the hovering system to allow that.

[ocornut]

This is now committed after much back-and-forth on my local branch.

I have refactored the code so that CollapsingHeader() is now a trivial function calling into TreeNodeBehavior().

I have also designed the code so that the CloseButton functionality isn't hard-coded in the lower-level TreeNodeBehavior() function, but rather in a way that will be easier to later extend into user-space function.

Right now this is the new CollapsingHeader() function that has this feature:

bool ImGui::CollapsingHeader(const char* label, bool* p_open, ImGuiTreeNodeFlags flags)
{
    ImGuiWindow* window = GetCurrentWindow();
    if (window->SkipItems)
        return false;

    if (p_open && !*p_open)
        return false;

    ImGuiID id = window->GetID(label);
    bool opened = TreeNodeBehavior(id, flags | ImGuiTreeNodeFlags_CollapsingHeader | ImGuiTreeNodeFlags_NoTreePushOnOpen | (p_open ? ImGuiTreeNodeFlags_AllowOverlapMode : 0), label);
    if (p_open)
    {
        // Create a small overlapping close button // FIXME: We can evolve this into user accessible helpers to add extra buttons on title bars, headers, etc.
        ImGuiState& g = *GImGui;
        float button_sz = g.FontSize * 0.5f;
        if (CloseButton(window->GetID((void*)(intptr_t)(id+1)), ImVec2(window->DC.LastItemRect.Max.x - g.Style.FramePadding.x - button_sz, window->DC.LastItemRect.Min.y + g.Style.FramePadding.y + button_sz), button_sz))
            *p_open = false;
    }

    return opened;
}

There's some extra test to allow for NULL p_open which may not be necessary.

I am actually considering REMOVING that new function (disclaimer: reserving myself the right to break more stuff in-between tagged releases :).

Here's the reasoning.

• The end-goal is that all the ugly CloseButton() parameters mess are turned into a nifty helper to easily add extra overlapping buttons over a title bar, header, etc.
• When that happens (later down the line) perhaps this function wouldn't be needed anymore.
• It is ugly but short enough to copy into a user-side helper using imgui_internal.h in the meanwhile.

So the end-goal is that you should be able to write:

bool open = ImGui::CollapsingHeader(label, ImGuiTreeNodeFlags_AllowOverlapMode);
if (ImGui::SubCloseButton())
{
    // process deletion
}

Hypothetical API here, the layout of said mini/sub buttons may want more parameters, etc. But that's the idea and at that point the extra CollapsingHeader() function isn't really desirable anymore.

@petrihakkinen Does that make sense? If you agree maybe I'll just remove that function and you can include it in your codebase (in a file that is including imgui_internal.h) and perhaps in a few months we'll have the better helpers.

PS: The ID for the button is completely arbitrary, doesn't matter what it is as long as it is stable. Here I am hashing the ID of the parent header.

[ocornut]

(
Extra info
The ImGuiTreeNodeFlags_AllowOverlapMode flag I have added is introducing at lower-level a pattern I've been thinking of for a while, which will come in very useful for many uses outside of just tree nodes.

Splitter #319 coming to mind. If you want to use empty space as a splitter but still allow objects to overlap the splitter bounding box, previously the splitter had to have a "hovered" color with zero alpha, otherwise the user would see both overlapping items visibility hovered. Now, using the low-level flag ImGuiButtonFlags_AllowOverlapMode they can have a hovered color. Howering a button that's overlapping the splitter won't show both hovered.

I know this is all cryptic, leaving that out for the one person of a half who may be concerned!
)

[petrihakkinen]

Thank you! I will test this early next week, hopefully tomorrow, and get back to you. I suspect I may have to include a custom version of this code anyway in my codebase, because I probably want to remove ImGuiTreeNodeFlags_NoTreePushOnOpen (to fix the id collision issue in Shinobi I mentioned before).

[petrihakkinen]

btw. I'm toying with the idea of replacing "p_opened" arg with "ImGuiWindowFlags_CloseButton" and "ImGuiTreeNodeFlags_CloseButton", and adding a new API for retrieving the state of the close button: bool CloseButtonPressed(). This way the function overloads can be eliminated and the API becomes cleaner, I think.

[ocornut]

This is exactly what I was trying to avoid (with great effort), because the idea is that we want to be able down the line to add user defined custom buttons (pin,minimize,whatnot) and not just a close button

So the whole shenanigan with that change in hovering is to allow those extra widgets to be submitted after and without needing to be part of the core function.

So that entire ugly block

    if (p_open)
    {
        // Create a small overlapping close button // FIXME: We can evolve this into user accessible helpers to add extra buttons on title bars, headers, etc.
        ImGuiState& g = *GImGui;
        float button_sz = g.FontSize * 0.5f;
        if (CloseButton(window->GetID((void*)(intptr_t)(id+1)), ImVec2(window->DC.LastItemRect.Max.x - g.Style.FramePadding.x - button_sz, window->DC.LastItemRect.Min.y + g.Style.FramePadding.y + button_sz), button_sz))
            *p_open = false;
    }

Would eventually be a sort of one-liner CloseButton() function that can be called AFTER CollapsingHeader() and it would layout a button within the host/parent widget.

As per my hypothetical example above:

bool open = ImGui::CollapsingHeader(label, ImGuiTreeNodeFlags_AllowOverlapMode);
if (ImGui::SubCloseButton())
{
    // process deletion
}

Which isn't more complex/longer than what you are describing BUT way more flexible.

[petrihakkinen]

Yes, I got that and I agree. In the meanwhile, before the new API is here, I'm doing this for myself to make the API a little bit easier to use. Those p_opened args are a bit of a problem because Lua doesn't have pointers.

[ocornut]

Ouch yes, I'm totally not designing this for LUA.

Perhaps your wrapper could just return two values?

[petrihakkinen]

That's what I've been doing but it's messy because the 1st and 2nd return values are usually a local and a global:

window_open = true -- global variable
...
if window_open then
    local not_collapsed, open = imgui_begin("Foo", window_open) 
    -- note:
    -- 1) "not_collapsed" argh :) 
    -- 2) the second arg "window_open" just signifies "add close button" here

    window_open = open  -- store local 'open' in a global variable

    if not_collapsed then
        ...
    end
end

vs.

if window_open then
    if imgui_begin("Foo", imgui.close_button) then
        ...
    end
    if imgui_window_closed() then window_open = false end
end

[ocornut]

During my research for evolving treenodes I had a ImGuiResFlags enum which was an attempt to unify all the possible flags that could returned by widgets (e.g. Hovered, Active, Clicked, Open, etc.) so that would make that sort of thing easier. It turned out that this would add some complications and extra expectations so it isn't a trivial change but we could head there.

Right now we have IsItemHovered IsItemActive IsItemClicked IsItemVisible
In your we could add IsItemOpen() - explicitely functional on Begin as well as TreeNode, etc. if we manage to store this state reliably. Could we may need to clarify some of the vocabulary, because "Open" for a TreeNode correspond to "Collapsed" for a window, and the Open of a window is a user-facing flag to direct code flow.

[petrihakkinen]

For what its worth, I think returning a bool for the common case ("collapsed") and accessing (rarely used?) extra data like "hovered" and "visible" etc. using separate functions makes sense. I also really like the pattern "if(ImGui::Begin()) ..."

btw. I'm happy with the new close button on collapsing header feature -- big thank you! Should this issue be closed now, or do you want it to stay open for discussions?

[petrihakkinen]

And to clarify, +1 to "ImGui::SubCloseButton()" :)

[ocornut]

And to clarify, +1 to "ImGui::SubCloseButton()" :)

Will probably close this topic once SubCloseButton or equivalent is actually added. I will however probably also remove the bool* variation of CollapsingHeader so it doesn't stay in master too long, and you can copy that function locally.

[petrihakkinen]

This is probably hard to implement (?), but it would be nice if mouse hoving over the close button would not cause the collapsing header to be drawn as "unhovered". The current behavior makes it feel like the close button is a separate widget and causes the header to flicker between normal and highlighted states when mouse travels over the close button. For example, tabs in Sublime Text work like this: if you hover over the close button the tab is still highlighted.

[ocornut]

In ButtonBehavior() you can try to change:

    // AllowOverlap mode (rarely used) requires previous frame HoveredId to be null or to match. This allows using patterns where a later submitted widget overlaps a previous one.
    if (hovered && (flags & ImGuiButtonFlags_AllowOverlapMode) && (g.HoveredIdPreviousFrame != id && g.HoveredIdPreviousFrame != 0))
        hovered = pressed = held = false;

to

    // AllowOverlap mode (rarely used) requires previous frame HoveredId to be null or to match. This allows using patterns where a later submitted widget overlaps a previous one.
    if (hovered && (flags & ImGuiButtonFlags_AllowOverlapMode) && (g.HoveredIdPreviousFrame != id && g.HoveredIdPreviousFrame != 0))
        pressed = held = false;

It may work. It may has to be an option / separated ImGuiButtonFlags flags because I think both behaviors may be useful.

[petrihakkinen]

More motivation for supporting close buttons on collapsing headers natively (as opposed to moving it to user land):

The title text should be clipped so that it does not overlap with close button.

[ocornut]

Yeah I was thinking that the other day. Bit tricky.
We could be all fancy pantsy and clip the text vertices after the fact. If they had a way to identify them. It would be a rather helpful low-level trick to use in various places and more sane than the previous idea (but not as simple).

That would rely on ordering, however that SubButton layer idea would already be relying on ordering anyway so maybe not a stupid idea. If we are going to store a "host" bounding box for those inner layout we may as well store vertices ranges for the label and then after-the-fact clipping is made easy.

I am really hoping to avoid hard-coding those buttons because it would add so much more potential to support those patterns.

[petrihakkinen]

Sounds complicated, but if you can make it work, it would be nice, yes.

[ocornut]

Sounds complicated, but if you can make it work, it would be nice

That sounds like the story of everything so far!

[petrihakkinen]

It's a great story, though!

[ocornut]

@petrihakkinen Any news on this?

but it would be nice if mouse hoving over the close button would not cause the collapsing header to be drawn as "unhovered".

#600 (comment)

As per your other request #684 I am wondering how to treat this.
ImGuiTreeNodeFlags_AllowOverlapMode actually does something more than just calling SetItemAllowOverlap, the button also behave in a slightly different way that allows disabling the highlight (which in this case you may not care about in your Selectable case of #684).

[petrihakkinen]

I guess it depends if you want to "just make it work" (simple) or do you want to make it work beautifully in all cases (hard). I noticed that imgui is somewhat hover happy, meaning that most of the controls have quite not so subtle hovering indicators, which can be nice, but also make these kind of beauty issues noticeable. Maybe one way could be to ease on the hovering effects... I dunno.

(speaking of hovering, I think there are some inconsistencies like some widgets not having hovering highlights while other similar widgets do.. just a mental note to myself at this stage I'll report back when I get to the office)

I also noticed today that the hovering behavior with the small button over selectable felt somehow off, but I could not pin-point what the issue actually was (I did not have much time to investigate it further). Maybe it felt strange that the selectable highlight disappears when clicking on the overlaid button. While logical it maybe felt similarly off like the collapsing header flicking colors when pressing the close button. Not sure about this, take this with a grain of salt.

The clipping problem of collapsing header title vs. close button definitely feels like a bug that would be worth fixing though. Again not sure how to fix that.

I like the idea of subbuttons / subwidgets. I wonder, is it possible to have the API structured that way, but still make the background widgets somehow know about the overlaid widgets, so that they could adapt (and e.g. clip the title text in the collapsing header case). I dunno, sounds complicated and hackish.

Sorry if I'm burdening you with these silly ideas :)

[petrihakkinen]

btw. I'm definitely missing something here because I don't know why we need SetItemAllowOverlap? Isn't the idea of hot/active stuff in imgui to track the z-order?

[ocornut]

btw. I'm definitely missing something here because I don't know why we need SetItemAllowOverlap? Isn't the idea of hot/active stuff in imgui to track the z-order?

I think you are ignoring/forgetting here that we are dealing with a single-pass immediate style ui that has no knowledge of what is upcoming. Widgets are processed and rendered at they are called, so there's lot of hack slash magic to get this sort of behaviour working.

To allow "last item submitted gets the mouse focus" it needs to introduce a frame of lag in the not-hovered to hovered transition (nb: doesn't affect the hovered to clicked transition) to validate that no other widgets is subsequently submitted that overlaps our early widget. It records an ID and check the next frame it is hasn't been overridden by another ID. This is what the AllowOverlap flag introduce with TreeNode. This lag was recently introduced and only active for this feature and I'm not sure buttons should behave like this by default.

Will process the rest of the comments later.