[Marble-devel] Review Request 114343: [GCI] Added support for basic KML Tour modifications.

[Dennis Nienhüser]

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


Great work! I'd like to see two changes (besides the one pointed out below):
- Add src/lib/marble/libmarble.qrc as resource file and select icons from it for the buttons: list-add, go-up, go-down, list-remove. I just added them all to Marble, make sure to update your git checkout.
- Add more checks for an invalid/empty rootIndex and only use static_cast where you can be sure that it succeeds. Possibly add a method to retrieve a GeoDataObject* from the root index to make that easier, which returns 0 if the index is invalid. Something like this:

GeoDataObject* rootIndexObject() const
{
  QModelIndex const rootIndex = m_tourUi.m_listView->rootIndex();
  return rootIndex.isValid() ? static_cast<GeoDataObject*>( rootIndex.internalPointer() ) : 0;
}

This is needed in getPlaylistFeature() at least. The other casts seem safe to me, although I wouldn't mind them using the rootIndexObject() method as well.



src/lib/marble/TourWidget.cpp
<http://git.reviewboard.kde.org/r/114343/#comment32362>

    Given that it is a slot I'd rename it from something passive to something active which describes what's happening: updateButtonStates()



src/lib/marble/TourWidget.cpp
<http://git.reviewboard.kde.org/r/114343/#comment32364>

    Better "Remove Selected Items" (capitalized cause it is a title)



src/lib/marble/TourWidget.cpp
<http://git.reviewboard.kde.org/r/114343/#comment32363>

    Crashes here when no .kml file has been added yet. I think the add button should only be enabled when a .kml file has been loaded


- Dennis Nienhüser


On Dec. 7, 2013, 2:16 p.m., Mihail Ivchenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/114343/
> -----------------------------------------------------------
> 
> (Updated Dec. 7, 2013, 2:16 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> Added support for basic KML Tour modifications.
> GCI task: http://www.google-melange.com/gci/task/view/google/gci2013/5903763663486976
> 
> 
> Diffs
> -----
> 
>   src/lib/marble/TourWidget.h abd7276 
>   src/lib/marble/TourWidget.cpp 14e2f41 
>   src/lib/marble/TourWidget.ui 38d80b1 
>   src/lib/marble/geodata/data/GeoDataPlaylist.h 8b0e411 
>   src/lib/marble/geodata/data/GeoDataPlaylist.cpp dc85421 
> 
> Diff: http://git.reviewboard.kde.org/r/114343/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Mihail Ivchenko
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20131207/50902bea/attachment.html>