From d8dd287cdf4116eb9cf510a8735ede38b53aa0f6 Mon Sep 17 00:00:00 2001 From: paxcut <53811119+paxcut@users.noreply.github.com> Date: Fri, 5 Sep 2025 02:28:11 -0700 Subject: [PATCH] fix: Fixed ImHex crashing when using ctrl-backspace on empty file. (#2433) Editor was attempting to delete non-existent chars which is UB. Fixed by checking before deleting. Also fixed was a problem created by having to press enter to change the search string which advanced the selection to the first match. In the next step one would expect that pressing enter on the replace field would replace the selected item but was replacing the item found after he first. This was fixed by always replacing the current selection first. If the replacement is the same as the searched term then replacing won't advance the cursor, but if they are different then the current match will no longer exist so it would search fora new one. --- .github/workflows/build.yml | 10 +-- cmake/build_helpers.cmake | 2 +- dist/macOS/Brewfile | 2 +- .../content/views/view_pattern_editor.cpp | 13 +-- plugins/ui/include/ui/text_editor.hpp | 9 +- plugins/ui/source/ui/text_editor/editor.cpp | 1 - plugins/ui/source/ui/text_editor/navigate.cpp | 90 +++++++++---------- plugins/ui/source/ui/text_editor/render.cpp | 34 ++++--- plugins/ui/source/ui/text_editor/support.cpp | 44 ++++++--- plugins/ui/source/ui/text_editor/utf8.cpp | 6 +- 10 files changed, 122 insertions(+), 89 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index aedcb0f7f..6df5e25a8 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -473,15 +473,15 @@ jobs: set -x mkdir -p build cd build - CC=$(brew --prefix llvm)/bin/clang \ - CXX=$(brew --prefix llvm)/bin/clang++ \ - OBJC=$(brew --prefix llvm)/bin/clang \ - OBJCXX=$(brew --prefix llvm)/bin/clang++ \ + CC=$(brew --prefix llvm@20)/bin/clang \ + CXX=$(brew --prefix llvm@20)/bin/clang++ \ + OBJC=$(brew --prefix llvm@20)/bin/clang \ + OBJCXX=$(brew --prefix llvm@20)/bin/clang++ \ PKG_CONFIG_PATH="$(brew --prefix openssl)/lib/pkgconfig":"$(brew --prefix)/lib/pkgconfig" \ cmake -G "Ninja" \ -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} \ -DIMHEX_GENERATE_PACKAGE=ON \ - -DIMHEX_SYSTEM_LIBRARY_PATH="$(brew --prefix llvm)/lib;$(brew --prefix llvm)/lib/unwind;$(brew --prefix llvm)/lib/c++;$(brew --prefix)/lib" \ + -DIMHEX_SYSTEM_LIBRARY_PATH="$(brew --prefix llvm@20)/lib;$(brew --prefix llvm@20)/lib/unwind;$(brew --prefix llvm@20)/lib/c++;$(brew --prefix)/lib" \ -DCMAKE_C_COMPILER_LAUNCHER=ccache \ -DCMAKE_CXX_COMPILER_LAUNCHER=ccache \ -DCMAKE_OBJC_COMPILER_LAUNCHER=ccache \ diff --git a/cmake/build_helpers.cmake b/cmake/build_helpers.cmake index 27230ad30..041a238c6 100644 --- a/cmake/build_helpers.cmake +++ b/cmake/build_helpers.cmake @@ -744,7 +744,7 @@ macro(setupCompilerFlags target) endif() if (CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND APPLE) - execute_process(COMMAND brew --prefix llvm OUTPUT_VARIABLE LLVM_PREFIX OUTPUT_STRIP_TRAILING_WHITESPACE) + execute_process(COMMAND brew --prefix llvm@20 OUTPUT_VARIABLE LLVM_PREFIX OUTPUT_STRIP_TRAILING_WHITESPACE) set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -L${LLVM_PREFIX}/lib/c++") set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -L${LLVM_PREFIX}/lib/c++") addCCXXFlag("-Wno-unknown-warning-option" ${target}) diff --git a/dist/macOS/Brewfile b/dist/macOS/Brewfile index 820d35ad6..2057b1a1f 100644 --- a/dist/macOS/Brewfile +++ b/dist/macOS/Brewfile @@ -6,7 +6,7 @@ brew "freetype2" brew "libmagic" brew "pkg-config" brew "curl" -brew "llvm" +brew "llvm@20" brew "glfw" brew "ninja" brew "zlib" diff --git a/plugins/builtin/source/content/views/view_pattern_editor.cpp b/plugins/builtin/source/content/views/view_pattern_editor.cpp index fbcf613bf..ce3d62439 100644 --- a/plugins/builtin/source/content/views/view_pattern_editor.cpp +++ b/plugins/builtin/source/content/views/view_pattern_editor.cpp @@ -904,7 +904,7 @@ namespace hex::plugin::builtin { static std::string replaceWord; static bool downArrowReplace = false; static bool upArrowReplace = false; - if (ImGui::InputTextWithHint("##replaceInputTextWidget", hint.c_str(), replaceWord, replaceFlags) || downArrowReplace || upArrowReplace) { + if (ImGui::InputTextWithHint("###replaceInputTextWidget", hint.c_str(), replaceWord, replaceFlags) || downArrowReplace || upArrowReplace) { findReplaceHandler->setReplaceWord(replaceWord); historyInsert(m_replaceHistory, m_replaceHistorySize, m_replaceHistoryIndex, replaceWord); @@ -972,7 +972,8 @@ namespace hex::plugin::builtin { if ((ImGui::IsKeyPressed(ImGuiKey_F3, false)) || downArrowFind || upArrowFind || enterPressedFind) { historyInsert(m_findHistory, m_findHistorySize, m_findHistoryIndex, findWord); - position = findReplaceHandler->findMatch(textEditor, !shift && !upArrowFind); + i32 index = !shift && !upArrowFind ? 1 : -1; + position = findReplaceHandler->findMatch(textEditor, index); count = findReplaceHandler->getMatches().size(); updateCount = true; downArrowFind = false; @@ -2021,9 +2022,9 @@ namespace hex::plugin::builtin { ContentRegistry::UserInterface::addMenuItem({ "hex.builtin.menu.file", "hex.builtin.view.pattern_editor.menu.find_next" }, 1520, AllowWhileTyping + Keys::F3, [this] { if (auto editor = getEditorFromFocusedWindow(); editor != nullptr) { ui::TextEditor::FindReplaceHandler *findReplaceHandler = editor->getFindReplaceHandler(); - findReplaceHandler->findMatch(editor, true); + findReplaceHandler->findMatch(editor, 1); } else { - m_textEditor->getFindReplaceHandler()->findMatch(&*m_textEditor, true); + m_textEditor->getFindReplaceHandler()->findMatch(&*m_textEditor, 1); } }, [this] { if (auto editor = getEditorFromFocusedWindow(); editor != nullptr) { @@ -2039,9 +2040,9 @@ namespace hex::plugin::builtin { ContentRegistry::UserInterface::addMenuItem({ "hex.builtin.menu.file", "hex.builtin.view.pattern_editor.menu.find_previous" }, 1530, AllowWhileTyping + SHIFT + Keys::F3, [this] { if (auto editor = getEditorFromFocusedWindow(); editor != nullptr) { ui::TextEditor::FindReplaceHandler *findReplaceHandler = editor->getFindReplaceHandler(); - findReplaceHandler->findMatch(editor, false); + findReplaceHandler->findMatch(editor, -1); } else { - m_textEditor->getFindReplaceHandler()->findMatch(&*m_textEditor, false); + m_textEditor->getFindReplaceHandler()->findMatch(&*m_textEditor, -1); } }, [this] { if (auto editor = getEditorFromFocusedWindow(); editor != nullptr) { diff --git a/plugins/ui/include/ui/text_editor.hpp b/plugins/ui/include/ui/text_editor.hpp index 54032a3aa..afb11d293 100644 --- a/plugins/ui/include/ui/text_editor.hpp +++ b/plugins/ui/include/ui/text_editor.hpp @@ -59,6 +59,12 @@ namespace hex::ui { Coordinates getSelectedColumns(); bool isSingleLine(); bool contains(Coordinates coordinates, int8_t endsInclusive=1); + bool operator==(const Selection &o) const { + return m_start == o.m_start && m_end == o.m_end; + } + bool operator!=(const Selection &o) const { + return m_start != o.m_start || m_end != o.m_end; + } }; struct EditorState { @@ -72,7 +78,7 @@ namespace hex::ui { typedef std::vector Matches; Matches &getMatches() { return m_matches; } bool findNext(TextEditor *editor); - u32 findMatch(TextEditor *editor, bool isNex); + u32 findMatch(TextEditor *editor, i32 index); bool replace(TextEditor *editor, bool right); bool replaceAll(TextEditor *editor); std::string &getFindWord() { return m_findWord; } @@ -687,7 +693,6 @@ namespace hex::ui { bool m_raiseContextMenu = false; Coordinates m_focusAtCoords = {}; bool m_updateFocus = false; - bool m_ensureCursorVisible = false; std::vector m_clickableText; diff --git a/plugins/ui/source/ui/text_editor/editor.cpp b/plugins/ui/source/ui/text_editor/editor.cpp index 73b5d5c34..bc2232713 100644 --- a/plugins/ui/source/ui/text_editor/editor.cpp +++ b/plugins/ui/source/ui/text_editor/editor.cpp @@ -487,7 +487,6 @@ namespace hex::ui { return; auto pos = setCoordinates(m_state.m_cursorPosition); - //auto start = std::min(pos, m_state.m_selection.m_start); insertTextAt(pos, value); m_lines[pos.m_line].m_colorized = false; diff --git a/plugins/ui/source/ui/text_editor/navigate.cpp b/plugins/ui/source/ui/text_editor/navigate.cpp index a2da8b89f..2bc6e7024 100644 --- a/plugins/ui/source/ui/text_editor/navigate.cpp +++ b/plugins/ui/source/ui/text_editor/navigate.cpp @@ -120,7 +120,7 @@ namespace hex::ui { auto oldPos = m_state.m_cursorPosition; - if (isEmpty() || oldPos.m_line >= (i64)m_lines.size()) + if (isEmpty() || oldPos < Coordinates(0, 0)) return; auto lindex = m_state.m_cursorPosition.m_line; @@ -163,7 +163,7 @@ namespace hex::ui { auto oldPos = m_state.m_cursorPosition; - if (isEmpty() || oldPos.m_line >= (i64) m_lines.size()) + if (isEmpty() || oldPos > setCoordinates(-1, -1)) return; auto lindex = m_state.m_cursorPosition.m_line; @@ -184,11 +184,9 @@ namespace hex::ui { } if (select) { - if (oldPos == m_interactiveSelection.m_end) { - m_interactiveSelection.m_end = Coordinates(m_state.m_cursorPosition); - if (m_interactiveSelection.m_end == Invalid) - return; - } else if (oldPos == m_interactiveSelection.m_start) + if (oldPos == m_interactiveSelection.m_end) + m_interactiveSelection.m_end = m_state.m_cursorPosition; + else if (oldPos == m_interactiveSelection.m_start) m_interactiveSelection.m_start = m_state.m_cursorPosition; else { m_interactiveSelection.m_start = oldPos; @@ -387,16 +385,17 @@ namespace hex::ui { auto &line = m_lines[at.m_line]; auto charIndex = lineCoordinatesToIndex(at); - if (isWordChar(line.m_chars[charIndex])) { - while (charIndex > 0 && isWordChar(line.m_chars[charIndex - 1])) - --charIndex; - } else if (ispunct(line.m_chars[charIndex])) { - while (charIndex > 0 && ispunct(line.m_chars[charIndex - 1])) - --charIndex; - } else if (isspace(line.m_chars[charIndex])) { - while (charIndex > 0 && isspace(line.m_chars[charIndex - 1])) - --charIndex; + bool found = false; + while (charIndex > 0 && isWordChar(line.m_chars[charIndex - 1])) { + found = true; + --charIndex; } + while (!found && charIndex > 0 && ispunct(line.m_chars[charIndex - 1])) { + found = true; + --charIndex; + } + while (!found && charIndex > 0 && isspace(line.m_chars[charIndex - 1])) + --charIndex; return getCharacterCoordinates(at.m_line, charIndex); } @@ -408,16 +407,18 @@ namespace hex::ui { auto &line = m_lines[at.m_line]; auto charIndex = lineCoordinatesToIndex(at); - if (isWordChar(line.m_chars[charIndex])) { - while (charIndex < (i32) line.m_chars.size() && isWordChar(line.m_chars[charIndex])) - ++charIndex; - } else if (ispunct(line.m_chars[charIndex])) { - while (charIndex < (i32) line.m_chars.size() && ispunct(line.m_chars[charIndex])) - ++charIndex; - } else if (isspace(line.m_chars[charIndex])) { - while (charIndex < (i32) line.m_chars.size() && isspace(line.m_chars[charIndex])) - ++charIndex; + bool found = false; + while (charIndex < (i32) line.m_chars.size() && isWordChar(line.m_chars[charIndex])) { + found = true; + ++charIndex; } + while (!found && charIndex < (i32) line.m_chars.size() && ispunct(line.m_chars[charIndex])) { + found = true; + ++charIndex; + } + while (!found && charIndex < (i32) line.m_chars.size() && isspace(line.m_chars[charIndex])) + ++charIndex; + return getCharacterCoordinates(at.m_line, charIndex); } @@ -429,17 +430,16 @@ namespace hex::ui { auto &line = m_lines[at.m_line]; auto charIndex = lineCoordinatesToIndex(at); - if (isspace(line.m_chars[charIndex])) { - while (charIndex < (i32) line.m_chars.size() && isspace(line.m_chars[charIndex])) - ++charIndex; - } - if (isWordChar(line.m_chars[charIndex])) { - while (charIndex < (i32) line.m_chars.size() && (isWordChar(line.m_chars[charIndex]))) - ++charIndex; - } else if (ispunct(line.m_chars[charIndex])) { - while (charIndex < (i32) line.m_chars.size() && (ispunct(line.m_chars[charIndex]))) - ++charIndex; + while (charIndex < (i32) line.m_chars.size() && isspace(line.m_chars[charIndex])) + ++charIndex; + bool found = false; + while (charIndex < (i32) line.m_chars.size() && (isWordChar(line.m_chars[charIndex]))) { + found = true; + ++charIndex; } + while (!found && charIndex < (i32) line.m_chars.size() && (ispunct(line.m_chars[charIndex]))) + ++charIndex; + return getCharacterCoordinates(at.m_line, charIndex); } @@ -451,17 +451,17 @@ namespace hex::ui { auto &line = m_lines[at.m_line]; auto charIndex = lineCoordinatesToIndex(at); - if (isspace(line.m_chars[charIndex - 1])) { - while (charIndex > 0 && isspace(line.m_chars[charIndex - 1])) - --charIndex; - } - if (isWordChar(line.m_chars[charIndex - 1])) { - while (charIndex > 0 && isWordChar(line.m_chars[charIndex - 1])) - --charIndex; - } else if (ispunct(line.m_chars[charIndex - 1])) { - while (charIndex > 0 && ispunct(line.m_chars[charIndex - 1])) - --charIndex; + bool found = false; + while (charIndex > 0 && isspace(line.m_chars[charIndex - 1])) + --charIndex; + + while (charIndex > 0 && isWordChar(line.m_chars[charIndex - 1])) { + found = true; + --charIndex; } + while (!found && charIndex > 0 && ispunct(line.m_chars[charIndex - 1])) + --charIndex; + return getCharacterCoordinates(at.m_line, charIndex); } diff --git a/plugins/ui/source/ui/text_editor/render.cpp b/plugins/ui/source/ui/text_editor/render.cpp index 0b6d5df4f..5f49199f3 100644 --- a/plugins/ui/source/ui/text_editor/render.cpp +++ b/plugins/ui/source/ui/text_editor/render.cpp @@ -19,10 +19,10 @@ namespace hex::ui { m_topMarginChanged = true; } - void TextEditor::setFocusAtCoords(const Coordinates &coords, bool ensureVisible) { + void TextEditor::setFocusAtCoords(const Coordinates &coords, bool scrollToCursor) { m_focusAtCoords = coords; m_updateFocus = true; - m_ensureCursorVisible = ensureVisible; + m_scrollToCursor = scrollToCursor; } void TextEditor::clearErrorMarkers() { @@ -199,11 +199,13 @@ namespace hex::ui { auto height = ImGui::GetWindowHeight() - m_topMargin - scrollBarSize - m_charAdvance.y; auto width = ImGui::GetWindowWidth() - windowPadding.x - scrollBarSize; - auto top = (i32) rint((m_topMargin > scrollY ? m_topMargin - scrollY : scrollY) / m_charAdvance.y); - auto bottom = top + (i32) rint(height / m_charAdvance.y); + auto topPixels = m_topMargin > scrollY ? m_topMargin - scrollY : scrollY; + auto top = (i32) rint(topPixels / m_charAdvance.y) + 1; + top -= (top >= (i32) m_lines.size()); + auto bottom = (i32) rint((topPixels + height) / m_charAdvance.y); auto left = (i32) rint(scrollX / m_charAdvance.x); - auto right = left + (i32) rint(width / m_charAdvance.x); + auto right = (i32) rint((scrollX + width) / m_charAdvance.x); auto pos = setCoordinates(m_state.m_cursorPosition); pos.m_column = (i32) rint(textDistanceToLineStart(pos) / m_charAdvance.x); @@ -221,16 +223,24 @@ namespace hex::ui { } if (mScrollToCursorY) { - if (pos.m_line < top) - ImGui::SetScrollY(std::max(0.0f, pos.m_line * m_charAdvance.y)); - if (pos.m_line > bottom) + if (pos.m_line < top) { + ImGui::SetScrollY(std::max(0.0f, (pos.m_line - 1) * m_charAdvance.y)); + m_scrollToCursor = true; + } + if (pos.m_line > bottom) { ImGui::SetScrollY(std::max(0.0f, pos.m_line * m_charAdvance.y - height)); + m_scrollToCursor = true; + } } if (mScrollToCursorX) { - if (pos.m_column < left) + if (pos.m_column < left) { ImGui::SetScrollX(std::max(0.0f, pos.m_column * m_charAdvance.x)); - if (pos.m_column > right) + m_scrollToCursor = true; + } + if (pos.m_column > right) { ImGui::SetScrollX(std::max(0.0f, pos.m_column * m_charAdvance.x - width)); + m_scrollToCursor = true; + } } m_oldTopMargin = m_topMargin; } @@ -354,7 +364,7 @@ namespace hex::ui { void TextEditor::setFocus() { m_state.m_cursorPosition = m_focusAtCoords; resetCursorBlinkTime(); - if (m_ensureCursorVisible) + if (m_scrollToCursor) ensureCursorVisible(); if (!this->m_readOnly) @@ -607,7 +617,7 @@ namespace hex::ui { ImVec2 TextEditor::calculateCharAdvance() const { /* Compute mCharAdvance regarding scaled font size (Ctrl + mouse wheel)*/ const float fontSize = ImGui::GetFont()->CalcTextSizeA(ImGui::GetFontSize(), FLT_MAX, -1.0f, "#", nullptr, nullptr).x; - return ImVec2(fontSize, ImGui::GetTextLineHeightWithSpacing() * m_lineSpacing); + return ImVec2(fontSize, GImGui->FontSize * m_lineSpacing); } float TextEditor::textDistanceToLineStart(const Coordinates &aFrom) { diff --git a/plugins/ui/source/ui/text_editor/support.cpp b/plugins/ui/source/ui/text_editor/support.cpp index 676f88173..958fe4154 100644 --- a/plugins/ui/source/ui/text_editor/support.cpp +++ b/plugins/ui/source/ui/text_editor/support.cpp @@ -4,15 +4,11 @@ namespace hex::ui { bool TextEditor::Coordinates::operator==(const Coordinates &o) const { - return - m_line == o.m_line && - m_column == o.m_column; + return m_line == o.m_line && m_column == o.m_column; } bool TextEditor::Coordinates::operator!=(const Coordinates &o) const { - return - m_line != o.m_line || - m_column != o.m_column; + return m_line != o.m_line || m_column != o.m_column; } bool TextEditor::Coordinates::operator<(const Coordinates &o) const { @@ -327,6 +323,8 @@ namespace hex::ui { void TextEditor::Line::erase(LineIterator begin, u64 count) { if (count == (u64) -1) count = m_chars.size() - (begin.m_charsIter - m_chars.begin()); + else + count = std::min(count, (u64) (m_chars.size() - (begin.m_charsIter - m_chars.begin()))); m_chars.erase(begin.m_charsIter, begin.m_charsIter + count); m_colors.erase(begin.m_colorsIter, begin.m_colorsIter + count); m_flags.erase(begin.m_flagsIter, begin.m_flagsIter + count); @@ -557,6 +555,7 @@ namespace hex::ui { io.WantCaptureMouse = true; m_state.m_cursorPosition = m_interactiveSelection.m_end = screenPosToCoordinates(ImGui::GetMousePos()); setSelection(m_interactiveSelection); + ensureCursorVisible(); resetBlinking = true; } if (resetBlinking) @@ -575,7 +574,7 @@ namespace hex::ui { // The returned index is shown in the form // 'index of count' so 1 based - u32 TextEditor::FindReplaceHandler::findMatch(TextEditor *editor, bool isNext) { + u32 TextEditor::FindReplaceHandler::findMatch(TextEditor *editor, i32 index) { if (editor->m_textChanged || m_optionsChanged) { std::string findWord = getFindWord(); @@ -585,18 +584,34 @@ namespace hex::ui { findAllMatches(editor, findWord); } Coordinates targetPos = editor->m_state.m_cursorPosition; - if (editor->hasSelection()) - targetPos = isNext ? editor->m_state.m_selection.m_end : editor->m_state.m_selection.m_start; - - auto count = m_matches.size(); + i32 count = m_matches.size(); if (count == 0) { editor->setCursorPosition(targetPos); return 0; } - if (isNext) { - for (u32 i = 0; i < count; i++) { + if (editor->hasSelection()) { + i32 matchIndex = 0; + for (i32 i = 0; i < count; i++) { + if (m_matches[i].m_selection == editor->m_state.m_selection) { + matchIndex = i; + break; + } + } + if (matchIndex >= 0 && matchIndex 0 ? editor->m_state.m_selection.m_end : (index < 0 ? editor->m_state.m_selection.m_start : editor->m_state.m_selection.m_start + Coordinates(0,1)); + + + if (index >= 0) { + for (i32 i = 0; i < count; i++) { if (targetPos <= m_matches[i].m_selection.m_start) { selectFound(editor, i); return i + 1; @@ -808,7 +823,8 @@ namespace hex::ui { } else editor->m_state.m_cursorPosition.m_column--; } - auto matchIndex = findMatch(editor, right); + i32 index = right ? 0 : -1; + auto matchIndex = findMatch(editor, index); if (matchIndex != 0) { UndoRecord u; u.m_before = editor->m_state; diff --git a/plugins/ui/source/ui/text_editor/utf8.cpp b/plugins/ui/source/ui/text_editor/utf8.cpp index 6bfc74767..dfc9ae54e 100644 --- a/plugins/ui/source/ui/text_editor/utf8.cpp +++ b/plugins/ui/source/ui/text_editor/utf8.cpp @@ -114,8 +114,10 @@ namespace hex::ui { TextEditor::Coordinates TextEditor::screenPosToCoordinates(const ImVec2 &position) { ImVec2 local = position - ImGui::GetCursorScreenPos(); i32 lineNo = std::max(0, (i32) floor(local.y / m_charAdvance.y)); - if (local.x < (m_leftMargin - 2_scaled) || lineNo >= (i32) m_lines.size() || m_lines[lineNo].empty()) - return setCoordinates(std::min(lineNo, (i32) m_lines.size() - 1), 0); + if (lineNo >= (i32) m_lines.size()) + return setCoordinates((i32) m_lines.size() - 1, -1); + else if (local.x < (m_leftMargin - 2_scaled) || m_lines[lineNo].empty()) + return setCoordinates(lineNo, 0); std::string line = m_lines[lineNo].m_chars; local.x -= (m_leftMargin - 5_scaled); i32 count = 0;