[Marble-devel] Review Request 124327: Search Bar has been added for Marble Maps

[Dennis Nienhüser]

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



src/apps/marble_maps/SearchBar.qml (line 22)
<https://git.reviewboard.kde.org/r/124327/#comment57015>

    I don't think we need z-values in here at all.



src/apps/marble_maps/SearchBar.qml (line 26)
<https://git.reviewboard.kde.org/r/124327/#comment57019>

    height should be searchButton.height
    
    However I'd really prefer anchors to width/height calculations. It's much easier to maintain. The effect you want is visually like this
    |  text input       |O|
    with O the button
    
    Try setting up anchors like this:
    inputField.left => parent.left
    inputField.right => searchButton.left
    searchButton.right => parent.right
    and finally a proper width for searchButton.
    
    This way all information is there needed to calculate the (horizontal) positions and the width of both elements, and there is no redundant information. You can also easily introduce margins (anchors.margins or anchors.marginLeft, ...) for fine-tuning. See http://doc.qt.io/qt-5/qtquick-positioning-anchors.html for details. http://doc.qt.io/qt-5/qtquick-positioning-topic.html is a good read also for the more general concepts.



src/apps/marble_maps/SearchBar.qml (line 69)
<https://git.reviewboard.kde.org/r/124327/#comment57014>

    I think using anchors+margins in inputField would be nicer and achieves the same result (see above)



src/apps/marble_maps/SearchBar.qml (line 82)
<https://git.reviewboard.kde.org/r/124327/#comment57013>

    I wonder if we should introduce some common ColorPalette.qml or similar, which has colors as named properties. Makes it much easier to read and also to maintain.



src/apps/marble_maps/SearchBarBackend.h (line 48)
<https://git.reviewboard.kde.org/r/124327/#comment57011>

    Seems unused



src/apps/marble_maps/SearchBarBackend.cpp (line 20)
<https://git.reviewboard.kde.org/r/124327/#comment57012>

    also initialize m_placemarkModel to nullptr here



src/apps/marble_maps/SearchBarBackend.cpp (line 64)
<https://git.reviewboard.kde.org/r/124327/#comment57010>

    I'd delete m_searchManager before (just in case the method is called more than once)



src/plugins/runner/openrouteservice/OpenRouteServiceRunner.cpp 
<https://git.reviewboard.kde.org/r/124327/#comment57020>

    Can you push this and the similar change in YoursRunner.cpp directly to master (no review request needed)?


- Dennis Nienhüser


On July 19, 2015, 3:44 p.m., Gábor Péterffy wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124327/
> -----------------------------------------------------------
> 
> (Updated July 19, 2015, 3:44 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> This patch adds a search bar to Marble Maps. Because the loading of the plugins not works on Android, the search function is not working.
> 
> 
> Diffs
> -----
> 
>   data/android/pixmaps/searchFocused.png PRE-CREATION 
>   data/android/pixmaps/searchPressed.png PRE-CREATION 
>   data/android/pixmaps/searchReleased.png PRE-CREATION 
>   src/apps/marble_maps/CMakeLists.txt PRE-CREATION 
>   src/apps/marble_maps/MainScreen.qml PRE-CREATION 
>   src/apps/marble_maps/MarbleMaps.qrc PRE-CREATION 
>   src/apps/marble_maps/QmlView.h PRE-CREATION 
>   src/apps/marble_maps/QmlView.cpp PRE-CREATION 
>   src/apps/marble_maps/SearchBar.qml PRE-CREATION 
>   src/apps/marble_maps/SearchBarBackend.h PRE-CREATION 
>   src/apps/marble_maps/SearchBarBackend.cpp PRE-CREATION 
>   src/apps/marble_maps/main.cpp PRE-CREATION 
>   src/lib/marble/MarblePlacemarkModel.cpp a2c78c4f8603752182f64e42e02a585d5ea6b2e3 
>   src/lib/marble/MarbleQuickItem.h 41e20bb69bbeb9c1d662cd943ecb13674fd04498 
>   src/plugins/runner/openrouteservice/OpenRouteServiceRunner.cpp 7c1803998ce5a01e338f2eb04a2b7f310a926c26 
>   src/plugins/runner/yours/YoursRunner.cpp 4b1c0552718acc4a2f3070e95950975a57d4fdd4 
> 
> Diff: https://git.reviewboard.kde.org/r/124327/diff/
> 
> 
> Testing
> -------
> 
> On linux it works fine when the runner plugins are loaded. On Android the UI appears fine but not working with loaded plugins.
> 
> 
> File Attachments
> ----------------
> 
> Screenshot
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/07/13/95493cb2-c43c-48f4-a1f0-7a5ca0486856__Screenshot_2015-07-13-17-57-06.png
> Screenshot 2
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/07/13/ab43529f-26f4-4f52-9ba5-1c759fcee07d__Screenshot_2015-07-13-17-56-45.png
> Screenshot 3
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/07/13/b6c76a6b-9118-4841-97e7-c204567454b8__Screenshot_2015-07-13-17-40-46.png
> searchFocused.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/07/19/4fc5e41a-40aa-4d62-879c-6436e93dd1a0__searchFocused.png
> searchPressed.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/07/19/c16c83c4-d25f-482d-b409-7cece2f7bad0__searchPressed.png
> searchReleased.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/07/19/58b3d964-641d-457f-911e-62066d55cf61__searchReleased.png
> 
> 
> Thanks,
> 
> Gábor Péterffy
> 
>

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