[Marble-devel] Review Request 114419: [GCI] Added support for starting KML tours from the legend

[Bernhard Beschow]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/114419/#review45666
-----------------------------------------------------------


Indeed, your code looks very clean already. Some minor nitpicks below.


src/lib/marble/MarbleLegendBrowser.cpp
<http://git.reviewboard.kde.org/r/114419/#comment32624>

    If you emit a signal as Dennis suggestes, you could also decouple MarbleLegendBrowser from TourWidget (simply remove m_tourWidget attribute & Co).



src/lib/marble/MarbleLegendBrowser.cpp
<http://git.reviewboard.kde.org/r/114419/#comment32623>

    Although you're checking for d->m_marbleWidget, you're not using it below. Hence, MarbleLegendBrowser doesn't need the m_marbleWidget attribute. Just remove it and its setter.



src/lib/marble/MarbleLegendBrowser.cpp
<http://git.reviewboard.kde.org/r/114419/#comment32622>

    You're requiring a "./" in the legned.html: "./" is really need.
    
    If you insert a "+ '/'" after head()->theme(), you don't need that requirement.


- Bernhard Beschow


On Dec. 12, 2013, 6:30 p.m., Mihail Ivchenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/114419/
> -----------------------------------------------------------
> 
> (Updated Dec. 12, 2013, 6:30 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> Added support for starting KML tours from the legend
> GCI task: http://www.google-melange.com/gci/task/view/google/gci2013/5796158626594816
> 
> Example of using:
> Put this into <legend></legend> element of .dgml:
>       <section name="tours" checkable="false" spacing="12">
> 	  <heading>Tours:</heading> 
> 	  <item name="geographic-pole">
> 	      <text><![CDATA[<a href="tour://./test_tour.kml">Test Tour</a>]]></text>   // "./" is really need. 
> 	  </item>
>       </section>
> 
> 
> Diffs
> -----
> 
>   src/apps/marble-mobile/MobileMainWindow.cpp 2eadff5 
>   src/apps/marble-ui/ControlView.cpp fc9022f 
>   src/lib/marble/LegendWidget.h e56966a 
>   src/lib/marble/LegendWidget.cpp 280fd64 
>   src/lib/marble/MarbleControlBox.cpp 900fa59 
>   src/lib/marble/MarbleLegendBrowser.h e7fb4b5 
>   src/lib/marble/MarbleLegendBrowser.cpp 644dd0f 
>   src/lib/marble/TourWidget.h c18c849 
>   src/lib/marble/TourWidget.cpp 8bd7aa6 
> 
> Diff: http://git.reviewboard.kde.org/r/114419/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Mihail Ivchenko
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20131213/370aa123/attachment.html>