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

Wed Jun 18 14:23:46 UTC 2014

-----------------------------------------------------------
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
> 
>

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