<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/124519/">https://git.reviewboard.kde.org/r/124519/</a>
</td>
</tr>
</table>
<br />
<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;">The patch doesn't apply, can you update to latest master and redo it?</p></pre>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/124519/diff/1/?file=388502#file388502line5" style="color: black; font-weight: bold; text-decoration: underline;">src/apps/marble-maps/CircleButton.qml</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">5</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nx">Rectangle</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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 wonder if there shouldn't be a parent Item { } such that the Rectangle is not exposed and becomes an implementation detail.</p></pre>
</div>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/124519/diff/1/?file=388502#file388502line8" style="color: black; font-weight: bold; text-decoration: underline;">src/apps/marble-maps/CircleButton.qml</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">8</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nx">property</span> <span class="kr">int</span> <span class="k">size:</span> <span class="mi">0</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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'd rather name it diameter. Or use radius directly.</p></pre>
</div>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/124519/diff/1/?file=388502#file388502line9" style="color: black; font-weight: bold; text-decoration: underline;">src/apps/marble-maps/CircleButton.qml</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">9</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nx">property</span> <span class="nx">url</span> <span class="k">iconSource:</span> <span class="s2">""</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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'd rather do
<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">property alias iconSource: icon.iconSource</code></p></pre>
</div>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/124519/diff/1/?file=388502#file388502line35" style="color: black; font-weight: bold; text-decoration: underline;">src/apps/marble-maps/CircleButton.qml</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">35</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">onPressedChanged:</span> <span class="p">{</span><span class="nx">root</span><span class="p">.</span><span class="nx">color</span> <span class="o">=</span> <span class="nx">pressed</span> <span class="o">?</span> <span class="k">palette.highlight :</span> <span class="nx">palette</span><span class="p">.</span><span class="nx">button</span><span class="p">;}</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">Wouldn't it be nicer to use a state for such things? 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: #008000; font-weight: bold">Item</span> {
id<span style="color: #666666">:</span> root
property alias iconSource<span style="color: #666666">:</span> <span style="color: #008000; font-weight: bold">icon</span><span style="color: #666666">.</span>iconSource
<span style="color: #666666">...</span>
Rectangle <span style="border: 1px solid #FF0000">{</span> id<span style="color: #666666">:</span> <span style="color: #008000; font-weight: bold">background</span>; <span style="color: #666666">...</span> }
<span style="color: #008000; font-weight: bold">State</span> {
name<span style="color: #666666">:</span> <span style="color: #BA2121">"pressed"</span>
when<span style="color: #666666">:</span> touchHandler<span style="color: #666666">.</span>pressed
PropertyChanges <span style="border: 1px solid #FF0000">{</span>
target<span style="color: #666666">:</span> <span style="color: #008000; font-weight: bold">background</span>
<span style="color: #008000; font-weight: bold">color</span><span style="color: #666666">:</span> palette<span style="color: #666666">.</span>highlight
}
<span style="border: 1px solid #FF0000">}</span>
<span style="border: 1px solid #FF0000">}</span>
</pre></div>
</p></pre>
</div>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/124519/diff/1/?file=388503#file388503line84" style="color: black; font-weight: bold; text-decoration: underline;">src/apps/marble-maps/MainScreen.qml</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">83</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">size:</span> <span class="nx">Screen</span><span class="p">.</span><span class="nx">primaryOrientation</span> <span class="o">==</span> <span class="nx">Qt</span><span class="p">.</span><span class="nx">PortraitOrientation</span> <span class="o">?</span> <span class="nx">Screen</span><span class="p">.</span><span class="nx">width</span> <span class="o">*</span> <span class="mf">0.17</span> <span class="o">:</span> <span class="nx">Screen</span><span class="p">.</span><span class="nx">height</span> <span class="o">*</span> <span class="mf">0.17</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">We should be careful to choose a sensible size for buttons that are touch-operated. Both the resolution as the actual size of smartphone and tablet screens vary considerably. Therefore neither an absolute size in pixels nor something relative to their size (like here) works well.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The button is to be operated by fingers, e.g. a thumb. A good way to choose its size therefore is to make it as large (in real-world units, not screen units) as needed to operate it comfortably. Fortunately other people already did some research on that and published their results. On quick look this looks sensible: https://msdn.microsoft.com/en-us/library/windows/apps/hh465326.aspx</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That said I'd do the following changes:
- instead of one circular item, use two wrapped into each other: an outer, invisible one (which reacts to touch) and an inner, visible one
- the size of the outer one should be 9 mm (<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">size: 9 * Screen.pixelDensity</code>)
- margins should be 2 * Screen.pixelDensity</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Probably it also makes sense to specify the size and margins inside CircleButton and not here. This makes it clear that it's not intended to be changed.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I haven't tested this on a device yet, the actual values might need some minor adjustments. Google Maps for example comes up with 9 mm diameter for the visual portion of their circular buttons (Nexus 4, Nexus 7). Spotify has a play button as small as 4 mm (same devices), and I often get a button just after several tries. Here's a nice graph for such size-dependent miss-rates: http://ux.stackexchange.com/a/39041</p></pre>
</div>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/124519/diff/1/?file=388503#file388503line93" style="color: black; font-weight: bold; text-decoration: underline;">src/apps/marble-maps/MainScreen.qml</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">92</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">visible:</span> <span class="nx">marbleMaps</span><span class="p">.</span><span class="nx">positionAvailable</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">what's nicer - visible or enabled? not sure.</p></pre>
</div>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/124519/diff/1/?file=388506#file388506line321" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/marble/MarbleQuickItem.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">namespace Marble</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">321</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span> <span class="n">d</span><span class="o">-></span><span class="n">model</span><span class="p">()</span><span class="o">-></span><span class="n">positionTracking</span><span class="p">()</span><span class="o">-></span><span class="n">status</span><span class="p">()</span> <span class="o">==</span> <span class="n">PositionProviderStatusAvailable</span> <span class="p">)</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">return d->model()->positionTracking()->status() == PositionProviderStatusAvailable;</p></pre>
</div>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/124519/diff/1/?file=388506#file388506line347" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/marble/MarbleQuickItem.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">namespace Marble</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">347</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span> <span class="n">coordinates</span> <span class="o">==</span> <span class="n">GeoDataCoordinates</span><span class="p">()</span> <span class="p">)</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">can't we use <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">!coordinates.isValid()</code> here?</p></pre>
</div>
</div>
<br />
<p>- Dennis Nienhüser</p>
<br />
<p>On July 28th, 2015, 11:04 p.m. UTC, Gábor Péterffy 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.</div>
<div>By Gábor Péterffy.</div>
<p style="color: grey;"><i>Updated July 28, 2015, 11:04 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;">This patch introduces the CircleButton qml type. Based on this there is a button now which navigates the map at the current position. The button should be visible if the position is known.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I have also updated the icons. I think with using black borders for white icons we can handle both the dark and the light themes.</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;">It seems calling update() stops for the position providing plugin when I turn off the locationing -> It can not hide the button, because no signal has been emitted about status change. Any ideas?</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>data/android/drawable-xxxhdpi/locate.png <span style="color: grey">(PRE-CREATION)</span></li>
<li>data/android/drawable-xxxhdpi/search.png <span style="color: grey">(599a3c7ccdcedb11835378562f7f34c2a4c39669)</span></li>
<li>src/apps/marble-maps/CircleButton.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/apps/marble-maps/MainScreen.qml <span style="color: grey">(5552a54eca8b37fa17588a14a035927418b23fbe)</span></li>
<li>src/apps/marble-maps/MarbleMaps.qrc <span style="color: grey">(c24c38a507da4a8d41729d61fc23035d6f75a446)</span></li>
<li>src/lib/marble/MarbleQuickItem.h <span style="color: grey">(21b8fe5c4570ac894f668603a660da81f1d8a8e4)</span></li>
<li>src/lib/marble/MarbleQuickItem.cpp <span style="color: grey">(ee8bae8ea379cae3a0e6378259622a1ad88f8a2b)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/124519/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/28/d1e80eda-39e0-4c53-b09a-59b115d95785__search.png">search.png</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/07/28/8461a526-bfbf-4298-af71-e99616964e62__locate.png">locate.png</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/07/28/5b1f5fe0-d02d-415e-aa49-3cf20dedab0b__Screenshot_2015-07-29-00-49-51.png">Screenshot</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>