[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/#review60466
-----------------------------------------------------------



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

    no need for this #include



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

    did you mean to write m_delayBeforeTrackStarts = 0?



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

    please remove (don't change the playback state in seek). Sound currently starts playing in pause mode when you seek.



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

    please remove (don't change the playback state in seek)



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

    please remove (don't change the playback state in seek)



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

    #include "config-phonon.h"



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

    I'd use
    #else
      return 0;
    #endif
    
    here to avoid that compilers/code checkers see unreachable code



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

    m_mediaObject->stop();



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

    Will only be the case if m_items is empty. We should test that case (tour with e.g. just sound) also, it might lead to a crash currently.



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

    remove



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

    remove



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

    private



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

    should be private (or remove completely)



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

    not needed anymore here



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

    please remove the phonon includes here, not used anymore



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

    not needed



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

    unused, remove



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

    this check is not needed anymore. You can simply do
    d->m_mainTrack->append( new PlaybackFlyToItem( flyTo ) );
    always



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

    I'd work directly on d->m_parallelTracks



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

    the button should not both be checked and switch its icon (that's confusing), just one of both:
    
    Variant A)
    button is not checkable
    in play mode: show pause icon
    in pause mode: show play icon
    
    Variant B)
    play-icon, button is checkable
    in play mode: checked
    in pause mode: not checked
    
    Variant A) is more popular nowadays, so I'd go for that
    



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

    did you mean to execute this on actionPlay?


- Dennis Nienhüser


On June 19, 2014, 3:08 a.m., Sanjiban Bairagya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118755/
> -----------------------------------------------------------
> 
> (Updated June 19, 2014, 3:08 a.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/ParallelTrack.h PRE-CREATION 
>   src/lib/marble/CMakeLists.txt 4987c90 
>   src/apps/marble-ui/ControlView.cpp 84ed594 
>   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/20140619/c2f4ee3e/attachment-0001.html>