----------------------------------------------------------- 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 > >
_______________________________________________ Marble-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/marble-devel
