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
Details
Details
- Reviewers
mwolff - Group Reviewers
KDevelop - Commits
- R32:4f2fc9e32452: Replace leading typed text when completing function implementation
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
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.
Comment Actions
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.
Comment Actions
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
but as I said, your new behavior is already better than before, we can improve it later as needed | |
Comment Actions
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.
Comment Actions
I've changed the implementation to accomodate for whitespace, by comparing
a whitespace-stripped version of the signature and the leading text.
Comment Actions
can you also add a test for this new behavior with white space?
thanks
| kdevplatform/language/duchain/stringhelpers.cpp | ||
|---|---|---|
| 513 | remove(QLatin1Char(' ')) | |
Comment Actions
I actually changed the last test to also test for this. Should I split it to a new one ?
| 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 | |
Comment Actions
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 ?
Comment Actions
master sounds fine to me; after a bit of exposure we could backport to 5.3 branch, targeting 5.3.1.