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

Wed Jul 29 07:31:42 UTC 2015

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

The patch doesn't apply, can you update to latest master and redo it?

src/apps/marble-maps/CircleButton.qml (line 5)
<https://git.reviewboard.kde.org/r/124519/#comment57393>

    I wonder if there shouldn't be a parent Item { } such that the Rectangle is not exposed and becomes an implementation detail.

src/apps/marble-maps/CircleButton.qml (line 8)
<https://git.reviewboard.kde.org/r/124519/#comment57392>

    I'd rather name it diameter. Or use radius directly.

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

    I'd rather do
    `property alias iconSource: icon.iconSource`

src/apps/marble-maps/CircleButton.qml (line 35)
<https://git.reviewboard.kde.org/r/124519/#comment57395>

    Wouldn't it be nicer to use a state for such things? Like this:

    ```
    Item {
      id: root

      property alias iconSource:  icon.iconSource

      ...

      Rectangle { id: background; ... }

      State {
        name: "pressed"
        when: touchHandler.pressed
        PropertyChanges {
          target: background
          color: palette.highlight
        }
      }
    }
    ```

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

    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.

    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

    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 (`size: 9 * Screen.pixelDensity`)
    - margins should be 2 * Screen.pixelDensity

    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.

    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

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

    what's nicer - visible or enabled? not sure.

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

    return d->model()->positionTracking()->status() == PositionProviderStatusAvailable;

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

    can't we use `!coordinates.isValid()` here?

- Dennis Nienhüser

On July 28, 2015, 11:04 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 28, 2015, 11:04 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. The button should be visible if the position is known.
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   data/android/drawable-xxxhdpi/locate.png PRE-CREATION 
>   data/android/drawable-xxxhdpi/search.png 599a3c7ccdcedb11835378562f7f34c2a4c39669 
>   src/apps/marble-maps/CircleButton.qml PRE-CREATION 
>   src/apps/marble-maps/MainScreen.qml 5552a54eca8b37fa17588a14a035927418b23fbe 
>   src/apps/marble-maps/MarbleMaps.qrc c24c38a507da4a8d41729d61fc23035d6f75a446 
>   src/lib/marble/MarbleQuickItem.h 21b8fe5c4570ac894f668603a660da81f1d8a8e4 
>   src/lib/marble/MarbleQuickItem.cpp ee8bae8ea379cae3a0e6378259622a1ad88f8a2b 
> 
> Diff: https://git.reviewboard.kde.org/r/124519/diff/
> 
> 
> Testing
> -------
> 
> 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?
> 
> 
> File Attachments
> ----------------
> 
> search.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/07/28/d1e80eda-39e0-4c53-b09a-59b115d95785__search.png
> locate.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/07/28/8461a526-bfbf-4298-af71-e99616964e62__locate.png
> Screenshot
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/07/28/5b1f5fe0-d02d-415e-aa49-3cf20dedab0b__Screenshot_2015-07-29-00-49-51.png
> 
> 
> Thanks,
> 
> Gábor Péterffy
> 
>

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