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

Sat Aug 1 09:09:56 UTC 2015

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

src/apps/marble-maps/BoxedText.qml (line 10)
<https://git.reviewboard.kde.org/r/124519/#comment57503>

    What about calling it simply text? (might need to rename Text {id: text } below then to avoid conflicts)

src/apps/marble-maps/BoxedText.qml (line 22)
<https://git.reviewboard.kde.org/r/124519/#comment57515>

    I guess the intent here is that
    `18 pt = 18 * 1/72 inch = 6.35 mm` is about the same as root.height (6.5 mm) above? That doesn't work well on my device (Nexus 4), the text fills only about 50% of the height of the box. As discussed in IRC the other day pointSize is not really the height of text.

src/apps/marble-maps/BoxedText.qml (line 23)
<https://git.reviewboard.kde.org/r/124519/#comment57516>

    there should be some small margins to the left and to the right between the text and the background box.

src/apps/marble-maps/CircularButton.qml (line 9)
<https://git.reviewboard.kde.org/r/124519/#comment57517>

    should this be read-only?
    `readonly property int diameter: ...`

src/apps/marble-maps/CircularButton.qml (line 16)
<https://git.reviewboard.kde.org/r/124519/#comment57518>

    should this be `root.diameter`? Same for `height`, and `radius: root.diameter / 2`?

src/apps/marble-maps/CircularButton.qml (line 40)
<https://git.reviewboard.kde.org/r/124519/#comment57519>

    `0.6 * root.diameter`?

src/apps/marble-maps/CircularButton.qml (line 63)
<https://git.reviewboard.kde.org/r/124519/#comment57520>

    `width: diameter`
    `height: diameter`

src/apps/marble-maps/IndicatorCircularButton.qml (line 10)
<https://git.reviewboard.kde.org/r/124519/#comment57521>

    I'd prefer `showDirection`

src/apps/marble-maps/IndicatorCircularButton.qml (line 20)
<https://git.reviewboard.kde.org/r/124519/#comment57522>

    should be `root.diameter`, right? Same for `height`.

src/apps/marble-maps/IndicatorCircularButton.qml (line 25)
<https://git.reviewboard.kde.org/r/124519/#comment57514>

    what about using `indicator.width / 2` and `indicator.height / 2` below? The rotation axis is always the item's center, so you can avoid the redundancy here.

src/apps/marble-maps/IndicatorCircularButton.qml (line 46)
<https://git.reviewboard.kde.org/r/124519/#comment57523>

    Help me with this one.

    ```
      Screen.pixelDensity * 9 * 1.41
    = root.diameter * sqrt(2)
    = sqrt(root.diameter*root.diameter*2)
    = sqrt(r.d*r.d+r.d*.r.d)
    ```
    So this is the length of the diagonal of the square with side length root.diameter. Are you trying to square the circle? :-)

    The effect is that you get some kind of automatic margins to the screen bottom right, but I'd prefer doing that the classic way. So width should be root.diameter and in MainScreen.qml where this item is created let's specify explicit margins to the right and bottom. Then we can also use a larger margin to the bottom to avoid an overlap with the license text (I enabled that one yesterday, you might have to update to latest master to see it).

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

    Shall we shorten this to IndicatorButton? Or PositionButton to hint better at what it is?

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

    What about using `showIndicator: !marbleMaps.positionVisible` directly and have QML do the work (instead of setting it manually in onPositionVisibleChanged())

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

    Similarly I'd use `visible: !marbleMaps.positionVisible` here

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

    const GeoDataLatLonAltBox &
    You could also just remove the parameter, same for the two parameters of positionChanged since you don't use them. Qt's handles it fine if you connect a signal with n parameters to one with m<n parameters, the spare ones are just ignored. Just need to remove them from the connect statement accordingly.

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

    Shouldn't we cache the value in a boolean member and only emit the signal if it really has changed? Most often when the position has changed  the visibility on the screen will not change and we should avoid unnecessary signal generation and binding re-evaluation.

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

    Same as above. This is even more important because visibleLatLonAltBoxChanged will be called very often (e.g. hundreds of times every time you pan the map a bit)

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

    qreal lon2 =
    qreal lat2 =
    and remove the unitialized variables above.

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

    No need to set up a new viewport, just use the existing one from map()->viewport() which is also correctly shifted, such that position equals (x2, y2). The whole method can then be shortened to

    ```
    qreal MarbleQuickItem::angleFromPointToCurrentLocation( const QPoint & position ) const
    {
        if ( positionAvailable() ) {
            qreal x, y;
            PositionTracking const * positionTracking = d->model()->positionTracking();
            map()->viewport()->screenCoordinates( positionTracking->currentLocation(), x, y );
            return atan2( y-position.y(), x-position.x() ) * RAD2DEG;
        }
        return 0;
    }
    ```

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

    I don't really like the behavior here from an end-users point of view. No matter which zoom value I have at the moment it overwrites it with this rather arbitrary one. I'd rather check the current zoom value here. If it is low, center on the current position and choose a high zoom value. Otherwise only center on the current position and keep the current zoom value.

One larger issue (we should look at outside of the review request) is that GPS stays active now when Marble is in the background. This will drain the battery, and we need to be able to operate in several modes: During tracking or turn-by-turn navigation GPS needs to stay on, but for regular map browsing etc it should go off once Marble gets suspended.

- Dennis Nienhüser

On July 31, 2015, 11:23 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 July 31, 2015, 11:23 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/backdrop.png PRE-CREATION 
>   data/android/drawable-xxxhdpi/gps_fixed.png PRE-CREATION 
>   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/IndicatorCircularButton.qml PRE-CREATION 
>   src/apps/marble-maps/MainScreen.qml 2cc996aef7b89f888f822ef4ff3bf5460c729991 
>   src/apps/marble-maps/MarbleMaps.qrc d027af982f31144107baedf77fe07f7239ef53ed 
>   src/lib/marble/declarative/MarbleQuickItem.h cca329761f23cd72315df2d359166a2cbfa8056c 
>   src/lib/marble/declarative/MarbleQuickItem.cpp e7dbbe7de31858286f6d0e38f5721f7f8ae45e79 
>   src/plugins/positionprovider/qtpositioning/QtPositioningPositionProviderPlugin.cpp 35884a7c844b590ecf44f3346f0d41b4c46b0f81 
> 
> 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/20150801/930ff598/attachment-0001.html>