From bcfb3c9cfdffa7d5158110708c618479782e3797 Mon Sep 17 00:00:00 2001 From: Luke Hines Date: Sat, 18 Jul 2026 22:29:36 +0100 Subject: [PATCH] Fix terminal cursor, search, and selection bugs --- src/ghosttyvt.cpp | 16 ++++---- src/ghosttyvt.h | 15 ++++---- src/glrenderer.cpp | 42 +++++++++++++++----- src/glrenderer.h | 2 + src/glrenderer_shaders.cpp | 24 ++++++++---- src/terminalview.cpp | 43 +++++++++++++++++++-- src/terminalview.h | 4 ++ src/terminalview_search.cpp | 77 ++++++++++++++++++++++++++++++------- 8 files changed, 176 insertions(+), 47 deletions(-) diff --git a/src/ghosttyvt.cpp b/src/ghosttyvt.cpp index 777d2c0..b837b4b 100644 --- a/src/ghosttyvt.cpp +++ b/src/ghosttyvt.cpp @@ -340,6 +340,15 @@ bool GhosttyVt::isWideSpacerCell(GhosttyCell cell) || wide == GHOSTTY_CELL_WIDE_SPACER_HEAD; } +bool GhosttyVt::isWideSpacerTailCell(GhosttyCell cell) +{ + if (cell == 0) + return false; + GhosttyCellWide wide = GHOSTTY_CELL_WIDE_NARROW; + ghostty_cell_get(cell, GHOSTTY_CELL_DATA_WIDE, &wide); + return wide == GHOSTTY_CELL_WIDE_SPACER_TAIL; +} + bool GhosttyVt::isWideCharSpacer(GhosttyTerminal terminal, uint16_t col, uint32_t row) { if (!terminal) @@ -513,16 +522,11 @@ QStringList GhosttyVt::extractSearchText() QStringList result; result.reserve(static_cast(totalRows)); - m_wideSpacerCache.clear(); - m_wideSpacerCache.reserve(static_cast(totalRows)); uint32_t graphemeBuf[128]; for (size_t row = 0; row < totalRows; row++) { QString line; line.reserve(cols); - QVector rowSpacers; - rowSpacers.resize(static_cast(cols)); - rowSpacers.fill(false); for (uint16_t col = 0; col < cols; col++) { GhosttyPoint point = {}; @@ -540,7 +544,6 @@ QStringList GhosttyVt::extractSearchText() GhosttyCell cell = 0; if (ghostty_grid_ref_cell(&ref, &cell) == GHOSTTY_SUCCESS && isWideSpacerCell(cell)) { - rowSpacers[static_cast(col)] = true; continue; } @@ -553,7 +556,6 @@ QStringList GhosttyVt::extractSearchText() } } - m_wideSpacerCache.append(rowSpacers); result.append(line); } diff --git a/src/ghosttyvt.h b/src/ghosttyvt.h index 8cf1f73..cde3a7c 100644 --- a/src/ghosttyvt.h +++ b/src/ghosttyvt.h @@ -80,10 +80,10 @@ class GhosttyVt : public QObject void setMouseButtonPressed(bool pressed); QStringList extractSearchText(); bool isSearchTextDirty() const { return m_searchTextDirty; } - - // Per-row wide-spacer cache populated by extractSearchText(). - // m_wideSpacerCache[row][col] == true means the cell is a wide-char spacer. - const QVector>& wideSpacerCache() const { return m_wideSpacerCache; } + // Mark the search cache stale. Call after operations that invalidate the + // cached row text (e.g. ghostty_terminal_resize) so the next findNext/ + // findPrevious re-extracts before navigating. + void markSearchTextDirty() { m_searchTextDirty = true; } // Scrollback persistence — export terminal content (scrollback + active) as // VT sequences that can be replayed to restore the terminal state. @@ -107,6 +107,10 @@ class GhosttyVt : public QObject // True if `cell` is a wide-char spacer (head or tail). Prefer this over // isWideCharSpacer() when you already hold a cell, to avoid a redundant ghostty_terminal_grid_ref call. static bool isWideSpacerCell(GhosttyCell cell); + // True only if `cell` is a SPACER_TAIL (head sits at col-1). Use when + // walking left to find a wide char's head; SPACER_HEAD's head is on the + // next row, so a left-walk would land on the wrong cell. + static bool isWideSpacerTailCell(GhosttyCell cell); Q_SIGNALS: void titleChanged(const QString &title); @@ -129,9 +133,6 @@ class GhosttyVt : public QObject bool m_needsEncoderSync = true; // Only sync encoders when terminal modes change bool m_searchTextDirty = true; // Set in vtWrite(), cleared by extractSearchText() - // Per-row wide-spacer cache, populated by extractSearchText(). See wideSpacerCache(). - QVector> m_wideSpacerCache; - // OSC 777 desktop notification scanner Osc777State m_osc777State = OSC777_IDLE; int m_osc777NotifyIdx = 0; diff --git a/src/glrenderer.cpp b/src/glrenderer.cpp index b57d279..9616ecb 100644 --- a/src/glrenderer.cpp +++ b/src/glrenderer.cpp @@ -269,6 +269,7 @@ void GLRenderer::Renderer::synchronize(QQuickFramebufferObject *item) bool cursorVisible = false, cursorInViewport = false; bool cursorBlinking = true; + bool cursorWideTail = false; uint16_t cursorX = 0, cursorY = 0; GhosttyRenderStateCursorVisualStyle cursorStyle = GHOSTTY_RENDER_STATE_CURSOR_VISUAL_STYLE_BLOCK; ghostty_render_state_get(state, GHOSTTY_RENDER_STATE_DATA_CURSOR_VISIBLE, &cursorVisible); @@ -278,6 +279,9 @@ void GLRenderer::Renderer::synchronize(QQuickFramebufferObject *item) ghostty_render_state_get(state, GHOSTTY_RENDER_STATE_DATA_CURSOR_VIEWPORT_X, &cursorX); ghostty_render_state_get(state, GHOSTTY_RENDER_STATE_DATA_CURSOR_VIEWPORT_Y, &cursorY); ghostty_render_state_get(state, GHOSTTY_RENDER_STATE_DATA_CURSOR_VISUAL_STYLE, &cursorStyle); + // Cursor X sits on the spacer-tail of a wide char (head at X-1). + // See ghostty src/renderer/generic.zig:2521-2534. + ghostty_render_state_get(state, GHOSTTY_RENDER_STATE_DATA_CURSOR_VIEWPORT_WIDE_TAIL, &cursorWideTail); } // Read blink state from TerminalView (single source of truth) @@ -317,16 +321,33 @@ void GLRenderer::Renderer::synchronize(QQuickFramebufferObject *item) // Update cursor state every frame (blink changes don't set dirty flag). // This is cheap — no vertex rebuild, just updating uniform values. - int newCursorX = static_cast(cursorX); - int newCursorY = static_cast(cursorY); - // Guard: m_cursorX starts at kCursorUnset, skip first-frame spurious move - if (m_cursorX != kCursorUnset && (newCursorX != static_cast(m_cursorX) || newCursorY != static_cast(m_cursorY))) { - m_prevCursorX = static_cast(m_cursorX); - m_prevCursorY = static_cast(m_cursorY); - m_cursorMoved = true; - } - m_cursorX = static_cast(cursorX); - m_cursorY = static_cast(cursorY); + // + // Only update m_cursorX/m_cursorY when ghostty is actually tracking the + // cursor (visible + in viewport). When hidden, ghostty leaves cursorX/ + // cursorY at default (0,0); writing that would flag a spurious move from + // the real position to (0,0) and fire a trail leg to the top-left. + bool cursorTracked = cursorVisible && cursorInViewport; + if (cursorTracked) { + // Shift X to the wide-char head when on a spacer-tail so the shader + // cursor test ([cursorPos.x, cursorPos.x + cursorWidth)) covers both cells. + int effectiveX = static_cast(cursorX); + int newWidth = 1; + if (cursorWideTail && effectiveX > 0) { + effectiveX -= 1; + newWidth = 2; + } + int newCursorX = effectiveX; + int newCursorY = static_cast(cursorY); + // Guard: m_cursorX starts at kCursorUnset, skip first-frame spurious move + if (m_cursorX != kCursorUnset && (newCursorX != static_cast(m_cursorX) || newCursorY != static_cast(m_cursorY))) { + m_prevCursorX = static_cast(m_cursorX); + m_prevCursorY = static_cast(m_cursorY); + m_cursorMoved = true; + } + m_cursorX = static_cast(newCursorX); + m_cursorY = static_cast(newCursorY); + m_cursorWidth = newWidth; + } bool cursorEffective = cursorVisible && cursorInViewport && cursorBlinkVisible; if (cursorEffective != m_cursorVisible) m_gridDirty = true; // blink toggle needs redraw @@ -676,6 +697,7 @@ void GLRenderer::Renderer::drawScene(int width, int height) m_program->setUniformValue(m_matrixUniform, proj); m_program->setUniformValue(m_atlasUniform, 0); m_program->setUniformValue(m_cursorPosUniform, m_cursorX, m_cursorY); + m_program->setUniformValue(m_cursorWidthUniform, static_cast(m_cursorWidth)); m_program->setUniformValue(m_cellSizeUniform, static_cast(m_cellWidth), static_cast(m_cellHeight)); diff --git a/src/glrenderer.h b/src/glrenderer.h index 85332c0..608113c 100644 --- a/src/glrenderer.h +++ b/src/glrenderer.h @@ -164,6 +164,7 @@ class GLRenderer : public QQuickFramebufferObject int m_matrixUniform = -1; int m_atlasUniform = -1; int m_cursorPosUniform = -1; + int m_cursorWidthUniform = -1; int m_cellSizeUniform = -1; int m_cursorBlinkUniform = -1; int m_cursorStyleUniform = -1; @@ -196,6 +197,7 @@ class GLRenderer : public QQuickFramebufferObject float m_cursorY = kCursorUnset; bool m_cursorVisible = false; int m_cursorStyle = 0; // 0=none, 1=block, 2=bar, 3=underline, 4=hollow + int m_cursorWidth = 1; // Cell span: 1 = narrow, 2 = wide (surrogate/CJK) int m_prevCursorX = -1; // matches kCursorUnset sentinel int m_prevCursorY = -1; float m_cursorChangeTime = 0.0f; diff --git a/src/glrenderer_shaders.cpp b/src/glrenderer_shaders.cpp index 05daf34..f4ea291 100644 --- a/src/glrenderer_shaders.cpp +++ b/src/glrenderer_shaders.cpp @@ -37,6 +37,7 @@ static const char *fragmentShaderSource = "varying float v_deco;\n" "uniform sampler2D u_atlas;\n" "uniform vec2 u_cursorPos;\n" + "uniform float u_cursorWidth;\n" "uniform vec2 u_cellSize;\n" "uniform float u_cursorBlink;\n" "uniform float u_cursorStyle;\n" @@ -47,26 +48,32 @@ static const char *fragmentShaderSource = " vec2 adj_cell = v_cell - vec2(0.0, u_topPadding);\n" " if (u_cursorBlink > 0.5 && u_cellSize.x > 0.0) {\n" " vec2 cellCoord = floor(adj_cell / u_cellSize);\n" - " if (cellCoord == u_cursorPos) {\n" + // Wide-char cursor: u_cursorPos.x is the head column, u_cursorWidth is + // 1.0 (narrow) or 2.0 (wide). The fragment is inside the cursor when + // cellCoord.x spans [u_cursorPos.x, u_cursorPos.x + u_cursorWidth). + " if (cellCoord.y == u_cursorPos.y &&\n" + " cellCoord.x >= u_cursorPos.x &&\n" + " cellCoord.x < u_cursorPos.x + u_cursorWidth) {\n" + // cx/cy are measured from the cursor's top-left (not the cell's), so + // BAR/HOLLOW render at the cursor's edges across both wide cells. + " float cursorW = u_cursorWidth * u_cellSize.x;\n" + " float cx = adj_cell.x - u_cursorPos.x * u_cellSize.x;\n" + " float cy = adj_cell.y - cellCoord.y * u_cellSize.y;\n" " if (u_cursorStyle < 1.5) {\n" " fg = v_bg_color;\n" " bg = v_fg_color;\n" " } else if (u_cursorStyle < 2.5) {\n" - " float cx = adj_cell.x - cellCoord.x * u_cellSize.x;\n" " if (cx < 2.0) {\n" " gl_FragColor = v_fg_color;\n" " return;\n" " }\n" " } else if (u_cursorStyle < 3.5) {\n" - " float cy = adj_cell.y - cellCoord.y * u_cellSize.y;\n" " if (cy > u_cellSize.y - 2.0) {\n" " gl_FragColor = v_fg_color;\n" " return;\n" " }\n" " } else {\n" - " float cx = adj_cell.x - cellCoord.x * u_cellSize.x;\n" - " float cy = adj_cell.y - cellCoord.y * u_cellSize.y;\n" - " if (cx < 1.0 || cx > u_cellSize.x - 1.0 ||\n" + " if (cx < 1.0 || cx > cursorW - 1.0 ||\n" " cy < 1.0 || cy > u_cellSize.y - 1.0) {\n" " gl_FragColor = v_fg_color;\n" " return;\n" @@ -259,6 +266,7 @@ void GLRenderer::Renderer::createShaders() m_matrixUniform = m_program->uniformLocation("u_matrix"); m_atlasUniform = m_program->uniformLocation("u_atlas"); m_cursorPosUniform = m_program->uniformLocation("u_cursorPos"); + m_cursorWidthUniform = m_program->uniformLocation("u_cursorWidth"); m_cellSizeUniform = m_program->uniformLocation("u_cellSize"); m_cursorBlinkUniform = m_program->uniformLocation("u_cursorBlink"); m_cursorStyleUniform = m_program->uniformLocation("u_cursorStyle"); @@ -503,12 +511,14 @@ void GLRenderer::Renderer::uploadPostShaderUniforms(PostShader &shader, int fboW // Cursor uniforms — Ghostty shader expects (x, y_top, w, h) // In our Y-up ortho, cy = row*cellHeight + topPadding is the bottom edge. // Add cellHeight to get the top edge (higher Y in Y-up = top of cell). + // Width is m_cursorWidth cells (1 narrow / 2 wide) so the trail rect + // matches the wide-char cursor block. if (loc.iCurrentCursor >= 0) { float cx = m_cursorX * m_cellWidth; float cy = m_cursorY * m_cellHeight + m_topPadding + static_cast(m_cellHeight); shader.program->setUniformValue(loc.iCurrentCursor, cx, cy, - static_cast(m_cellWidth), + static_cast(m_cursorWidth * m_cellWidth), static_cast(m_cellHeight)); } if (loc.iPreviousCursor >= 0) { diff --git a/src/terminalview.cpp b/src/terminalview.cpp index 07272e6..8c6c7cc 100644 --- a/src/terminalview.cpp +++ b/src/terminalview.cpp @@ -150,6 +150,8 @@ void TerminalView::recalculateDimensions() ghostty_terminal_resize(m_vt->terminal(), m_cols, m_rows, m_cellWidth, m_cellHeight); + m_vt->markSearchTextDirty(); // reflow moves search offsets + // Update mouse encoder geometry m_vt->updateMouseEncoderSize( static_cast(width()), @@ -565,8 +567,21 @@ void TerminalView::copySelection() colIdx++; } - if (rowIdx < endRow) - text += QLatin1Char('\n'); + if (rowIdx < endRow) { + // Skip the newline on soft-wrapped rows so a visual continuation of + // one logical line doesn't get split on paste. + // Mirrors exportScrollback (ghosttyvt.cpp:632-648) and refreshLinks + // (terminalview_links.cpp:62-69). + bool isWrapped = false; + GhosttyRow rawRow = 0; + if (ghostty_render_state_row_get(iterator, + GHOSTTY_RENDER_STATE_ROW_DATA_RAW, + &rawRow) == GHOSTTY_SUCCESS) { + ghostty_row_get(rawRow, GHOSTTY_ROW_DATA_WRAP, &isWrapped); + } + if (!isWrapped) + text += QLatin1Char('\n'); + } rowIdx++; } @@ -804,15 +819,37 @@ void TerminalView::selectWordAt(const QPointF &pos) // Check if the tapped cell is a word character // First, get the grapheme at the tapped column auto getGraphemeAt = [&](int c) -> uint32_t { + if (c < 0 || c >= m_cols) + return 0; if (ghostty_render_state_row_cells_select(cells, static_cast(c)) != GHOSTTY_SUCCESS) return 0; uint32_t len = 0; ghostty_render_state_row_cells_get(cells, GHOSTTY_RENDER_STATE_ROW_CELLS_DATA_GRAPHEMES_LEN, &len); + // Spacers carry no grapheme — walk LEFT to the head cell of the wide + // char and return its codepoint. SPACER_TAIL only; SPACER_HEAD's head + // is on the next row, so a left-walk would land on the wrong cell. + // Mirrors the spacer-handling pattern in refreshLinks + // (terminalview_links.cpp:78-85). + while (len == 0 && c > 0) { + GhosttyCell rawCell = 0; + if (ghostty_render_state_row_cells_get( + cells, GHOSTTY_RENDER_STATE_ROW_CELLS_DATA_RAW, + &rawCell) != GHOSTTY_SUCCESS + || !GhosttyVt::isWideSpacerTailCell(rawCell)) { + return 0; // genuinely empty cell, or SPACER_HEAD (head is on the next row) + } + c--; + if (ghostty_render_state_row_cells_select(cells, static_cast(c)) != GHOSTTY_SUCCESS) + return 0; + ghostty_render_state_row_cells_get(cells, + GHOSTTY_RENDER_STATE_ROW_CELLS_DATA_GRAPHEMES_LEN, &len); + } if (len == 0) return 0; uint32_t buf[128]; if (len > 128) return 0; - ghostty_render_state_row_cells_get(cells, GHOSTTY_RENDER_STATE_ROW_CELLS_DATA_GRAPHEMES_BUF, buf); + ghostty_render_state_row_cells_get(cells, + GHOSTTY_RENDER_STATE_ROW_CELLS_DATA_GRAPHEMES_BUF, buf); return buf[0]; // base codepoint }; diff --git a/src/terminalview.h b/src/terminalview.h index bc05691..9512194 100644 --- a/src/terminalview.h +++ b/src/terminalview.h @@ -233,6 +233,10 @@ private slots: void performSearch(); void scrollToMatch(int index); void buildCellMapping(); + // Re-extract the search cache after the terminal changed (resize or live + // PTY output), then re-run performSearch() keeping m_currentMatchIndex on + // the match nearest to the previously-current match's row. + void refreshSearchCachePreservingMatch(); void scrollViewportToBottom(); void resetSessionSwipe(); // defensive — call from every path that abandons a gesture void resetTouchInteractionState(); // consolidate TouchCancel / release state resets diff --git a/src/terminalview_search.cpp b/src/terminalview_search.cpp index 2413076..b1ec1a8 100644 --- a/src/terminalview_search.cpp +++ b/src/terminalview_search.cpp @@ -40,9 +40,12 @@ void TerminalView::closeSearch() void TerminalView::buildCellMapping() { - // Build cell-to-character index mapping for CJK/emoji support. - // Wide chars (CJK/emoji) take 2 cells but produce 1 QChar; - // without this mapping, search highlights would be wrong. + // Wide chars take 2 cells; supplementary-plane codepoints (emoji) + // expand to 2 QChars (surrogate pair) in the QString. Each cell's + // mapping must reflect the actual QChar count of its grapheme cluster, + // or every subsequent cell in the row will be mis-aligned. + // Mirrors refreshLinks() (terminalview_links.cpp:96-110) which emits + // supplementary codepoints as two QChars pointing at the same cell. m_cellMapping.clear(); m_cellMapping.reserve(m_searchCache.size()); size_t totalRows = 0; @@ -57,27 +60,43 @@ void TerminalView::buildCellMapping() return; } const int colsInt = static_cast(cols); + uint32_t graphemeBuf[128]; for (int row = 0; row < m_searchCache.size(); row++) { QVector mapping; if (row < static_cast(totalRows)) { mapping.resize(colsInt); int charIdx = 0; const QString &line = m_searchCache[row]; - // Use the wide-spacer cache from extractSearchText() when available, - // avoiding redundant ghostty_terminal_grid_ref calls per cell. - const QVector> &spacers = m_vt->wideSpacerCache(); - bool hasSpacerCache = (row < spacers.size() - && spacers[row].size() == colsInt); for (int cell = 0; cell < colsInt; cell++) { mapping[cell] = charIdx; - bool isSpacer = hasSpacerCache - ? spacers[row][cell] - : GhosttyVt::isWideCharSpacer(terminal, static_cast(cell), static_cast(row)); - if (isSpacer) { + + GhosttyPoint point = {}; + point.tag = GHOSTTY_POINT_TAG_SCREEN; + point.value.coordinate.x = static_cast(cell); + point.value.coordinate.y = static_cast(row); + GhosttyGridRef ref = GHOSTTY_INIT_SIZED(GhosttyGridRef); + if (ghostty_terminal_grid_ref(terminal, point, &ref) != GHOSTTY_SUCCESS) + continue; + + // Spacers carry no grapheme — charIdx stays put for the next cell. + GhosttyCell cellData = 0; + if (ghostty_grid_ref_cell(&ref, &cellData) == GHOSTTY_SUCCESS + && GhosttyVt::isWideSpacerCell(cellData)) { continue; } + + // Count QChars the cell contributes to the QString: + // BMP codepoint = 1, supplementary (e.g. emoji) = 2 (surrogate pair). + int advance = 1; // blank cell — extractSearchText appends one space + size_t graphemeLen = 0; + if (ghostty_grid_ref_graphemes(&ref, graphemeBuf, 128, &graphemeLen) + == GHOSTTY_SUCCESS && graphemeLen > 0) { + advance = 0; + for (size_t g = 0; g < graphemeLen; ++g) + advance += (graphemeBuf[g] > 0xFFFF) ? 2 : 1; + } if (charIdx < line.size()) - charIdx++; + charIdx = std::min(charIdx + advance, line.size()); } } m_cellMapping.append(mapping); @@ -227,8 +246,37 @@ void TerminalView::scrollToMatch(int index) } } +void TerminalView::refreshSearchCachePreservingMatch() +{ + int prevRow = (m_currentMatchIndex >= 0 && m_currentMatchIndex < m_searchMatches.size()) + ? m_searchMatches[m_currentMatchIndex].row : -1; + + m_searchCache = m_vt->extractSearchText(); + buildCellMapping(); + performSearch(); + + if (prevRow >= 0) { + int bestIdx = -1; + int bestDist = -1; + for (int i = 0; i < m_searchMatches.size(); ++i) { + int d = m_searchMatches[i].row - prevRow; + if (d < 0) d = -d; + if (bestDist < 0 || d < bestDist) { + bestDist = d; + bestIdx = i; + } + } + m_currentMatchIndex = bestIdx; + } +} + void TerminalView::findNext() { + // Live PTY output or a resize may have invalidated the cache; refresh + // before navigating so matches reflect the current terminal state. + if (m_vt && m_vt->isSearchTextDirty()) + refreshSearchCachePreservingMatch(); + if (m_searchMatches.isEmpty()) return; @@ -240,6 +288,9 @@ void TerminalView::findNext() void TerminalView::findPrevious() { + if (m_vt && m_vt->isSearchTextDirty()) + refreshSearchCachePreservingMatch(); + if (m_searchMatches.isEmpty()) return;