[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/#review82396
-----------------------------------------------------------



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

    shouldn't this rather use the height of text?



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

    why not use anchors and margins, and place it inside backgroundBar (as a child)?



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

    i'd rather have the icon (Image) set the size



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

    I'd prefer using a property for Image.source. The property itself can listen to signals of the searchButton to change its values. This allows QML to do all updates on its own.
    (just a thought, there might be blockers)



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

    I think you can replace indexAt(...) with a simple index
    The index property should be setup automatically by QML for all delegate items



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

    `const QString &place` should work as well, I think



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

    A const reference should be possible here also



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

    const reference



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

    must initialize m_marbleQuickItem to 0 (better yet: nullptr) here



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

    qvariant_cast is preferred for QObjects



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

    We also have a `MarblePlacemarkModel` which might be displayable by QML without any conversion like below. Did you give that a try yet? There's a similar signal `searchResultChanged(MarblePlacemarkModel*)`  that passes it.


- Dennis Nienhüser


On July 12, 2015, 12:56 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 12, 2015, 12:56 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/searchUnpressed.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/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/11/a670df37-4e21-42c0-880a-5e38ba94cc4b__Screenshot_2015-07-11-13-12-22.png
> Screenshot 2
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/07/12/5c0b8ef4-603a-4f87-be56-3dab9d1c71c3__Screenshot_2015-07-12-14-52-34.png
> 
> 
> Thanks,
> 
> Gábor Péterffy
> 
>

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