Review Request: Proper auto-completion in switch statements

[Ivan Shapovalov]

> On July 5, 2012, 10:55 p.m., Olivier Jean de Gaalon wrote:
> > Without time for a proper review...
> > Demoting vs hiding: item.cpp is actually (I'm quite sure) wrongly promoting items, otherwise you'd never have to manually demote them to 0. I'm actually thinking you should lose the demote, and if you want you can send another patch to fix item.cpp.
> > 
> > The alternative text should only be blank if the enum is local (which shouldn't happen anymore), otherwise it should be something like "namespace::enumname::foo". Is that not the case?
> > If it's not, specialItemsForArgumentType needs to be fixed to provide proper alternative text for inserted items, or not use alt text at all.

Demoting vs hiding: item.cpp AFAIK doesn't know whether we are completing for a type or instance; it uses only type-id to do its matching (the type is contained in field currentMatchContext).
But well, I'll dig in a bit more. Maybe we could pass "type or instance" flag based on the completion context's type.

Alt-text: yes, it's the case. Though the issue is a bit different: code in item.cpp forcedly returns an empty type-name prefix if alternative text is set.

So, for _local_ enumerations we see following in the completion window:
------------------------
someEnumTypeName  |  foo <-- here "foo" and "someEnumTypeName" are generated by Cpp::NormalDeclarationCompletionItem::data()
someEnumTypeName  |  bar
------------------------

And for non-local enumerations we see following:
-----------------------
  |  someNamespace::foo <-- here "someNamespace::foo" is the alternative text (which, being set, makes type column empty)
  |  someNamespace::bar
-----------------------

My fix makes the latter case look like:
------------------------------------------------------
someNamespace::someEnumTypeName  |  someNamespace::foo
someNamespace::someEnumTypeName  |  someNamespace::bar
------------------------------------------------------

Which one is better? That fix makes "someNamespace::" to be repeated twice per line, is it OK?


- Ivan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105410/#review15443
-----------------------------------------------------------


On July 5, 2012, 1:29 p.m., Ivan Shapovalov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105410/
> -----------------------------------------------------------
> 
> (Updated July 5, 2012, 1:29 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> Improve code completion within switch statements.
> 
> 1) Resolve type of the switch'd expression correctly
>  - in switchExpressionType(), replaced evaluateType() with evaluateExpression()
> 
> 2) Correctly complete code for enumerations (e. g. do not mark type declarations as matching items)
>  - this required adding integral constant check in CodeCompletionContext::standardAccessCompletionItems()
> 
> 3) Add completion items for enumerations declared in different scopes
>  - this required adding another condition branch in the end of CodeCompletionContext::standardAccessCompletionItems(),
> which in turn required proper setting of m_expressionResult in CaseAccess contexts,
> which required moving switchExpressionType() to doCaseCompletion() to avoid code duplication.
> 
> 
> Diffs
> -----
> 
>   languages/cpp/codecompletion/context.h a5fdea7 
>   languages/cpp/codecompletion/context.cpp 33dcad1 
>   languages/cpp/codecompletion/item.cpp affd4e6 
>   languages/cpp/tests/test_cppcodecompletion.h 20a70cb 
>   languages/cpp/tests/test_cppcodecompletion.cpp ec82d2d 
> 
> Diff: http://git.reviewboard.kde.org/r/105410/diff/
> 
> 
> Testing
> -------
> 
> there is a unit-test for change (1),
> I don't know how to test (2) and
> (3) cannot be tested due to some weird things with CompletionItemTester though it can be tested manually.
> 
> 
> Thanks,
> 
> Ivan Shapovalov
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20120706/c576948b/attachment.html>