Do you think it'd make sense to actually fix the bug you mentioned instead of changing click to a queued invoke? Feels a bit weird to be honest

In D15347#322414, @aacid wrote:

Do you think it'd make sense to actually fix the bug you mentioned instead of changing click to a queued invoke? Feels a bit weird to be honest

Makes perfect sense (if it's really a bug - am not sure yet). It was the main reason why we divided test and feature.

We'll try to get the feature D15204 + D15205 ready with priority, while having working tests at least in Phabricator. Then we can try to debug the QInputDialog issue.

@tobiasdeiminger , that hidden bug you mention, could it be the cause of https://bugs.kde.org/show_bug.cgi?id=409638 ?

In D15347#495182, @sander wrote:

@tobiasdeiminger , that hidden bug you mention, could it be the cause of https://bugs.kde.org/show_bug.cgi?id=409638 ?

That may well be. I have not tried to reproduce bug 409638 myself, but the bug description hints that it's also the QInputDialog within PickPointEngine::end that gets spawned multiple times. If so, the symptoms are the same. Seems like a good time to pick the issue here up again.

Agreed. I think the first step should be to write a unit test that triggers https://bugs.kde.org/show_bug.cgi?id=409638. That way we get a reproducible way to trigger the problem even for people without a stylus.

In D15347#495392, @sander wrote:

Agreed. I think the first step should be to write a unit test that triggers https://bugs.kde.org/show_bug.cgi?id=409638. That way we get a reproducible way to trigger the problem even for people without a stylus.

How about this test? We can reproduce a second QInputDialog by using a simple mouse click event. In bug 409638 it's probably a TabletEnterProximity event instead. We could change the test accordingly to be closer to the bug report.

void PartTest::testInlineNoteInputDialog()
{
    QInputDialog *goodInputDialog { nullptr };
    QInputDialog *spuriousInputDialog { nullptr };
    QScopedPointer<CloseDialogHelper> closeDialogHelper;
    Okular::Part part { nullptr, nullptr, QVariantList() };
    part.openUrl(QUrl::fromLocalFile(QStringLiteral(KDESRCDIR "data/file1.pdf")));
    part.widget()->show();
    QVERIFY( QTest::qWaitForWindowExposed(part.widget()) );
    const int width = part.m_pageView->horizontalScrollBar()->maximum() +
                      part.m_pageView->viewport()->width();
    const int height = part.m_pageView->verticalScrollBar()->maximum() +
                       part.m_pageView->viewport()->height();
    part.m_document->setViewportPage( 0 );

    // Open annotation toolbar and get the inline note tool button.
    QMetaObject::invokeMethod(part.m_pageView, "slotToggleAnnotator", Q_ARG( bool, true ));
    QList<QToolButton*> toolbuttonList = part.m_pageView->findChildren<QToolButton *>();
    QList<QToolButton*>::iterator it = std::find_if( toolbuttonList.begin(), toolbuttonList.end(), [](const QToolButton * x) -> bool {
        if (x->toolTip().contains("Inline Note") )
            return true;
        return false;
    } );
    QVERIFY(it != toolbuttonList.end());

    // Clicking the tool button constructs a PickPointEngine in PageViewAnnotator,
    // but doesn't use it yet.
    QToolButton* inlineNoteButton = *it;
    inlineNoteButton->click();
    QTest::mouseMove( part.m_pageView->viewport(), QPoint(width * 0.5, height * 0.2) );

    // Before we click into the viewport to finally open the input dialog,
    // we schedule a function to immediately close the dialog again.
    QTimer::singleShot( 0, this, [this, &goodInputDialog, &spuriousInputDialog]() {
        // this lambda gets processed by the local event loop of the upcoming QInputDialog
        goodInputDialog = dynamic_cast<QInputDialog*>( QApplication::activeModalWidget() );
        QVERIFY( goodInputDialog );
        QDialogButtonBox *buttonBox = goodInputDialog->findChild<QDialogButtonBox*>();
        buttonBox->button( QDialogButtonBox::Ok )->click();

        QTimer::singleShot( 0, this, [&goodInputDialog, &spuriousInputDialog]() {
            // This nested lambda is called by the local event loop of the spurious input dialog (I guess?).
            // But we shouldn't find another QInputDialog at all. If we do, it's a bug.
            spuriousInputDialog = dynamic_cast<QInputDialog*>( QApplication::activeModalWidget() );
            QVERIFY2( !spuriousInputDialog, QString("New spurious QInputDialog (0x%1) found after closing the previous QInputDialog (0x%2)")
                .arg((quintptr)spuriousInputDialog, QT_POINTER_SIZE * 2, 16, QChar('0'))
                .arg((quintptr)goodInputDialog, QT_POINTER_SIZE * 2, 16, QChar('0')).toUtf8().constData() );
            // the test hangs here, how to close gracefully?
        } );
    } );

    // Now actually cause the event to spawn the input dialog. We could also try tablet events here.
    QTest::mouseClick( part.m_pageView->viewport(), Qt::LeftButton, Qt::NoModifier, QPoint(width * 0.5, height * 0.2) );

    QVERIFY( goodInputDialog );
    QVERIFY( !spuriousInputDialog );
}

Interestingly, if we remove QTest::mouseMove and just keep QTest::mouseClick in above test, the test passes OK as expected.

This holds some hints: Events can queue up right before a QInputDialog is spawned, and are then processed in sequence and interpreted incorrectly by PageViewAnnotator. In particular, I observed the following (if mouseMove + mouseClick is used):

1. QMouseEvent(MouseButtonPress, LeftButton, localPos=204,106, screenPos=1705,131) is processed by PageViewAnnotator. It sets a locked item, but does not yet trigger m_engine->end(). That's correct.
2. QMouseEvent(MouseButtonRelease, LeftButton, localPos=204,106, screenPos=1705,131) is processed by PageViewAnnotator and leads to m_engine->end(), which spawns a QInputDialog. That's correct.
3. QMouseEvent(MouseMove, localPos=204,106, screenPos=1705,131) is processed by PageViewAnnotator and leads to m_engine->end(), which spawns a second QInputDialog. That's NOT correct.

A further hint is that MouseMove got processed last, even if it was produced first.

In bug 409638 it's probably a TabletEnterProximity event instead.

That's unlikely, because TabletEnterProximityEvents are a bit special. From https://doc.qt.io/qt-5/qtabletevent.html :

"The [...] TabletEnterProximity and TabletLeaveProximity [...] are only sent to QApplication"

In D15347#496156, @sander wrote:

In bug 409638 it's probably a TabletEnterProximity event instead.

That's unlikely, because TabletEnterProximityEvents are a bit special. From https://doc.qt.io/qt-5/qtabletevent.html :

"The [...] TabletEnterProximity and TabletLeaveProximity [...] are only sent to QApplication"

Could you put a qInfo() << *e in PageViewAnnotator::routeMouseEvent while reproducing bug 409638, so that we're sure what kind of events cause the trouble?

Could you put a qInfo() << *e in PageViewAnnotator::routeMouseEvent while reproducing bug 409638, so that we're sure what kind of events cause the trouble?

That doesn't show me any events at all, as long as a really only use the stylus. A corresponding debug output in routeTabletEvent gives

tablet: QEvent::Type(TabletPress)
tablet: QEvent::Type(TabletMove)
tablet: QEvent::Type(TabletMove)
[snip]
tablet: QEvent::Type(TabletMove)
tablet: QEvent::Type(TabletMove)
tablet: QEvent::Type(TabletMove)
tablet: QEvent::Type(TabletRelease)
tablet: QEvent::Type(TabletRelease)
tablet: QEvent::Type(TabletRelease)
tablet: QEvent::Type(TabletRelease)
tablet: QEvent::Type(TabletRelease)
tablet: QEvent::Type(TabletRelease)