D14939: Making more recommended xplanet changes and fixing bugs:

[Pino Toscano]

pino requested changes to this revision.
pino added a comment.
This revision now requires changes to proceed.


  I have more notes, but no time right now.

INLINE COMMENTS

> xplanetimageviewer.cpp:688
> +
> +    int timeout = Options::xplanetTimeout().toInt();
> +

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

> xplanetimageviewer.cpp:708-712
> +                while(fifoImageLoadWatcher.isRunning() && time < timeout)
> +                {
> +                    time += 10;
> +                    QThread::msleep(10);
> +                }

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.

> xplanetimageviewer.cpp:749
> +            int fd =0;
> +            if ((fd = mkfifo(file.fileName().toLatin1(), S_IRUSR | S_IWUSR) < 0))
> +            {

fd is leaked. Most probably you want to set it for the QFile object? (see `QFile::open()` that takes a fd.

> xplanetimageviewer.h:148-155
> +    typedef enum {
> +    SUN,MERCURY,VENUS,
> +    EARTH,MOON,
> +    MARS,PHOBOS,DEIMOS,
> +    JUPITER,GANYMEDE,IO,CALLISTO,EUROPA,
> +    SATURN,TITAN,MIMAS,ENCELADUS,TETHYS,DIONE,RHEA,HYPERION,IAPETUS,PHOEBE,
> +    URANUS,UMBRIEL,ARIEL,MIRANDA,TITANIA,OBERON,

No need to have it as class member, just move it to the cpp file.

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/c244646f/attachment-0001.html>