[Marble-devel] Review Request 117057: Progress slider for KML tours

[Dennis Nienhüser]

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


When testing with the moon tour (open the Moon (clementine) map theme, open the Legend panel, click on "Play a tour of different landing sites"), playback starts including the background music, but a seek attempt results in lots of

[0xa960d870] access_http access error: seek failed
[0xa960d870] main access error: connection failed: Network is unreachable
[0xa960d870] access_http access error: cannot connect to ftp5.gwdg.de:80



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

    setupProgressBar (no capital U)



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

    Can you change it to a proper class with getters/setters that introduce some semantics to make the code easier readable?
    
    e.g.
    
    class PositionInfo
    {
      // getter & setter methods here
    
    private:
      double m_position; // pair.first
      double m_duration; // pair.second.first
      int m_index; // pair.second.second
    };
    



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

    The method does not seem to be prepared to be called multiple times. Each time it's called it will connect signals/slots another time. Also the member variables should be reset.



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

    This pattern (foreach timer... stop) is repeated three times, can you extract a method for it?



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

    What's the intent here? Wait for playback to start? It looks like a hack.



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

    m_tourUi.m_slider->blockSignals(true);
    m_tourUi.m_slider->setValue( position );
    m_tourUi.m_slider->blockSignals(false);
    
    Maybe listening to valueChanged(int) can be avoided at all? I don't see where it's used currently really.
    


- Dennis Nienhüser


On March 25, 2014, 2:42 p.m., Cruceru Calin-Cristian wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117057/
> -----------------------------------------------------------
> 
> (Updated March 25, 2014, 2:42 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Bugs: 332332
>     http://bugs.kde.org/show_bug.cgi?id=332332
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> I adapted an older patch for adding a progress slider in KML tours (  https://git.reviewboard.kde.org/r/114531/ ) to the current version. I also updated the Tour.kml file from tests/data for better testing.
> 
> I think it works as expected ( regarded to that patch ) but there are still some improvements to be done. So please open some issues with what should be modified or added.
> 
> 
> Diffs
> -----
> 
>   src/lib/marble/TourPlayback.h 44a793f 
>   src/lib/marble/TourPlayback.cpp 03ab5f5 
>   src/lib/marble/TourWidget.h c38d620 
>   src/lib/marble/TourWidget.cpp 6797530 
>   src/lib/marble/TourWidget.ui 7a8a8ce 
>   tests/data/Tour.kml d9fc9ed 
> 
> Diff: https://git.reviewboard.kde.org/r/117057/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Cruceru Calin-Cristian
> 
>

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