----------------------------------------------------------- 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 > >
_______________________________________________ Marble-devel mailing list Marble-devel@kde.org https://mail.kde.org/mailman/listinfo/marble-devel
