[Marble-devel] Review Request 118755: Refactors tour playback logic and adds basic interpolation of tours in Marble

[Dennis Nienhüser]

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


Can you please add copyright headers to all new files?


src/lib/marble/ParallelTrack.h
<https://git.reviewboard.kde.org/r/118755/#comment41853>

    const



src/lib/marble/ParallelTrack.cpp
<https://git.reviewboard.kde.org/r/118755/#comment41854>

    can be called on m_item directly without checking its type



src/lib/marble/PlaybackAnimatedUpdateItem.h
<https://git.reviewboard.kde.org/r/118755/#comment41855>

    const



src/lib/marble/PlaybackFlyToItem.h
<https://git.reviewboard.kde.org/r/118755/#comment41856>

    const GeoDataCoordinates &startCoordinates



src/lib/marble/PlaybackFlyToItem.h
<https://git.reviewboard.kde.org/r/118755/#comment41857>

    const



src/lib/marble/PlaybackFlyToItem.h
<https://git.reviewboard.kde.org/r/118755/#comment41858>

    const GeoDataCoordinates &coordinates



src/lib/marble/PlaybackFlyToItem.cpp
<https://git.reviewboard.kde.org/r/118755/#comment41859>

    m_isPlaying is not initialized



src/lib/marble/PlaybackFlyToItem.cpp
<https://git.reviewboard.kde.org/r/118755/#comment41860>

    Note that calling the method multiple times currently would screw things up. Maybe initialize m_isPlaying to false in the constructor and add a check here such that nothing happens if m_isPlaying is already true



src/lib/marble/PlaybackFlyToItem.cpp
<https://git.reviewboard.kde.org/r/118755/#comment41861>

    I'd add a Q_ASSERT( progress >= 0.0 ); Do we need to handle day switches here? Maybe we should use QDateTime instead to avoid that.



src/lib/marble/PlaybackItem.h
<https://git.reviewboard.kde.org/r/118755/#comment41862>

    const



src/lib/marble/PlaybackItem.h
<https://git.reviewboard.kde.org/r/118755/#comment41863>

    Seems unused to me



src/lib/marble/PlaybackItem.h
<https://git.reviewboard.kde.org/r/118755/#comment41864>

    const GeoDataCoordinates &coordinates



src/lib/marble/PlaybackItem.h
<https://git.reviewboard.kde.org/r/118755/#comment41865>

    what's the unit here? I'd indicate it in the variable name, e.g. double fraction or double seconds
    



src/lib/marble/PlaybackSoundCueItem.h
<https://git.reviewboard.kde.org/r/118755/#comment41866>

    const



src/lib/marble/PlaybackSoundCueItem.cpp
<https://git.reviewboard.kde.org/r/118755/#comment41867>

    seems wrong to me



src/lib/marble/PlaybackTourControlItem.h
<https://git.reviewboard.kde.org/r/118755/#comment41868>

    const



src/lib/marble/PlaybackWaitItem.h
<https://git.reviewboard.kde.org/r/118755/#comment41869>

    please remove, write-only and redundant to m_isPlaying



src/lib/marble/PlaybackWaitItem.cpp
<https://git.reviewboard.kde.org/r/118755/#comment41870>

    m_isPlaying is not initialized



src/lib/marble/PlaybackWaitItem.cpp
<https://git.reviewboard.kde.org/r/118755/#comment41871>

    same as flyto, this has problems with multiple play() calls in a row



src/lib/marble/SerialTrack.h
<https://git.reviewboard.kde.org/r/118755/#comment41872>

    const GeoDataCoordinates &coordinates



src/lib/marble/SerialTrack.h
<https://git.reviewboard.kde.org/r/118755/#comment41874>

    what is its purpose?



src/lib/marble/SerialTrack.h
<https://git.reviewboard.kde.org/r/118755/#comment41873>

    const GeoDataCoordinates &coordinates



src/lib/marble/SerialTrack.cpp
<https://git.reviewboard.kde.org/r/118755/#comment41876>

    could also spare the centerOnHandler slot and call
    connect( item, SIGNAL( centerOn(GeoDataCoordinates) ), this, SIGNAL( centerOn(GeoDataCoordinates) ) );



src/lib/marble/SerialTrack.cpp
<https://git.reviewboard.kde.org/r/118755/#comment41875>

    do we own them? then also qDeleteAll( m_items );



src/lib/marble/TourPlayback.h
<https://git.reviewboard.kde.org/r/118755/#comment41877>

    unused



src/lib/marble/TourPlayback.cpp
<https://git.reviewboard.kde.org/r/118755/#comment41878>

    i've seen this too often now... ;-)
    can you add a convenience method
    GeoDataAbstractView::coordinates() const which handles this?


- Dennis Nienhüser


On June 15, 2014, 8:09 a.m., Sanjiban Bairagya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118755/
> -----------------------------------------------------------
> 
> (Updated June 15, 2014, 8:09 a.m.)
> 
> 
> Review request for Marble.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> This patch refactors tour playback logic and adds basic interpolation of tours in Marble
> 
> 
> Diffs
> -----
> 
>   src/lib/marble/CMakeLists.txt 395e8bf 
>   src/lib/marble/ParallelTrack.h PRE-CREATION 
>   src/lib/marble/ParallelTrack.cpp PRE-CREATION 
>   src/lib/marble/PlaybackAnimatedUpdateItem.h PRE-CREATION 
>   src/lib/marble/PlaybackAnimatedUpdateItem.cpp PRE-CREATION 
>   src/lib/marble/PlaybackFlyToItem.h PRE-CREATION 
>   src/lib/marble/PlaybackFlyToItem.cpp PRE-CREATION 
>   src/lib/marble/PlaybackItem.h PRE-CREATION 
>   src/lib/marble/PlaybackItem.cpp PRE-CREATION 
>   src/lib/marble/PlaybackSoundCueItem.h PRE-CREATION 
>   src/lib/marble/PlaybackSoundCueItem.cpp PRE-CREATION 
>   src/lib/marble/PlaybackTourControlItem.h PRE-CREATION 
>   src/lib/marble/PlaybackTourControlItem.cpp PRE-CREATION 
>   src/lib/marble/PlaybackWaitItem.h PRE-CREATION 
>   src/lib/marble/PlaybackWaitItem.cpp PRE-CREATION 
>   src/lib/marble/SerialTrack.h PRE-CREATION 
>   src/lib/marble/SerialTrack.cpp PRE-CREATION 
>   src/lib/marble/TourPlayback.h 44a793f 
>   src/lib/marble/TourPlayback.cpp 03ab5f5 
>   src/lib/marble/TourWidget.h 31d3a59 
>   src/lib/marble/TourWidget.cpp 95d95b5 
>   src/lib/marble/TourWidget.ui 7a8a8ce 
> 
> Diff: https://git.reviewboard.kde.org/r/118755/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sanjiban Bairagya
> 
>

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