From 25eee91542adaef308270fa864fc92b0d9d6eb61 Mon Sep 17 00:00:00 2001 From: omar Date: Wed, 13 Nov 2019 21:35:42 +0100 Subject: [PATCH] Error handling: Assert if user mistakenly calls End() instead of EndChild() on a child window. (#1651) Internals: Moved some error handling code. --- docs/CHANGELOG.txt | 1 + imgui.cpp | 118 ++++++++++++++++++++++++++++----------------- imgui_internal.h | 23 ++++----- 3 files changed, 88 insertions(+), 54 deletions(-) diff --git a/docs/CHANGELOG.txt b/docs/CHANGELOG.txt index 80d2229f..29e869b0 100644 --- a/docs/CHANGELOG.txt +++ b/docs/CHANGELOG.txt @@ -74,6 +74,7 @@ Other Changes: interactions with custom multi-selections patterns. (#1896, #1861) - DragScalar, SliderScalar, InputScalar: Added p_ prefix to parameter that are pointers to the data to clarify how they are used, and more comments redirecting to the demo code. (#2844) +- Error handling: Assert if user mistakenly calls End() instead of EndChild() on a child window. (#1651) - Misc: Optimized storage of window settings data (reducing allocation count). - Misc: Windows: Do not use _wfopen() if IMGUI_DISABLE_WIN32_FUNCTIONS is defined. (#2815) - Docs: Improved and moved FAQ to docs/FAQ.md so it can be readable on the web. [@ButternCream, @ocornut] diff --git a/imgui.cpp b/imgui.cpp index 9c4f1949..42b7f454 100644 --- a/imgui.cpp +++ b/imgui.cpp @@ -54,6 +54,7 @@ CODE // [SECTION] ImGuiListClipper // [SECTION] RENDER HELPERS // [SECTION] MAIN CODE (most of the code! lots of stuff, needs tidying up!) +// [SECTION] ERROR CHECKING // [SECTION] SCROLLING // [SECTION] TOOLTIPS // [SECTION] POPUPS @@ -838,7 +839,6 @@ static const float WINDOWS_MOUSE_WHEEL_SCROLL_LOCK_TIMER = 2.00f; // Lock static void SetCurrentWindow(ImGuiWindow* window); static void FindHoveredWindow(); static ImGuiWindow* CreateNewWindow(const char* name, ImVec2 size, ImGuiWindowFlags flags); -static void CheckStacksSize(ImGuiWindow* window, bool write); static ImVec2 CalcNextScrollFromScrollTargetAndClamp(ImGuiWindow* window, bool snap_on_edges); static void AddDrawListToDrawData(ImVector* out_list, ImDrawList* draw_list); @@ -874,6 +874,10 @@ static void NavSaveLastChildNavWindowIntoParent(ImGuiWindow* nav_win static ImGuiWindow* NavRestoreLastChildNavWindow(ImGuiWindow* window); static int FindWindowFocusIndex(ImGuiWindow* window); +// Error Checking +static void ErrorCheckEndFrame(); +static void ErrorCheckBeginEndCompareStacksSize(ImGuiWindow* window, bool write); + // Misc static void UpdateMouseInputs(); static void UpdateMouseWheel(); @@ -3539,7 +3543,7 @@ void ImGui::NewFrame() } g.Time += g.IO.DeltaTime; - g.FrameScopeActive = true; + g.WithinFrameScope = true; g.FrameCount += 1; g.TooltipOverrideCount = 0; g.WindowsActiveCount = 0; @@ -3713,7 +3717,7 @@ void ImGui::NewFrame() // This fallback is particularly important as it avoid ImGui:: calls from crashing. SetNextWindowSize(ImVec2(400,400), ImGuiCond_FirstUseEver); Begin("Debug##Default"); - g.FrameScopePushedImplicitWindow = true; + g.WithinFrameScopeWithImplicitWindow = true; #ifdef IMGUI_ENABLE_TEST_ENGINE ImGuiTestEngineHook_PostNewFrame(&g); @@ -3982,7 +3986,7 @@ void ImGui::EndFrame() IM_ASSERT(g.Initialized); if (g.FrameCountEnded == g.FrameCount) // Don't process EndFrame() multiple times. return; - IM_ASSERT(g.FrameScopeActive && "Forgot to call ImGui::NewFrame()?"); + IM_ASSERT(g.WithinFrameScope && "Forgot to call ImGui::NewFrame()?"); // Notify OS when our Input Method Editor cursor has moved (e.g. CJK inputs using Microsoft IME) if (g.IO.ImeSetInputScreenPosFn && (g.PlatformImeLastPos.x == FLT_MAX || ImLengthSqr(g.PlatformImeLastPos - g.PlatformImePos) > 0.0001f)) @@ -3991,24 +3995,10 @@ void ImGui::EndFrame() g.PlatformImeLastPos = g.PlatformImePos; } - // Report when there is a mismatch of Begin/BeginChild vs End/EndChild calls. Important: Remember that the Begin/BeginChild API requires you - // to always call End/EndChild even if Begin/BeginChild returns false! (this is unfortunately inconsistent with most other Begin* API). - if (g.CurrentWindowStack.Size != 1) - { - if (g.CurrentWindowStack.Size > 1) - { - IM_ASSERT(g.CurrentWindowStack.Size == 1 && "Mismatched Begin/BeginChild vs End/EndChild calls: did you forget to call End/EndChild?"); - while (g.CurrentWindowStack.Size > 1) // FIXME-ERRORHANDLING - End(); - } - else - { - IM_ASSERT(g.CurrentWindowStack.Size == 1 && "Mismatched Begin/BeginChild vs End/EndChild calls: did you call End/EndChild too much?"); - } - } + ErrorCheckEndFrame(); // Hide implicit/fallback "Debug" window if it hasn't been used - g.FrameScopePushedImplicitWindow = false; + g.WithinFrameScopeWithImplicitWindow = false; if (g.CurrentWindow && !g.CurrentWindow->WriteAccessed) g.CurrentWindow->Active = false; End(); @@ -4035,7 +4025,7 @@ void ImGui::EndFrame() } // End frame - g.FrameScopeActive = false; + g.WithinFrameScope = false; g.FrameCountEnded = g.FrameCount; // Initiate moving window + handle left-click and right-click focus @@ -4602,7 +4592,10 @@ void ImGui::EndChild() ImGuiContext& g = *GImGui; ImGuiWindow* window = g.CurrentWindow; + IM_ASSERT(g.WithinEndChild == false); IM_ASSERT(window->Flags & ImGuiWindowFlags_ChildWindow); // Mismatched BeginChild()/EndChild() callss + + g.WithinEndChild = true; if (window->BeginCount > 1) { End(); @@ -4634,6 +4627,7 @@ void ImGui::EndChild() ItemAdd(bb, 0); } } + g.WithinEndChild = false; } // Helper to create a child window / scrolling region that looks like a normal widget frame. @@ -4656,22 +4650,6 @@ void ImGui::EndChildFrame() EndChild(); } -// Save and compare stack sizes on Begin()/End() to detect usage errors -static void CheckStacksSize(ImGuiWindow* window, bool write) -{ - // NOT checking: DC.ItemWidth, DC.AllowKeyboardFocus, DC.ButtonRepeat, DC.TextWrapPos (per window) to allow user to conveniently push once and not pop (they are cleared on Begin) - ImGuiContext& g = *GImGui; - short* p_backup = &window->DC.StackSizesBackup[0]; - { int current = window->IDStack.Size; if (write) *p_backup = (short)current; else IM_ASSERT(*p_backup == current && "PushID/PopID or TreeNode/TreePop Mismatch!"); p_backup++; } // Too few or too many PopID()/TreePop() - { int current = window->DC.GroupStack.Size; if (write) *p_backup = (short)current; else IM_ASSERT(*p_backup == current && "BeginGroup/EndGroup Mismatch!"); p_backup++; } // Too few or too many EndGroup() - { int current = g.BeginPopupStack.Size; if (write) *p_backup = (short)current; else IM_ASSERT(*p_backup == current && "BeginMenu/EndMenu or BeginPopup/EndPopup Mismatch"); p_backup++;}// Too few or too many EndMenu()/EndPopup() - // For color, style and font stacks there is an incentive to use Push/Begin/Pop/.../End patterns, so we relax our checks a little to allow them. - { int current = g.ColorModifiers.Size; if (write) *p_backup = (short)current; else IM_ASSERT(*p_backup >= current && "PushStyleColor/PopStyleColor Mismatch!"); p_backup++; } // Too few or too many PopStyleColor() - { int current = g.StyleModifiers.Size; if (write) *p_backup = (short)current; else IM_ASSERT(*p_backup >= current && "PushStyleVar/PopStyleVar Mismatch!"); p_backup++; } // Too few or too many PopStyleVar() - { int current = g.FontStack.Size; if (write) *p_backup = (short)current; else IM_ASSERT(*p_backup >= current && "PushFont/PopFont Mismatch!"); p_backup++; } // Too few or too many PopFont() - IM_ASSERT(p_backup == window->DC.StackSizesBackup + IM_ARRAYSIZE(window->DC.StackSizesBackup)); -} - static void SetWindowConditionAllowFlags(ImGuiWindow* window, ImGuiCond flags, bool enabled) { window->SetWindowPosAllowFlags = enabled ? (window->SetWindowPosAllowFlags | flags) : (window->SetWindowPosAllowFlags & ~flags); @@ -5238,7 +5216,7 @@ bool ImGui::Begin(const char* name, bool* p_open, ImGuiWindowFlags flags) ImGuiContext& g = *GImGui; const ImGuiStyle& style = g.Style; IM_ASSERT(name != NULL && name[0] != '\0'); // Window name required - IM_ASSERT(g.FrameScopeActive); // Forgot to call ImGui::NewFrame() + IM_ASSERT(g.WithinFrameScope); // Forgot to call ImGui::NewFrame() IM_ASSERT(g.FrameCountEnded != g.FrameCount); // Called ImGui::Render() or ImGui::EndFrame() and haven't called ImGui::NewFrame() again yet // Find or create @@ -5300,7 +5278,7 @@ bool ImGui::Begin(const char* name, bool* p_open, ImGuiWindowFlags flags) // We intentionally set g.CurrentWindow to NULL to prevent usage until when the viewport is set, then will call SetCurrentWindow() g.CurrentWindowStack.push_back(window); g.CurrentWindow = NULL; - CheckStacksSize(window, true); + ErrorCheckBeginEndCompareStacksSize(window, true); if (flags & ImGuiWindowFlags_Popup) { ImGuiPopupData& popup_ref = g.OpenPopupStack[g.BeginPopupStack.Size]; @@ -5839,16 +5817,22 @@ bool ImGui::Begin(const char* name, bool* p_open, ImGuiWindowFlags flags) void ImGui::End() { ImGuiContext& g = *GImGui; + ImGuiWindow* window = g.CurrentWindow; - if (g.CurrentWindowStack.Size <= 1 && g.FrameScopePushedImplicitWindow) + // Error checking: verify that user hasn't called End() too many times! + // FIXME-ERRORHANDLING + if (g.CurrentWindowStack.Size <= 1 && g.WithinFrameScopeWithImplicitWindow) { IM_ASSERT(g.CurrentWindowStack.Size > 1 && "Calling End() too many times!"); - return; // FIXME-ERRORHANDLING + return; } IM_ASSERT(g.CurrentWindowStack.Size > 0); - ImGuiWindow* window = g.CurrentWindow; + // Error checking: verify that user doesn't directly call End() on a child window. + if (window->Flags & ImGuiWindowFlags_ChildWindow) + IM_ASSERT(g.WithinEndChild && "Must call EndChild() and not End()!"); + // Close anything that is open if (window->DC.CurrentColumns) EndColumns(); PopClipRect(); // Inner window clip rectangle @@ -5861,7 +5845,7 @@ void ImGui::End() g.CurrentWindowStack.pop_back(); if (window->Flags & ImGuiWindowFlags_Popup) g.BeginPopupStack.pop_back(); - CheckStacksSize(window, false); + ErrorCheckBeginEndCompareStacksSize(window, false); SetCurrentWindow(g.CurrentWindowStack.empty() ? NULL : g.CurrentWindowStack.back()); } @@ -7007,6 +6991,54 @@ void ImGui::Unindent(float indent_w) } +//----------------------------------------------------------------------------- +// [SECTION] ERROR CHECKING +//----------------------------------------------------------------------------- + +static void ImGui::ErrorCheckEndFrame() +{ + // Report when there is a mismatch of Begin/BeginChild vs End/EndChild calls. Important: Remember that the Begin/BeginChild API requires you + // to always call End/EndChild even if Begin/BeginChild returns false! (this is unfortunately inconsistent with most other Begin* API). + ImGuiContext& g = *GImGui; + if (g.CurrentWindowStack.Size != 1) + { + if (g.CurrentWindowStack.Size > 1) + { + IM_ASSERT(g.CurrentWindowStack.Size == 1 && "Mismatched Begin/BeginChild vs End/EndChild calls: did you forget to call End/EndChild?"); + while (g.CurrentWindowStack.Size > 1) // FIXME-ERRORHANDLING + End(); + } + else + { + IM_ASSERT(g.CurrentWindowStack.Size == 1 && "Mismatched Begin/BeginChild vs End/EndChild calls: did you call End/EndChild too much?"); + } + } + +} + +// Save and compare stack sizes on Begin()/End() to detect usage errors +// Begin() calls this with write=true +// End() calls this with write=false +static void ImGui::ErrorCheckBeginEndCompareStacksSize(ImGuiWindow* window, bool write) +{ + ImGuiContext& g = *GImGui; + short* p = &window->DC.StackSizesBackup[0]; + + // Window stacks + // NOT checking: DC.ItemWidth, DC.AllowKeyboardFocus, DC.ButtonRepeat, DC.TextWrapPos (per window) to allow user to conveniently push once and not pop (they are cleared on Begin) + { int n = window->IDStack.Size; if (write) *p = (short)n; else IM_ASSERT(*p == n && "PushID/PopID or TreeNode/TreePop Mismatch!"); p++; } // Too few or too many PopID()/TreePop() + { int n = window->DC.GroupStack.Size; if (write) *p = (short)n; else IM_ASSERT(*p == n && "BeginGroup/EndGroup Mismatch!"); p++; } // Too few or too many EndGroup() + + // Global stacks + // For color, style and font stacks there is an incentive to use Push/Begin/Pop/.../End patterns, so we relax our checks a little to allow them. + { int n = g.BeginPopupStack.Size; if (write) *p = (short)n; else IM_ASSERT(*p == n && "BeginMenu/EndMenu or BeginPopup/EndPopup Mismatch!"); p++; }// Too few or too many EndMenu()/EndPopup() + { int n = g.ColorModifiers.Size; if (write) *p = (short)n; else IM_ASSERT(*p >= n && "PushStyleColor/PopStyleColor Mismatch!"); p++; } // Too few or too many PopStyleColor() + { int n = g.StyleModifiers.Size; if (write) *p = (short)n; else IM_ASSERT(*p >= n && "PushStyleVar/PopStyleVar Mismatch!"); p++; } // Too few or too many PopStyleVar() + { int n = g.FontStack.Size; if (write) *p = (short)n; else IM_ASSERT(*p >= n && "PushFont/PopFont Mismatch!"); p++; } // Too few or too many PopFont() + IM_ASSERT(p == window->DC.StackSizesBackup + IM_ARRAYSIZE(window->DC.StackSizesBackup)); +} + + //----------------------------------------------------------------------------- // [SECTION] SCROLLING //----------------------------------------------------------------------------- diff --git a/imgui_internal.h b/imgui_internal.h index 0d0dda3e..3b7ad26e 100644 --- a/imgui_internal.h +++ b/imgui_internal.h @@ -903,9 +903,7 @@ struct ImGuiPtrOrIndex struct ImGuiContext { bool Initialized; - bool FrameScopeActive; // Set by NewFrame(), cleared by EndFrame() - bool FrameScopePushedImplicitWindow; // Set by NewFrame(), cleared by EndFrame() - bool FontAtlasOwnedByContext; // Io.Fonts-> is owned by the ImGuiContext and will be destructed along with it. + bool FontAtlasOwnedByContext; // IO.Fonts-> is owned by the ImGuiContext and will be destructed along with it. ImGuiIO IO; ImGuiStyle Style; ImFont* Font; // (Shortcut) == FontStack.empty() ? IO.Font : FontStack.back() @@ -916,19 +914,22 @@ struct ImGuiContext int FrameCount; int FrameCountEnded; int FrameCountRendered; + bool WithinFrameScope; // Set by NewFrame(), cleared by EndFrame() + bool WithinFrameScopeWithImplicitWindow; // Set by NewFrame(), cleared by EndFrame() when the implicit debug window has been pushed + bool WithinEndChild; // Set within EndChild() // Windows state ImVector Windows; // Windows, sorted in display order, back to front ImVector WindowsFocusOrder; // Windows, sorted in focus order, back to front ImVector WindowsSortBuffer; ImVector CurrentWindowStack; - ImGuiStorage WindowsById; - int WindowsActiveCount; - ImGuiWindow* CurrentWindow; // Being drawn into + ImGuiStorage WindowsById; // Map window's ImGuiID to ImGuiWindow* + int WindowsActiveCount; // Number of unique windows submitted by frame + ImGuiWindow* CurrentWindow; // Window being drawn into ImGuiWindow* HoveredWindow; // Will catch mouse inputs ImGuiWindow* HoveredRootWindow; // Will catch mouse inputs (for focus/move only) ImGuiWindow* MovingWindow; // Track the window we clicked on (in order to preserve focus). The actually window that is moved is generally MovingWindow->RootWindow. - ImGuiWindow* WheelingWindow; + ImGuiWindow* WheelingWindow; // Track the window we started mouse-wheeling on. Until a timer elapse or mouse has moved, generally keep scrolling the same window even if during the course of scrolling the mouse ends up hovering a child window. ImVec2 WheelingWindowRefMousePos; float WheelingWindowTimer; @@ -1057,9 +1058,9 @@ struct ImGuiContext ImFont InputTextPasswordFont; ImGuiID TempInputTextId; // Temporary text input when CTRL+clicking on a slider, etc. ImGuiColorEditFlags ColorEditOptions; // Store user options for color edit widgets - float ColorEditLastHue; + float ColorEditLastHue; // Backup of last Hue associated to LastColor[3], so we can restore Hue in lossy RGB<>HSV round trips float ColorEditLastColor[3]; - ImVec4 ColorPickerRef; + ImVec4 ColorPickerRef; // Initial/reference color at the time of opening the color picker. bool DragCurrentAccumDirty; float DragCurrentAccum; // Accumulator for dragging modification. Always high-precision, not rounded by end-user precision settings float DragSpeedDefaultRatio; // If speed == 0.0f, uses (max-min) * DragSpeedDefaultRatio @@ -1082,7 +1083,7 @@ struct ImGuiContext ImVector SettingsHandlers; // List of .ini settings handlers ImChunkStream SettingsWindows; // ImGuiWindow .ini settings entries - // Logging + // Capture/Logging bool LogEnabled; ImGuiLogType LogType; FILE* LogFile; // If != NULL log to stdout/ file @@ -1109,7 +1110,6 @@ struct ImGuiContext ImGuiContext(ImFontAtlas* shared_font_atlas) : BackgroundDrawList(&DrawListSharedData), ForegroundDrawList(&DrawListSharedData) { Initialized = false; - FrameScopeActive = FrameScopePushedImplicitWindow = false; Font = NULL; FontSize = FontBaseSize = 0.0f; FontAtlasOwnedByContext = shared_font_atlas ? false : true; @@ -1117,6 +1117,7 @@ struct ImGuiContext Time = 0.0f; FrameCount = 0; FrameCountEnded = FrameCountRendered = -1; + WithinFrameScope = WithinFrameScopeWithImplicitWindow = WithinEndChild = false; WindowsActiveCount = 0; CurrentWindow = NULL;