[Marble-devel] Review Request: Fix the timeChanged() signal emission timeout calculation in MarbleClock

[Bernhard Beschow]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104861/#review13530
-----------------------------------------------------------


The issues you raise should definetly be fixed. In addition, I expect a lot more issues coming up in MarbleClock, because a lot of code (e.g. SunPLugin, StarsPlugin, PlacemarkPositionProviderPlugin via SatellitesPlugin, clock in the status bar) depends on it. It gets even more complex when considering the "external" clocks Marble relies on, especially the remaining position provider plugins. It would therefore be helpful to underpin your fix with a unit test.

Do you see a way to achieve this?


src/lib/MarbleClock.cpp
<http://git.reviewboard.kde.org/r/104861/#comment10700>

    How should these values relate to one another? Couldn't a comparison with various cases be put into a unit test?


- Bernhard Beschow


On May 4, 2012, 10:43 p.m., Dennis Nienhüser wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104861/
> -----------------------------------------------------------
> 
> (Updated May 4, 2012, 10:43 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Description
> -------
> 
> MarbleClock currently rounds the internal clock to minutes. The implementation has rounding errors that result in the sleep timeout being calculated incorrectly and a fallback mechanism then sets it to one second. The same fallback mechanism handles negative speeds (time running backwards), which is wrong as well.
> 
> I'm not sure why the time is rounded to minutes. If it's done for aesthetic reasons, then it should rather be done in the UI code by setting the time once to the desired nice looking time. The timeout calculation however should stick to what is set from the outside. For a similar reason I'd like to remove the restriction that the timeout cannot be shorter than one second. If that is to be done for performance reasons, it should be done elsewhere (e.g. in the user interface where the clock settings can be manipulated). On my system I don't experience any problems though even when using the most extreme time settings (100x realtime at 1 second refresh = 100 fps) with the satellites plugin running.
> 
> The patch removes the rounding to minutes and timeout restriction. It also handles negative speeds correctly. This fixes the wrong signal emission timing.
> 
> 
> This addresses bug 299388.
>     http://bugs.kde.org/show_bug.cgi?id=299388
> 
> 
> Diffs
> -----
> 
>   src/lib/MarbleClock.cpp 56b2fe4 
> 
> Diff: http://git.reviewboard.kde.org/r/104861/diff/
> 
> 
> Testing
> -------
> 
> Custom application using
>     mapWidget->model()->clock()->setSpeed( 40 );
>     mapWidget->model()->clock()->setUpdateInterval( 20 );
> 
> and
> 
>     mapWidget->model()->clock()->setSpeed( 10 );
>     mapWidget->model()->clock()->setUpdateInterval( 20 );
> 
> and some other values.
> 
> Marble with satellites plugin and various time / refresh settings.
> 
> 
> Thanks,
> 
> Dennis Nienhüser
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20120507/63858b76/attachment.html>