D14939: Making more recommended xplanet changes and fixing bugs:

[Robert Lancaster]

lancaster marked an inline comment as done.
lancaster added inline comments.

INLINE COMMENTS

> pino wrote in xplanetimageviewer.cpp:688
> D14813: Adding XPlanet Viewer <https://phabricator.kde.org/D14813> adds this option... as string?! if it's a number, then just use the proper type, i.e. `Int`.
> 
> Also, what is the use case to make this configurable? Why should users change/edit it?
> Ideally, kstars should just use a proper timeout on its own, possibly doing some estimation based on the data provided to xplanet (i.e. wait more if requesting a bigger image).

Yes, you are correct, it should be int.

I think it is important to make it configurable because I have found that xplanet can take anywhere from a few milliseconds to several seconds to complete.  The time is not just dependent on the image size, a lot of it is due to the complexity of a rendering.  Jupiter is often very quickly rendered, except when it has a transit of one of its moons, then xplanet takes significantly longer to complete.  And we don't know when such events will occur in KStars currently.  The other issue would be computers, some computers could take longer to render complex images.  I think the user should be allowed to set the timeout based on personal preference.  They might want to give a complicated conjunction or transit time to render.  Or they might want it to stop before it spends too long trying to calculate a complicated one because it slows down their animation.

> pino wrote in xplanetimageviewer.cpp:708-712
> This is synchronous, and ugly.
> 
> Instead, use the signal of QFutureWatcher to know when it finishes. To implement a timeout, use a separate QTimer, to start when starting the computation, and stop when the future finishes -- if the timer fires, then cancel the future.

True, that would probably be better

> pino wrote in xplanetimageviewer.cpp:749
> fd is leaked. Most probably you want to set it for the QFile object? (see `QFile::open()` that takes a fd.

Yeah, I wasn't sure what to do with that.  It was the way it was in server manager.cpp in KStars.  I can re-examine it.

> mutlaqja wrote in opsxplanet.ui:431
> As noted previously, Requires Maps --> requires maps
> 
> Also at least on Linux, there is xplanet-images package, is there more to it than that?

Yes, the user can put any map they want to into this directory.   The xplanet-images package you mention is really not adequate.  It doesn't include most of the planets let alone the moons.  I provided a link to the maps recommended by xplanet.  For the MacOS version, I download the ones from from that page under flat planet and include the distributable planet images.  The package is called alien.  But they are not necessarily the best ones, just the ones I liked.  You can install any you like.

REPOSITORY
  R321 KStars

REVISION DETAIL
  https://phabricator.kde.org/D14939

To: lancaster, yurchor, pino, mutlaqja, kde-edu
Cc: kde-edu, mutlaqja, pino, yurchor, narvaez, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20180820/4a41584c/attachment.html>