<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/124541/">https://git.reviewboard.kde.org/r/124541/</a>
</td>
</tr>
</table>
<br />
<p>Ship it!</p>
<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;">Awesome :-)
Only thing I'd change is adding an indentation to the search results such that they are left-aligned with the input field and the suggestions. The quickest way I can think of is doing it like this:</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%"><span style="color: #000080; font-weight: bold">diff --git a/src/apps/marble-maps/Search.qml b/src/apps/marble-maps/Search.qml</span>
<span style="color: #000080; font-weight: bold">index 99d47fc..50eb700 100644</span>
<span style="color: #A00000">--- a/src/apps/marble-maps/Search.qml</span>
<span style="color: #00A000">+++ b/src/apps/marble-maps/Search.qml</span>
<span style="color: #800080; font-weight: bold">@@ -32,6 +32,7 @@ Item {</span>
left: parent.left
right: parent.right
}
<span style="color: #00A000">+ leftIndent: Screen.pixelDensity * 3</span>
visible: false
onItemSelected: {
searchField.query = name;
<span style="color: #000080; font-weight: bold">diff --git a/src/apps/marble-maps/SearchResults.qml b/src/apps/marble-maps/SearchResults.qml</span>
<span style="color: #000080; font-weight: bold">index a00c004..febfdbd 100644</span>
<span style="color: #A00000">--- a/src/apps/marble-maps/SearchResults.qml</span>
<span style="color: #00A000">+++ b/src/apps/marble-maps/SearchResults.qml</span>
<span style="color: #800080; font-weight: bold">@@ -14,6 +14,8 @@ Item {</span>
signal itemSelected(int index, string name)
<span style="color: #00A000">+ property int leftIndent: 0</span>
<span style="color: #00A000">+</span>
SystemPalette{
id: palette
colorGroup: SystemPalette.Active
<span style="color: #800080; font-weight: bold">@@ -28,11 +30,12 @@ Item {</span>
ListView {
id: view
anchors.fill: parent
<span style="color: #00A000">+ anchors.leftMargin: root.leftIndent</span>
snapMode: ListView.SnapToItem
model: root.model
delegate: Item {
width: view.width
<span style="color: #A00000">- height: placemarkName.height + 30</span>
<span style="color: #00A000">+ height: placemarkName.height + 20</span>
Rectangle {
id: delegateBackground
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There might be a more elegant way.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Another nice thing to have would be showing icons next to search results. They are already in the model, but it needs some tweaking as QML strongly prefers taking care of icon loading itself (i.e. it wants paths, not QImage or QPixmap instances). A quick proof of concept patch is here https://paste.kde.org/prfmmeom6 but needs more work to sanitize placemark icon path handling inside Marble (wrt location and sizes). Let's deal with that outside of this review request.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">http://nienhueser.de/marble/android-search-resultlist.png
<img alt="Search results with icons" src="http://nienhueser.de/marble/android-search-resultlist.png" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" /></p></pre>
<br />
<p>- Dennis Nienhüser</p>
<br />
<p>On July 31st, 2015, 9:16 p.m. UTC, Mihail Ivchenko 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 Marble and Gábor Péterffy.</div>
<div>By Mihail Ivchenko.</div>
<p style="color: grey;"><i>Updated July 31, 2015, 9:16 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
marble
</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;">Added search completion for Marble Maps.</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;">Works on Nexus 5 and Nexus 9 (both are Android 5.1.1)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm not sure about color for background so I created a screenshot which shows all available colors from SystemPalette with default Holo Dark theme on stock Android 5.1.1.</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>src/lib/marble/declarative/SearchBackend.cpp <span style="color: grey">(5c80cc3)</span></li>
<li>data/android/drawable-xxxhdpi/search.png <span style="color: grey">(599a3c7ccdcedb11835378562f7f34c2a4c39669)</span></li>
<li>src/apps/marble-maps/Completion.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/apps/marble-maps/MainScreen.qml <span style="color: grey">(2cc996a)</span></li>
<li>src/apps/marble-maps/MarbleMaps.qrc <span style="color: grey">(d027af9)</span></li>
<li>src/apps/marble-maps/Search.qml <span style="color: grey">(896f266)</span></li>
<li>src/apps/marble-maps/SearchField.qml <span style="color: grey">(5830098)</span></li>
<li>src/apps/marble-maps/SearchResults.qml <span style="color: grey">(a910038)</span></li>
<li>src/apps/marble-maps/package/AndroidManifest.xml <span style="color: grey">(4409ec5)</span></li>
<li>src/lib/marble/declarative/SearchBackend.h <span style="color: grey">(b041de3)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/124541/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/07/31/a115b5d5-7f57-4089-a896-d8fbf2ed2320__Screenshot_2015-08-01-01-06-06.png">Screenshot_2015-08-01-01-06-06.png</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/07/31/7df5b460-e705-4da3-a846-a9d308f37daf__Screenshot_2015-08-01-01-06-12.png">Screenshot_2015-08-01-01-06-12.png</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/07/31/386444f1-8d4e-43de-96fd-9810f91e5a14__Screenshot_2015-08-01-01-06-16.png">Screenshot_2015-08-01-01-06-16.png</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/07/31/490fe2b2-8cf3-4456-82c1-08b9a81dcc89__Screenshot_2015-08-01-01-06-22.png">Screenshot_2015-08-01-01-06-22.png</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/07/31/6d497bac-01ea-424d-a636-da1b2a5d0311__Screenshot_2015-08-01-01-06-26.png">Screenshot_2015-08-01-01-06-26.png</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>