<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/105410/">http://git.reviewboard.kde.org/r/105410/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 5th, 2012, 10:55 p.m., <b>Olivier Jean de Gaalon</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
<br />
<p>- Ivan</p>
<br />
<p>On July 5th, 2012, 1:29 p.m., Ivan Shapovalov wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Ivan Shapovalov.</div>
<p style="color: grey;"><i>Updated July 5, 2012, 1:29 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>languages/cpp/codecompletion/context.h <span style="color: grey">(a5fdea7)</span></li>
<li>languages/cpp/codecompletion/context.cpp <span style="color: grey">(33dcad1)</span></li>
<li>languages/cpp/codecompletion/item.cpp <span style="color: grey">(affd4e6)</span></li>
<li>languages/cpp/tests/test_cppcodecompletion.h <span style="color: grey">(20a70cb)</span></li>
<li>languages/cpp/tests/test_cppcodecompletion.cpp <span style="color: grey">(ec82d2d)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/105410/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>