From 4e09f089f940335bdd628139e870ba99721fddfa Mon Sep 17 00:00:00 2001 From: Pavel Khlebovich Date: Fri, 19 Oct 2018 10:20:25 -0400 Subject: [PATCH] Fix double click can only select text within visible region Summary: This is a reimplemented 914067d14a6a27b59bba1c53cc18cb67eb9811fc (hinted by @cfeck), plus a refactoring of TerminalDisplay::findWordStart() findWordStart() had bounds check issues and an off-by-one for the case when the acquired region was more than two screens down. TerminalDisplay::findWordEnd() is very similarly implemented, but I haven't found a case where it fails, so I haven't touched it. Since 914067d14a6a27b59bba1c53cc18cb67eb9811fc caused a revert, I've tested the changes thoroughly (the cases are in the Test Plan below). BUG: 399109 Test Plan: Corner cases tested (everything was run under valgrind, since I had no crashes without it pre - f98c752bce9fa11f1e81cf6ef8c02b3c3861c341): - At the end of the word, current Konsole doesn't select "@" at the end of the word when double-clicking, but selects it when the mouse is moved. With the introduced changes the behavior is to consistently ignore the last "@" character. - When 10K lines buffer is underfilled, dragged doubleclick+scrolled up by dragging the mouse above the terminal (this generates valgrind warnings on 5a31b4af6ddaba36866d092cb169faec3ca2cef1 that might have caused Konsole to crash) - Same with overfilled 10K lines buffer. - With decreased window size and some chars hidden, dblclick and drag up and down to the end of the buffer. The ends of the string are trimmed (like in current console), the rest of the string is properly selected. - With increased window size and padded chars, prod the buffer limits. Long lines are broken into separate lines -- no difference vs current Konsole. Reviewers: #konsole, hindenburg, cfeck Reviewed By: #konsole, hindenburg Subscribers: ngraham, cfeck, konsole-devel Tags: #konsole Differential Revision: https://phabricator.kde.org/D16159 --- src/TerminalDisplay.cpp | 151 ++++++++++++++-------------------------- 1 file changed, 52 insertions(+), 99 deletions(-) diff --git a/src/TerminalDisplay.cpp b/src/TerminalDisplay.cpp index af389664..5b437937 100644 --- a/src/TerminalDisplay.cpp +++ b/src/TerminalDisplay.cpp @@ -2540,48 +2540,19 @@ void TerminalDisplay::extendSelection(const QPoint& position) if (_wordSelectionMode) { // Extend to word boundaries - int i; - QChar selClass; - const bool left_not_right = (here.y() < _iPntSelCorr.y() || (here.y() == _iPntSelCorr.y() && here.x() < _iPntSelCorr.x())); const bool old_left_not_right = (_pntSelCorr.y() < _iPntSelCorr.y() || (_pntSelCorr.y() == _iPntSelCorr.y() && _pntSelCorr.x() < _iPntSelCorr.x())); swapping = left_not_right != old_left_not_right; - // Find left (left_not_right ? from here : from start) + // Find left (left_not_right ? from here : from start of word) QPoint left = left_not_right ? here : _iPntSelCorr; - i = loc(qBound(0, left.x(), _columns - 1), qBound(0, left.y(), _lines - 1)); - if (i >= 0 && i < _imageSize) { - selClass = charClass(_image[qMin(i, _imageSize - 1)]); - while (((left.x() > 0) || (left.y() > 0 && ((_lineProperties[left.y() - 1] & LINE_WRAPPED) != 0))) - && charClass(_image[i - 1]) == selClass) { - i--; - if (left.x() > 0) { - left.rx()--; - } else { - left.rx() = _usedColumns - 1; - left.ry()--; - } - } - } - - // Find left (left_not_right ? from start : from here) + // Find left (left_not_right ? from end of word : from here) QPoint right = left_not_right ? _iPntSelCorr : here; - i = loc(qBound(0, right.x(), _columns - 1), qBound(0, right.y(), _lines - 1)); - if (i >= 0 && i < _imageSize) { - selClass = charClass(_image[qMin(i, _imageSize - 1)]); - while (((right.x() < _usedColumns - 1) || (right.y() < _usedLines - 1 && ((_lineProperties[right.y()] & LINE_WRAPPED) != 0))) - && charClass(_image[i + 1]) == selClass) { - i++; - if (right.x() < _usedColumns - 1) { - right.rx()++; - } else { - right.rx() = 0; - right.ry()++; - } - } - } + + left = findWordStart(left); + right = findWordEnd(right); // Pick which is start (ohere) and which is extension (here) if (left_not_right) { @@ -2796,8 +2767,6 @@ void TerminalDisplay::mouseDoubleClickEvent(QMouseEvent* ev) _screenWindow->clearSelection(); QPoint bgnSel = pos; - QPoint endSel = pos; - int i = loc(bgnSel.x(), bgnSel.y()); _iPntSel = bgnSel; _iPntSel.ry() += _scrollBar->value(); @@ -2805,49 +2774,14 @@ void TerminalDisplay::mouseDoubleClickEvent(QMouseEvent* ev) _actSel = 2; // within selection // find word boundaries... - const QChar selClass = charClass(_image[i]); { // find the start of the word - int x = bgnSel.x(); - while (((x > 0) || (bgnSel.y() > 0 && ((_lineProperties[bgnSel.y() - 1] & LINE_WRAPPED) != 0))) - && charClass(_image[i - 1]) == selClass) { - i--; - if (x > 0) { - x--; - } else { - x = _usedColumns - 1; - bgnSel.ry()--; - } - } - - bgnSel.setX(x); - _screenWindow->setSelectionStart(bgnSel.x() , bgnSel.y() , false); - - // find the end of the word - i = loc(endSel.x(), endSel.y()); - x = endSel.x(); - while (((x < _usedColumns - 1) || (endSel.y() < _usedLines - 1 && ((_lineProperties[endSel.y()] & LINE_WRAPPED) != 0))) - && charClass(_image[i + 1]) == selClass) { - i++; - if (x < _usedColumns - 1) { - x++; - } else { - x = 0; - endSel.ry()++; - } - } - - endSel.setX(x); - - // In word selection mode don't select @ (64) if at end of word. - if (((_image[i].rendition & RE_EXTENDED_CHAR) == 0) && - (QChar(_image[i].character) == QLatin1Char('@')) && - ((endSel.x() - bgnSel.x()) > 0)) { - endSel.setX(x - 1); - } + const QPoint bgnSel = findWordStart(pos); + const QPoint endSel = findWordEnd(pos); _actSel = 2; // within selection + _screenWindow->setSelectionStart(bgnSel.x() , bgnSel.y() , false); _screenWindow->setSelectionEnd(endSel.x() , endSel.y()); copyToX11Selection(); @@ -3008,54 +2942,73 @@ QPoint TerminalDisplay::findLineEnd(const QPoint &pnt) QPoint TerminalDisplay::findWordStart(const QPoint &pnt) { const int regSize = qMax(_screenWindow->windowLines(), 10); - const int curLine = _screenWindow->currentLine(); - int i = pnt.y(); - int x = pnt.x(); - int y = i + curLine; - int j = loc(x, i); - QVector lineProperties = _lineProperties; + const int firstVisibleLine = _screenWindow->currentLine(); + Screen *screen = _screenWindow->screen(); Character *image = _image; Character *tmp_image = nullptr; - const QChar selClass = charClass(image[j]); + + int imgLine = pnt.y(); + int x = pnt.x(); + int y = imgLine + firstVisibleLine; + int imgLoc = loc(x, imgLine); + QVector lineProperties = _lineProperties; + const QChar selClass = charClass(image[imgLoc]); const int imageSize = regSize * _columns; while (true) { - for (;;j--, x--) { + for (;;imgLoc--, x--) { + if (imgLoc < 1) { + // no more chars in this region + break; + } if (x > 0) { - if (charClass(image[j - 1]) == selClass) { + // has previous char on this line + if (charClass(image[imgLoc - 1]) == selClass) { continue; } goto out; - } else if (i > 0) { - if (((lineProperties[i - 1] & LINE_WRAPPED) != 0) && - charClass(image[j - 1]) == selClass) { - x = _columns; - i--; - y--; - continue; + } else if (imgLine > 0) { + // not the first line in the session + if ((lineProperties[imgLine - 1] & LINE_WRAPPED) != 0) { + // have continuation on prev line + if (charClass(image[imgLoc - 1]) == selClass) { + x = _columns; + imgLine--; + y--; + continue; + } } goto out; } else if (y > 0) { + // want more data, but need to fetch new region break; } else { goto out; } } - int newRegStart = qMax(0, y - regSize); - lineProperties = screen->getLineProperties(newRegStart, y - 1); - i = y - newRegStart; - if (tmp_image == nullptr) { - tmp_image = new Character[imageSize]; - image = tmp_image; + if (y <= 0) { + // No more data + goto out; } + int newRegStart = qMax(0, y - regSize + 1); + lineProperties = screen->getLineProperties(newRegStart, y - 1); + imgLine = y - newRegStart; + + delete[] tmp_image; + tmp_image = new Character[imageSize]; + image = tmp_image; + screen->getImage(tmp_image, imageSize, newRegStart, y - 1); - j = loc(x, i); + imgLoc = loc(x, imgLine); + if (imgLoc < 1) { + // Reached the start of the session + break; + } } out: delete[] tmp_image; - - return QPoint(x, y - curLine); + return QPoint(x, y - firstVisibleLine); } QPoint TerminalDisplay::findWordEnd(const QPoint &pnt)