[Marble-devel] Review Request 124519: Jump to current location button for Marble Maps

Sun Aug 2 17:20:00 UTC 2015

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124519/#review83334
-----------------------------------------------------------

We're nearly there. Mostly need to clarify the different signals and slots such that they behave as expected.

src/apps/marble-maps/MainScreen.qml (line 73)
<https://git.reviewboard.kde.org/r/124519/#comment57594>

    This is a bool property and the signal should only be emitted when its value changes from false to true or from true to false. When you change the positionAvailable() and updatePositionVisibility() implementations (see comments there), the update of the direction indicator will then stop working based on this signal (which is correct). Therefore we need another signal in MarbleQuickItem, e.g. viewportChanged, which can be directly connected to visibleLatLonAltBoxChanged from MarbleMap. Then this method can be triggered as `onViewportChanged: {...}`.

src/lib/marble/declarative/MarbleQuickItem.h (line 169)
<https://git.reviewboard.kde.org/r/124519/#comment57585>

    Signals should have a passive name, slots an active. That way code is easier to read. Let's call it `updatePositionVisibility`

src/lib/marble/declarative/MarbleQuickItem.cpp (line 93)
<https://git.reviewboard.kde.org/r/124519/#comment57586>

    This is not needed. No touch event will change the ego position, and all kinds of map panning and zooming will result in a visibleLatLonAltBoxChange signal.

src/lib/marble/declarative/MarbleQuickItem.cpp (line 181)
<https://git.reviewboard.kde.org/r/124519/#comment57587>

    should call updatePositionVisiblity instead (the current visibleLatLonAltBoxChanged slot)

src/lib/marble/declarative/MarbleQuickItem.cpp (line 196)
<https://git.reviewboard.kde.org/r/124519/#comment57588>

    should call updatePositionVisiblity instead (the current visibleLatLonAltBoxChanged slot)

src/lib/marble/declarative/MarbleQuickItem.cpp (line 199)
<https://git.reviewboard.kde.org/r/124519/#comment57592>

    This should be renamed updatePositionVisibility and have the current implementation of positionVisible() (see also comment there)

src/lib/marble/declarative/MarbleQuickItem.cpp (line 341)
<https://git.reviewboard.kde.org/r/124519/#comment57591>

    This should return d->m_positionVisible and do nothing else. Otherwise caching d->m_positionVisible makes no sense.

src/lib/marble/declarative/MarbleQuickItem.cpp (line 343)
<https://git.reviewboard.kde.org/r/124519/#comment57589>

    for code readability I'd prefer renaming it isVisible

src/lib/marble/declarative/MarbleQuickItem.cpp (line 407)
<https://git.reviewboard.kde.org/r/124519/#comment57584>

    Bounding boxes aren't the only thing we can center on. Just use `d->centerOn(coordinates);` as a replacement for the four lines here.

- Dennis Nienhüser

On Aug. 2, 2015, 3:36 p.m., Gábor Péterffy wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124519/
> -----------------------------------------------------------
> 
> (Updated Aug. 2, 2015, 3:36 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> This patch introduces the CircleButton qml type. Based on this there is a button now which navigates the map at the current position.
> 
> 
> Diffs
> -----
> 
>   data/android/drawable-xxxhdpi/gps_not_fixed.png PRE-CREATION 
>   data/android/drawable-xxxhdpi/marblelogo.png bf462f7608169f70cc014d8da1e0ad86e11bed15 
>   src/apps/marble-maps/BoxedText.qml PRE-CREATION 
>   src/apps/marble-maps/CircularButton.qml PRE-CREATION 
>   src/apps/marble-maps/MainScreen.qml ce36f04c61764c7d813608f3bb7aeb671731c5a4 
>   src/apps/marble-maps/MarbleMaps.qrc 2978e72c2fd12fe8435cfb3d5e5559982b41b110 
>   src/apps/marble-maps/PositionButton.qml PRE-CREATION 
>   src/lib/marble/declarative/MarbleQuickItem.h cca329761f23cd72315df2d359166a2cbfa8056c 
>   src/lib/marble/declarative/MarbleQuickItem.cpp 28f599c3b51c7bf993e40c6c97265f351fe02f17 
>   src/plugins/positionprovider/qtpositioning/QtPositioningPositionProviderPlugin.cpp 1a11d67fba96f328a0a5aba7545fa4e93c597964 
>   data/android/drawable-xxxhdpi/backdrop.png PRE-CREATION 
>   data/android/drawable-xxxhdpi/gps_fixed.png PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/124519/diff/
> 
> 
> Testing
> -------
> 
> It seems my distance or angle functions are not working well, the almost always provide false data after some panning (my position is sometimes start to change when I am panning on the map, strange thing: the position marker also starts to go to an other place like it is fixed to the screen, but the map moves under it sometimes when I am penning and it is not jumping back to its place.)
> 
> 
> File Attachments
> ----------------
> 
> gps_not_fixed.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/07/30/35409caf-2dbd-4fc4-b495-43fe1d8e4279__gps_not_fixed.png
> gps_fixed.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/07/30/019abdbb-80cf-4935-8d6d-0c3e6927d2b3__gps_fixed.png
> marblelogo.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/07/31/ebaa4d45-b337-4e42-8a23-a73e2d88ddca__marblelogo.png
> backdrop.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/07/31/dacf5958-7270-40c4-9031-243350a80de8__backdrop.png
> 
> 
> Thanks,
> 
> Gábor Péterffy
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20150802/8cb31dfa/attachment-0001.html>