----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118755/#review60404 -----------------------------------------------------------
We're getting there, works quite nice already. src/lib/marble/ParallelTrack.cpp <https://git.reviewboard.kde.org/r/118755/#comment42161> m_delayToTourStart is not initialized src/lib/marble/PlaybackAnimatedUpdateItem.cpp <https://git.reviewboard.kde.org/r/118755/#comment42160> I'd add comments /** @todo Implement */ here to indicate the yet missing implementation src/lib/marble/PlaybackFlyToItem.h <https://git.reviewboard.kde.org/r/118755/#comment42159> unused src/lib/marble/PlaybackFlyToItem.cpp <https://git.reviewboard.kde.org/r/118755/#comment42158> whitespace near brackets missing here and below src/lib/marble/PlaybackFlyToItem.cpp <https://git.reviewboard.kde.org/r/118755/#comment42157> center( t ); and remove the Quaternion... above src/lib/marble/PlaybackFlyToItem.cpp <https://git.reviewboard.kde.org/r/118755/#comment42156> I'd reduce 20 to something smaller, otherwise we limit even the fastest system to 50 fps. 5 or 10 sounds good. src/lib/marble/PlaybackSoundCueItem.h <https://git.reviewboard.kde.org/r/118755/#comment42155> we'll have to guard the phonon objects with #ifdef HAVE_PHONON again to have phonon an optional dependency src/lib/marble/PlaybackTourControlItem.cpp <https://git.reviewboard.kde.org/r/118755/#comment42152> personally I write a comment // nothing to do in empty methods and empty loop bodies to indicate that it is not a missing implementation, but intentional. src/lib/marble/PlaybackWaitItem.cpp <https://git.reviewboard.kde.org/r/118755/#comment42150> lots of whitespace missing near brackets in this and the next lines src/lib/marble/PlaybackWaitItem.cpp <https://git.reviewboard.kde.org/r/118755/#comment42139> for safety I'd call stop() first before emitting the signal src/lib/marble/SerialTrack.h <https://git.reviewboard.kde.org/r/118755/#comment42134> private src/lib/marble/SerialTrack.h <https://git.reviewboard.kde.org/r/118755/#comment42137> int size() const src/lib/marble/SerialTrack.h <https://git.reviewboard.kde.org/r/118755/#comment42147> nicer name: progressChanged (see below) src/lib/marble/SerialTrack.h <https://git.reviewboard.kde.org/r/118755/#comment42145> changeProgress would be a nicer name. Rule of thumb for signal and slot names: - passive form for signals (e.g. fooChanged, fooActivated) - active form for slots (e.g. handleFoo, updateBar) - whenever you see people putting 'emit', 'slot', 'signal' in their signal/slot names, cringe and don't imitate src/lib/marble/SerialTrack.cpp <https://git.reviewboard.kde.org/r/118755/#comment42135> m_paused is not initialized src/lib/marble/SerialTrack.cpp <https://git.reviewboard.kde.org/r/118755/#comment42140> shouldn't this propagate back to TourPlayback and TourWidget and be reflected in the play/pause button state? Such that the user can continue the tour with a click on play. src/lib/marble/SerialTrack.cpp <https://git.reviewboard.kde.org/r/118755/#comment42133> Crashes here when index=-1 (seeking to the very end of the tour) src/lib/marble/TourWidget.h <https://git.reviewboard.kde.org/r/118755/#comment42126> handleSliderMove would be easier to understand src/lib/marble/TourWidget.cpp <https://git.reviewboard.kde.org/r/118755/#comment42125> the name sounds like a signal, but it is a slot. please rename handlePlaybackProgress src/lib/marble/TourWidget.ui <https://git.reviewboard.kde.org/r/118755/#comment42131> can we go for a minimalistic user interface? just one (checkable) button which toggles between play() and pause(), no stop button and the play button and the slider next to each other horizontally. src/lib/marble/geodata/data/GeoDataAbstractView.h <https://git.reviewboard.kde.org/r/118755/#comment42123> please move to the .cpp - Dennis Nienhüser On June 17, 2014, 10:16 p.m., Sanjiban Bairagya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/118755/ > ----------------------------------------------------------- > > (Updated June 17, 2014, 10:16 p.m.) > > > Review request for Marble, Dennis Nienhüser and Torsten Rahn. > > > Repository: marble > > > Description > ------- > > This patch refactors tour playback logic and adds basic interpolation of > tours in Marble > > > Diffs > ----- > > src/lib/marble/CMakeLists.txt 4987c90 > 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 > src/lib/marble/geodata/data/GeoDataAbstractView.h 0f3f13c > src/lib/marble/geodata/data/GeoDataAbstractView.cpp 32867ba > > 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
