<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>
<p>On July 6th, 2012, 6:52 a.m., <b>Ivan Shapovalov</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;">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>
</blockquote>
<p>On July 6th, 2012, 9:33 a.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;">There is a rather serious problem here, unfortunately. You cannot aquire the ForegroundLock when the duchain is locked, or you get deadlocks. That means that you cannot call doCaseCompletion in the constructor at all, as the DUChain is guaranteed to be read-locked throughout. The only way to change this would be to unlock before calling doCaseCompletion() and getParentContext() (since getParentContext is where the CaseAccess context is constructed) then relock immediately after and check m_duContext again. This is a bit ugly/verbose... if you can, try to get the case expression type at another point.
(/me mutters something about the threading model and welcomes you to the dark side of KDevelop)
Once you fix this, make sure to replace the ENSURE_CHAIN_NOT_LOCKED before aquiring the ForegroundLock... and add a comment explaining that it should never be removed.
As I was saying before I noticed...
-------------------------------------
I'm not in favor of /adding/ code to fix subsets of problems caused by other code, when the other code can be changed (albeit less simply). I would prefer you keep your other fixes, and simply drop that one from this patch. Perhaps in another review we can hash out the best fix for that.
Your current fix for the alt text issue breaks signal/slot completion items which also use alt text. I think the correct fix would be to not set or use the alt text on the specialItemsForArgumentType. That would provide adequate information and look ok.. no?
Also, add a comment explaining the "if (prefix.isEmpty) return" for posterity.</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;">Uh. Sorry for violating the threading model... Will fix it, of course.
For the alt-text, I've got another fix which does not use the alt-text at all and also leaves scope prefix only in type column, like this:
---------------------------------------
someNamespace::someEnumTypeName | foo
someNamespace::someEnumTypeName | bar
---------------------------------------
(though in completion itself correct prefixes are prepended.)
I'll upload it as soon as I fix threading.</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>