<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/119407/">https://git.reviewboard.kde.org/r/119407/</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 22nd, 2014, 8:04 p.m. UTC, <b>Sven Brauch</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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">How can I lock the foreground lock?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There's a ForegroundLock class in KDevPlatform.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Is KDevQmlJSPlugin::specialLanguageObjectNavigationWidget called in the foreground thread?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Not sure, just compare QObject::thread() to QApplication::instance()->thread() -- that should tell you.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Otherwise, I think this is a very nice feature. However, as we discussed a while ago (not sure if you agreed though?) I still think some of the widgets should be removed because they are probably more distracting than useful. Margin for example just isn't that great unless you manage to render it with an actual preview of the scene.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I've put an assert on "QThread::currentThread() == QCoreApplication::instance()->thread()", and the assertion succeeded. So, I think that KDevQmlJSPlugin::specialLanguageObjectNavigationWidget runs in the foreground thread and that no locking is needed. Do you want me to use ForegroundLock so that we are completely sure that everything is correct?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Regarding the widgets, I totally agree that some of them may not be that useful. In fact, I've already removed two or three widgets (but I don't remember which ones). "Margins" is the less useful one at this time, but I'd still keep it, because margins are difficult to imagine. Maybe the widget should be modified, though, because I don't find the 3x3 grid very relevant. I have some ideas that I will test once this patch is merged.</p></pre>
<br />
<p>- Denis</p>
<br />
<p>On July 22nd, 2014, 4:33 p.m. UTC, Denis Steckelmacher wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Denis Steckelmacher.</div>
<p style="color: grey;"><i>Updated July 22, 2014, 4:33 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdev-qmljs
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The QML/JS plugin can show navigation widgets that can be used to edit specific QML properties like width/height, spacing, color, etc. This patch extends these widgets so that they know the declaration (and hence the type) of the property being modified.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This allows two things: filters can be more specific (for instance, the font-family helper widget is displayed for the "family" property of "font", and not for anything else), and filters can be created for certain property types, independently of the property names (for instance, the color picker is now shown for any property that has the type color).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch uses DUChainUtils::itemUnderCursor to get the declaration used at a given position in a source file. The documentation of this function says that the function can only be called from the foreground thread, or with the foreground lock held. How can I lock the foreground lock? Is KDevQmlJSPlugin::specialLanguageObjectNavigationWidget called in the foreground thread?</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Manual testing has shown that everything works as expected. Here is my test file, with the results in comments:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">Item {
<span style="color: #A0A000">id:</span> root
<span style="color: #A0A000">width:</span> <span style="color: #666666">3</span> <span style="color: #408080; font-style: italic">// Shows the "Width" widget</span>
property color clr <span style="color: #408080; font-style: italic">// This line and the following ones don't show anything (this patch fixes a small bug: SimpleRange() does not create an invalid range, but a range from (0, 0) to (0, 0)</span>
property <span style="color: #B00040">float</span> margins
property font font
property string family
<span style="color: #A0A000">clr:</span> <span style="color: #BA2121">"#78d0aa"</span> <span style="color: #408080; font-style: italic">// Shows the color picker</span>
anchors.margins<span style="color: #666666">:</span> <span style="color: #666666">13</span> <span style="color: #408080; font-style: italic">// Shows the "Margins" widget</span>
anchors {
<span style="color: #A0A000">margins:</span> <span style="color: #666666">13</span> <span style="color: #408080; font-style: italic">// Shows the "Margins" widget</span>
<span style="color: #A0A000">leftMargin:</span> <span style="color: #666666">16</span> <span style="color: #408080; font-style: italic">// Shows the "Margins" widget</span>
}
<span style="color: #A0A000">margins:</span> <span style="color: #666666">34</span> <span style="color: #408080; font-style: italic">// Shows the standard navigation widget (no property is recognized because there is a filter for QQuickAnchors.margins)</span>
<span style="color: #A0A000">opacity:</span> <span style="color: #666666">0.5</span> <span style="color: #408080; font-style: italic">// Shows the "Opacity" widget</span>
font.family<span style="color: #666666">:</span> <span style="color: #BA2121">"Arial"</span> <span style="color: #408080; font-style: italic">// Shows the "FontFamily" widget</span>
<span style="color: #A0A000">family:</span> <span style="color: #BA2121">"Me"</span> <span style="color: #408080; font-style: italic">// Shows the standard navigation widget</span>
}
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch is already quite useful, but will be even more when the QML/JS plugin will be based on Qt5 and will be able to use QtQuick.Controls. When this will be the case, more advanced helper widgets will be possible (URL, combo boxes for font families and enumeration values, etc). Having type-dependend widgets will allow some nice things like a widget that allows the user to preview alignments (vertical/horizontal left/right/top/bottom/center) regardless of the name of the property that will contain the alignment.</p></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>kdevqmljsplugin.cpp <span style="color: grey">(a917bf2)</span></li>
<li>navigation/propertypreviewwidget.h <span style="color: grey">(ca416a9)</span></li>
<li>navigation/propertypreviewwidget.cpp <span style="color: grey">(6d39e7b)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119407/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>