From 3a62599a3fc6666bfa4b2bf03cd4f2f372593245 Mon Sep 17 00:00:00 2001 From: ericek111 Date: Thu, 15 Oct 2026 10:13:33 +0200 Subject: [PATCH] Long-press popups: one owner, always on screen - A second long-press finishes the first (typing its choice) before opening its own popup, instead of sharing one - Popups move down over their key when the room above is too small - No popup over an unknown key: an empty rect made every cell zero wide - A repeated touch id cancels the stale pointer instead of leaking it Co-Authored-By: Claude Opus 5.5 --- qml/Main.qml | 1 + src/input/AlternatesPicker.cpp | 10 +++-- src/input/AlternatesPicker.h | 5 ++- src/input/KeyboardSurface.cpp | 73 +++++++++++++++++++++++++++------- src/input/KeyboardSurface.h | 13 +++++- tests/tst_alternatespicker.cpp | 22 ++++++++-- tests/tst_keyboardsurface.cpp | 18 +++++++++ 7 files changed, 117 insertions(+), 25 deletions(-) diff --git a/qml/Main.qml b/qml/Main.qml index 1a5b26a..696cf50 100644 --- a/qml/Main.qml +++ b/qml/Main.qml @@ -50,6 +50,7 @@ Window { visible: !root.settingsOpen model: root.layoutModel controller: root.controller + popupHeadroom: root.popupMargin + anchors.margins } SettingsPanel { diff --git a/src/input/AlternatesPicker.cpp b/src/input/AlternatesPicker.cpp index e134bf4..772bb91 100644 --- a/src/input/AlternatesPicker.cpp +++ b/src/input/AlternatesPicker.cpp @@ -2,14 +2,16 @@ #include -void AlternatesPicker::open(const QRectF &keyRect, const QStringList &choices, qreal boundsWidth, qreal gap) +void AlternatesPicker::open(const QRectF &keyRect, const QStringList &choices, qreal boundsWidth, qreal minTop, + qreal gap) { - if (choices.isEmpty()) - return; + if (choices.isEmpty() || keyRect.isEmpty()) + return; // an empty rect would make every cell zero wide const qreal width = keyRect.width() * choices.size(); const qreal left = qBound(0.0, keyRect.left(), qMax(0.0, boundsWidth - width)); - m_rect = QRectF(left, keyRect.top() - keyRect.height() - gap, width, keyRect.height()); + const qreal top = qMax(minTop, keyRect.top() - keyRect.height() - gap); + m_rect = QRectF(left, top, width, keyRect.height()); m_choices = choices; m_selected = -1; selectAt(keyRect.center().x()); // emits changed() diff --git a/src/input/AlternatesPicker.h b/src/input/AlternatesPicker.h index 7d775a0..5adcc88 100644 --- a/src/input/AlternatesPicker.h +++ b/src/input/AlternatesPicker.h @@ -28,8 +28,9 @@ public: // Places one key-sized cell per choice just above the key, starting over it // and extending right (shifted left near the edge to stay within - // [0, boundsWidth]). The cell above the finger starts selected. - void open(const QRectF &keyRect, const QStringList &choices, qreal boundsWidth, qreal gap); + // [0, boundsWidth]; lowered, over the key, to stay below minTop). The cell + // above the finger starts selected. + void open(const QRectF &keyRect, const QStringList &choices, qreal boundsWidth, qreal minTop, qreal gap); void selectAt(qreal x); void close(); diff --git a/src/input/KeyboardSurface.cpp b/src/input/KeyboardSurface.cpp index bd5786e..1767730 100644 --- a/src/input/KeyboardSurface.cpp +++ b/src/input/KeyboardSurface.cpp @@ -97,8 +97,15 @@ void KeyboardSurface::geometryChange(const QRectF &newGeometry, const QRectF &ol void KeyboardSurface::pointerDown(int id, QPointF pos, qint64 timeMs) { + m_lastEventMs = timeMs; if (!m_model || !m_controller) return; + // A press for an id still down means its release was lost. + if (const auto stale = m_pointers.constFind(id); stale != m_pointers.cend()) { + const Pointer pointer = *stale; + m_pointers.erase(stale); + cancel(pointer); + } // Rollover would let go of keys still held, which hold-keys mode must not. if (!m_controller->holdsKeys()) commitPendingTaps(timeMs); @@ -134,7 +141,8 @@ void KeyboardSurface::pointerMove(int id, QPointF pos) m_model->setFlickProgress(pointer.index, pointer.tracker.flickProgress()); break; case Gesture::LongPress: - m_alternates.selectAt(pos.x()); + if (ownsAlternates(pointer)) + m_alternates.selectAt(pos.x()); break; case Gesture::Trackpad: m_controller->moveCursor(pointer.tracker.takeTrackpadSteps()); @@ -149,6 +157,7 @@ void KeyboardSurface::pointerMove(int id, QPointF pos) void KeyboardSurface::pointerUp(int id, qint64 timeMs) { + m_lastEventMs = timeMs; const auto it = m_pointers.constFind(id); if (it == m_pointers.cend()) return; @@ -166,7 +175,7 @@ void KeyboardSurface::longPressElapsed(int id, quint64 serial) if (it->tracker.gesture() == Gesture::Trackpad) setTrackpadActive(true); else if (it->tracker.gesture() == Gesture::LongPress) - openAlternates(*it); + openAlternates(Pointer(*it), m_lastEventMs); // a copy: opening may erase from m_pointers } // --- Gesture outcomes @@ -184,10 +193,14 @@ void KeyboardSurface::finish(const Pointer &pointer, qint64 timeMs) case Gesture::Flick: m_controller->keyUp(key, tracker.flickCommitted() ? KeyOutput::Flick : KeyOutput::Primary, {}, timeMs); break; - case Gesture::LongPress: - m_controller->keyUp(key, KeyOutput::Alternate, m_alternates.selectedText(), timeMs); - m_alternates.close(); + case Gesture::LongPress: { + // Without the popup (it never opened), the key types its primary output. + const QString alternate = ownsAlternates(pointer) ? m_alternates.selectedText() : QString(); + if (ownsAlternates(pointer)) + closeAlternates(); + m_controller->keyUp(key, KeyOutput::Alternate, alternate, timeMs); break; + } case Gesture::Trackpad: m_controller->keyCancelled(key); setTrackpadActive(false); @@ -201,6 +214,18 @@ void KeyboardSurface::finish(const Pointer &pointer, qint64 timeMs) } } +void KeyboardSurface::cancel(const Pointer &pointer) +{ + if (m_model) + release(pointer); + if (ownsAlternates(pointer)) + closeAlternates(); + if (pointer.tracker.gesture() == Gesture::Trackpad) + setTrackpadActive(false); + if (m_controller) + m_controller->keyCancelled(pointer.tracker.key()); +} + // Clears the visual state a pointer left on the model. void KeyboardSurface::release(const Pointer &pointer) { @@ -232,14 +257,38 @@ void KeyboardSurface::updateSlideHover(Pointer &pointer) pointer.hoverIndex = hover; } -void KeyboardSurface::openAlternates(const Pointer &pointer) +// One popup: a second long-press first finishes the one showing it, as a new +// press commits pending taps. +void KeyboardSurface::openAlternates(const Pointer &pointer, qint64 timeMs) { + if (pointer.index < 0) + return; // the layout changed under the finger: no key to place it over + for (auto it = m_pointers.begin(); it != m_pointers.end(); ++it) { + if (it->serial != pointer.serial && ownsAlternates(*it)) { + const Pointer owner = *it; + m_pointers.erase(it); + finish(owner, timeMs); + break; + } + } QStringList choices = pointer.tracker.key().alternates; if (m_model->isUppercase()) { for (QString &choice : choices) choice = choice.toUpper(); } - m_alternates.open(m_model->keyRect(pointer.index), choices, width(), PopupGap); + m_alternates.open(m_model->keyRect(pointer.index), choices, width(), -m_popupHeadroom, PopupGap); + m_alternatesSerial = pointer.serial; +} + +bool KeyboardSurface::ownsAlternates(const Pointer &pointer) const +{ + return pointer.serial == m_alternatesSerial && m_alternates.isOpen(); +} + +void KeyboardSurface::closeAlternates() +{ + m_alternates.close(); + m_alternatesSerial = 0; } void KeyboardSurface::setTrackpadActive(bool active) @@ -253,13 +302,9 @@ void KeyboardSurface::setTrackpadActive(bool active) void KeyboardSurface::cancelAll() { const auto pointers = std::exchange(m_pointers, {}); - for (const Pointer &pointer : pointers) { - if (m_model) - release(pointer); - if (m_controller) - m_controller->keyCancelled(pointer.tracker.key()); - } - m_alternates.close(); + for (const Pointer &pointer : pointers) + cancel(pointer); + closeAlternates(); setTrackpadActive(false); } diff --git a/src/input/KeyboardSurface.h b/src/input/KeyboardSurface.h index e6d33ea..5d5c5e2 100644 --- a/src/input/KeyboardSurface.h +++ b/src/input/KeyboardSurface.h @@ -8,6 +8,7 @@ #include #include #include +#include // Receives all touch points (and the mouse) over the keyboard, runs one // PointerTracker per finger, and turns finished gestures into controller @@ -21,6 +22,9 @@ class KeyboardSurface : public QQuickItem Q_PROPERTY(KeyboardController *controller READ controller WRITE setController NOTIFY controllerChanged) Q_PROPERTY(AlternatesPicker *alternates READ alternates CONSTANT) Q_PROPERTY(bool trackpadActive READ isTrackpadActive NOTIFY trackpadActiveChanged) + // Room above the surface that long-press popups may use; taller keys put + // top-row popups lower, over their key, instead of cutting them off. + Q_PROPERTY(qreal popupHeadroom MEMBER m_popupHeadroom NOTIFY popupHeadroomChanged) public: explicit KeyboardSurface(QQuickItem *parent = nullptr); @@ -36,6 +40,7 @@ Q_SIGNALS: void modelChanged(); void controllerChanged(); void trackpadActiveChanged(); + void popupHeadroomChanged(); protected: void touchEvent(QTouchEvent *event) override; @@ -60,10 +65,13 @@ private: void longPressElapsed(int id, quint64 serial); void finish(const Pointer &pointer, qint64 timeMs); + void cancel(const Pointer &pointer); void release(const Pointer &pointer); void commitPendingTaps(qint64 timeMs); void updateSlideHover(Pointer &pointer); - void openAlternates(const Pointer &pointer); + void openAlternates(const Pointer &pointer, qint64 timeMs); + bool ownsAlternates(const Pointer &pointer) const; + void closeAlternates(); void setTrackpadActive(bool active); void cancelAll(); void forgetIndices(); @@ -71,7 +79,10 @@ private: QPointer m_model; QPointer m_controller; AlternatesPicker m_alternates; + quint64 m_alternatesSerial = 0; // the pointer the popup belongs to QHash m_pointers; quint64 m_nextSerial = 1; + qint64 m_lastEventMs = 0; // timers have no event time; they use this bool m_trackpadActive = false; + qreal m_popupHeadroom = std::numeric_limits::max(); }; diff --git a/tests/tst_alternatespicker.cpp b/tests/tst_alternatespicker.cpp index 977ef25..bb6a23c 100644 --- a/tests/tst_alternatespicker.cpp +++ b/tests/tst_alternatespicker.cpp @@ -12,7 +12,7 @@ private Q_SLOTS: void opensAboveKeyExtendingRight() { AlternatesPicker picker; - picker.open(QRectF(100, 100, 50, 60), {u"é"_s, u"è"_s, u"ê"_s}, 1000, 8); + picker.open(QRectF(100, 100, 50, 60), {u"é"_s, u"è"_s, u"ê"_s}, 1000, -1000, 8); QVERIFY(picker.isOpen()); QCOMPARE(picker.rect(), QRectF(100, 32, 150, 60)); QCOMPARE(picker.selectedText(), u"é"); @@ -21,7 +21,7 @@ private Q_SLOTS: void staysInsideRightEdgeAndSelectsUnderFinger() { AlternatesPicker picker; - picker.open(QRectF(900, 100, 50, 60), {u"a"_s, u"b"_s, u"c"_s}, 1000, 8); + picker.open(QRectF(900, 100, 50, 60), {u"a"_s, u"b"_s, u"c"_s}, 1000, -1000, 8); QCOMPARE(picker.rect().left(), 850.0); QCOMPARE(picker.selectedText(), u"b"); // key center x=925 lies over the second cell } @@ -29,7 +29,7 @@ private Q_SLOTS: void selectionFollowsFingerAndClamps() { AlternatesPicker picker; - picker.open(QRectF(100, 100, 50, 60), {u"a"_s, u"b"_s, u"c"_s}, 1000, 8); + picker.open(QRectF(100, 100, 50, 60), {u"a"_s, u"b"_s, u"c"_s}, 1000, -1000, 8); picker.selectAt(210); QCOMPARE(picker.selected(), 2); picker.selectAt(-50); @@ -41,11 +41,25 @@ private Q_SLOTS: void closeClears() { AlternatesPicker picker; - picker.open(QRectF(0, 0, 10, 10), {u"a"_s}, 100, 0); + picker.open(QRectF(0, 0, 10, 10), {u"a"_s}, 100, -1000, 0); picker.close(); QVERIFY(!picker.isOpen()); QVERIFY(picker.selectedText().isEmpty()); } + + void emptyKeyRectDoesNotOpen() + { + AlternatesPicker picker; + picker.open(QRectF(), {u"a"_s, u"b"_s}, 1000, -1000, 8); + QVERIFY(!picker.isOpen()); + } + + void lowersToStayBelowMinTop() + { + AlternatesPicker picker; + picker.open(QRectF(100, 10, 50, 60), {u"a"_s}, 1000, -20, 8); + QCOMPARE(picker.rect().top(), -20.0); + } }; QTEST_GUILESS_MAIN(TestAlternatesPicker) diff --git a/tests/tst_keyboardsurface.cpp b/tests/tst_keyboardsurface.cpp index e05f549..acc39cb 100644 --- a/tests/tst_keyboardsurface.cpp +++ b/tests/tst_keyboardsurface.cpp @@ -124,6 +124,24 @@ private Q_SLOTS: QVERIFY(!surface->alternates()->isOpen()); } + void secondLongPressFinishesFirst() + { + const QPoint e = keyCenter(u"e"_s), u = keyCenter(u"u"_s); + auto *surface = qobject_cast(keyboard); + QTest::touchEvent(&window, touchscreen).press(0, e); + QTest::qWait(LongPressWaitMs); + QTest::touchEvent(&window, touchscreen).stationary(0).press(1, u); + QTest::qWait(LongPressWaitMs); // u's popup replaces e's, which types its choice + QCOMPARE(engine.output, u"é"); + QVERIFY(surface->alternates()->isOpen()); + QTest::touchEvent(&window, touchscreen).release(0, e).stationary(1); + QCOMPARE(engine.output, u"é"); // the first finger's lift does nothing more + QVERIFY(surface->alternates()->isOpen()); + QTest::touchEvent(&window, touchscreen).release(1, u); + QCOMPARE(engine.output.size(), 2); + QVERIFY(!surface->alternates()->isOpen()); + } + void secondFingerCommitsFirst() { const QPoint a = keyCenter(u"a"_s), s = keyCenter(u"s"_s);