Forward QComboBox signals instead of QComboBox lineedit signals
ClosedPublic

Authored by mwolff on Sep 24 2017, 12:31 PM.

Details

Summary

The QComboBox may not be editable when we connect the signals.
When we change the editable flag later on, the signals wouldn't be
connected. Instead, forward the signals available in QComboBox to
mitigate this issue.

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mwolff created this revision.Sep 24 2017, 12:31 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptSep 24 2017, 12:31 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
apol requested changes to this revision.Nov 19 2017, 10:59 PM
apol added a subscriber: apol.
apol added inline comments.
src/widgets/kurlrequester.cpp
133–149

We can use qOverload now (we depend on Qt 5.7). It will end being much more readable.

This revision now requires changes to proceed.Nov 19 2017, 10:59 PM
broulik added inline comments.
src/widgets/kurlrequester.cpp
133–149

qOverload() requires C++14 which I don't think we can use in Frameworks.

apol added a comment.Nov 19 2017, 11:16 PM

Yes, sorry: QOverload<>::of.

mwolff added a subscriber: dfaure.Nov 30 2017, 11:16 PM

right, but how do I unit test this properly? @dfaure?

dfaure added inline comments.Dec 2 2017, 3:55 PM
autotests/kurlrequestertest.cpp
216

Then use QTest::keyClick to send key events to the widget?

mwolff added inline comments.Dec 5 2017, 4:38 PM
autotests/kurlrequestertest.cpp
216

I still don't get it to work. This is my currently latest attempt:

void KUrlRequesterTest::testComboEditableRequester()
{
    KUrlComboRequester req;

    QSignalSpy textSpy(&req, &KUrlComboRequester::textChanged);
    QSignalSpy editSpy(&req, &KUrlComboRequester::textEdited);
    QSignalSpy returnSpy(&req, static_cast<void (KUrlComboRequester::*)()>(&KUrlComboRequester::returnPressed));
    QSignalSpy returnWithTextSpy(&req, static_cast<void (KUrlComboRequester::*)(const QString&)>(&KUrlComboRequester::returnPressed));

    QVERIFY(!req.comboBox()->isEditable());
    req.comboBox()->setEditable(true);
    req.show();

    QSignalSpy comboTextSpy(req.comboBox(), &QComboBox::currentTextChanged);
    QSignalSpy comboEditSpy(req.comboBox(), &QComboBox::editTextChanged);

    auto* lineEdit = req.comboBox()->lineEdit();
    QVERIFY(lineEdit);
    QSignalSpy lineTextSpy(lineEdit, &QLineEdit::textChanged);
    QSignalSpy lineEditSpy(lineEdit, &QLineEdit::textEdited);

    // FIXME: this still doesn't emit any signal, but it works in practice when actually interacting with the widget?
    QTest::keyClicks(lineEdit, QStringLiteral("foobar"), Qt::NoModifier, 100);
    QVERIFY(lineTextSpy.wait());
    QCOMPARE(lineTextSpy.first().first().toString(), QStringLiteral("foobar"));
    QVERIFY(lineEditSpy.wait());
    QCOMPARE(lineEditSpy.first().first().toString(), QStringLiteral("foobar"));
}

Note that the widget (which I now show) actually shows the text getting written via the QTest::keyClicks method. But none of the signals is actually getting emitted... I'm flabbergasted. Looking at the QLineEdit test code, it doesn't seem to actually contain a positive test for an of isEditable...

I feel like I'm missing something fundamental here. Note that I can see the signals just fine in GammaRay when I interact with an editable combo box :-/

mwolff updated this revision to Diff 23522.Dec 5 2017, 5:15 PM
mwolff retitled this revision from WIP: Forward QComboBox signals instead of QComboBox lineedit signals to Forward QComboBox signals instead of QComboBox lineedit signals.
mwolff edited the summary of this revision. (Show Details)
mwolff removed subscribers: dfaure, broulik, apol.

proper unit test, one must not QSignalSpy::wait when the signals already arrived!

mwolff updated this revision to Diff 23604.Dec 7 2017, 9:53 AM

use QOverload::of as suggested by apol

dfaure requested changes to this revision.Dec 8 2017, 11:24 PM

Looks good, but aren't there unittests missing for the non-editable combo case?

This revision now requires changes to proceed.Dec 8 2017, 11:24 PM
mwolff updated this revision to Diff 24188.Dec 20 2017, 9:20 PM
  • expand test to non-editable combo box
  • fix wrong signal connection (regression introduced when ported to QOverload<>::of)
dfaure accepted this revision.Dec 21 2017, 8:58 AM

Thanks!

autotests/kurlrequestertest.cpp
186

Please remove this line though.

This revision was automatically updated to reflect the committed changes.

Hey all, this change breaks Kompare https://bugs.kde.org/show_bug.cgi?id=390024 which watches a KUrlRequester's textChanged signal to update a button's enabled state. We are not seeing the textChanged signal on the KUrlRequester anymore when programatically setting the url with the KUrlComboBox we give to the KUrlRequester via it's setUrl method. Looking at KUrlComboBox::setUrl it's blocking signals in the body of setUrl and since we are not connecting to the QLineEdit anymore we never get that signal.

Hey all, this change breaks Kompare https://bugs.kde.org/show_bug.cgi?id=390024 which watches a KUrlRequester's textChanged signal to update a button's enabled state. We are not seeing the textChanged signal on the KUrlRequester anymore when programatically setting the url with the KUrlComboBox we give to the KUrlRequester via it's setUrl method. Looking at KUrlComboBox::setUrl it's blocking signals in the body of setUrl and since we are not connecting to the QLineEdit anymore we never get that signal.

So to be clear the bug is:

  • connected to KUrlRequester::textChanged
  • calling KUrlRequester->comboBox()->setUrl()
  • no textChanged signal emitted

this isn't covered by the test, so we need to add that and ensure it works