Replace leading typed text when completing function implementation
ClosedPublic

Authored by amhndu on Oct 19 2018, 6:33 PM.

Details

Summary

When executing function-implementation auto-completion, this will now
replace and leading typed text that matches the proposed completion
instead of duplicating it.

BUG: 384710

Test Plan
int Foo(int thing);
struct Bar
{
  void Method(int thing);
}

Start typing int Foo or Bar::Meth and then execute the offered completion.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
amhndu created this revision.Oct 19 2018, 6:33 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptOct 19 2018, 6:33 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
amhndu requested review of this revision.Oct 19 2018, 6:33 PM
apol added a subscriber: apol.Oct 19 2018, 6:38 PM

Would it be possible to get a test for this?

amhndu updated this revision to Diff 43961.Oct 20 2018, 8:42 AM

Added tests in testCompleteFunction, which already has some implements-completion
tests. Should I move them to a different test, something like testImplementExecute
in a different patch or this one ?

Completion tests are also broken, crashing after executing a single test case
because of a delayed connection, triggering the slot only after the document is closed.
As a workaround, I've used

diff --git a/plugins/clang/tests/codecompletiontestbase.cpp b/plugins/clang/tests/codecompletiontestbase.cpp
index 5b351d0d27..282f7da36e 100644
--- a/plugins/clang/tests/codecompletiontestbase.cpp
+++ b/plugins/clang/tests/codecompletiontestbase.cpp
@@ -70,6 +70,7 @@ void CodeCompletionTestBase::initTestCase()
     m_projectController = new TestProjectController(core);
     core->setProjectController(m_projectController);
     ICore::self()->documentController()->closeAllDocuments();
+    QObject::disconnect(ICore::self()->documentController(), nullptr, nullptr, nullptr);
 
     ClangSettingsManager::self()->m_enableTesting = true;
 }

Which still has a crash, but only at the very end after executing everything.

mwolff requested changes to this revision.Oct 21 2018, 4:02 PM
mwolff added a subscriber: mwolff.

lgtm in general, three small nitpicks and I'll have a look at the broken unit test now too.

plugins/clang/codecompletion/context.cpp
110

please move this to stringhelpers.h/cpp in kdevplatform/language/duchain, call it stripLeadingWhitespace or similar

113

this should return midRef

343–356

const auto* document =

345

I agree that we should integrate this proposed feature for now, but want to mention that it can easily fail. Most notably, it will fail when

  • someone uses the boost coding style with the return type on its own line
  • whitespace mismatch, e.g. someone typed foo* but the code completion wants to insert foo *

but as I said, your new behavior is already better than before, we can improve it later as needed

This revision now requires changes to proceed.Oct 21 2018, 4:02 PM

Added tests in testCompleteFunction, which already has some implements-completion
tests. Should I move them to a different test, something like testImplementExecute
in a different patch or this one ?

Completion tests are also broken, crashing after executing a single test case
because of a delayed connection, triggering the slot only after the document is closed.
As a workaround, I've used

diff --git a/plugins/clang/tests/codecompletiontestbase.cpp b/plugins/clang/tests/codecompletiontestbase.cpp
index 5b351d0d27..282f7da36e 100644
--- a/plugins/clang/tests/codecompletiontestbase.cpp
+++ b/plugins/clang/tests/codecompletiontestbase.cpp
@@ -70,6 +70,7 @@ void CodeCompletionTestBase::initTestCase()
     m_projectController = new TestProjectController(core);
     core->setProjectController(m_projectController);
     ICore::self()->documentController()->closeAllDocuments();
+    QObject::disconnect(ICore::self()->documentController(), nullptr, nullptr, nullptr);
 
     ClangSettingsManager::self()->m_enableTesting = true;
 }

Which still has a crash, but only at the very end after executing everything.

I've fixed both issues with:

commit 8f9f8d1c4452f8d9291ae530f3720511fd203aef
Author: Milian Wolff <mail@milianw.de>
Date:   Sun Oct 21 20:12:37 2018 +0200

    Fix crashes when document gets destroyed directly after load
    
    This fixes the clang code completion unit test, which used to
    crash on exit since the event loop wasn't run between loading
    a document and destroying it again.
    
    To guard against this, we need to jump through a QPointer hoop,
    which requires some lambda boiler plate but otherwise isn't too bad.

This should work for all unit tests and doesn't require such a hackish disconnect.

amhndu updated this revision to Diff 44039.Oct 21 2018, 6:32 PM

I've changed the implementation to accomodate for whitespace, by comparing
a whitespace-stripped version of the signature and the leading text.

mwolff requested changes to this revision.Oct 21 2018, 6:39 PM

can you also add a test for this new behavior with white space?

thanks

kdevplatform/language/duchain/stringhelpers.cpp
513

remove(QLatin1Char(' '))

This revision now requires changes to proceed.Oct 21 2018, 6:39 PM

I actually changed the last test to also test for this. Should I split it to a new one ?

amhndu updated this revision to Diff 44045.Oct 21 2018, 7:02 PM

Updated to use QChar overload as suggested.

amhndu marked 2 inline comments as done.Oct 21 2018, 7:03 PM
mwolff added inline comments.Oct 21 2018, 7:40 PM
plugins/clang/tests/test_codecompletion.cpp
1403

ah yes, then please keep the old version and duplicate this one with a different row title - I was looking for a row that had "whitespace mismatch" or similar in its title

amhndu updated this revision to Diff 44054.Oct 22 2018, 3:41 AM

Created new test case for whitespace mismatch.

amhndu marked an inline comment as done.Oct 22 2018, 3:42 AM
mwolff accepted this revision.Oct 22 2018, 10:43 AM

thanks a lot! do you have commit rights, or should we push this for you?

This revision is now accepted and ready to land.Oct 22 2018, 10:43 AM

I do have push rights, I can land this tomorrow as I'll be travelling a bit today.
Which branch should this be landed against ?

I'd say master personally, but @brauch or @kfunk should decide that.

master sounds fine to me; after a bit of exposure we could backport to 5.3 branch, targeting 5.3.1.

This revision was automatically updated to reflect the committed changes.