D14939: Making more recommended xplanet changes and fixing bugs:

[Robert Lancaster]

lancaster marked 19 inline comments as done.
lancaster added inline comments.

INLINE COMMENTS

> pino wrote in xplanetimageviewer.cpp:688
> Sure, and this is why I said that kstars can have a longer timeout on its own.
> Why e.g. 1 minute as timeout would not be enough, for example? Or make it 5 minutes -- still better than letting the user decide, which means that they can break the rendering for no reason...

The time that xplanet takes to complete a rendering is not predictable because it depends on the positioning of moons, shadows, maps, and many other variables other than size.  It will take a different amount of time on different machines too.  But we can't make the timeout a really long time because then the user would have to wait for it to finish.   The user might want to wait for a complicated render to finish or maybe they don't and just want to get on to the next frame.

> pino wrote in xplanetimageviewer.cpp:80-81
> Given what `resizeEvent()` does, this looks effectively dead code.

The lines you highlighted are not dead code.  I just tried commenting them out to make sure.  They are centering the drawing of the planet image in the view.  No, they aren't resizing it, as you stated that is already handled in resizeEvent, they are repositioning it.

> pino wrote in xplanetimageviewer.cpp:184-185
> It is already initialized to 0, so there is no need to set it explicitly.

Yes that is true now that I changed the header file to set it to zero.

> pino wrote in xplanetimageviewer.cpp:854-857
> You can use qBound here...

Nice feature, didn't know about it

> pino wrote in xplanetimageviewer.cpp:902-918
> As I said in the previous review, this whole method can use `toString()` provided by KStarsDateTime.

I do not believe so.  XPlanet requires its date and time to be in a numerical format with a very specific structure.  Here is a comparison of what the Date/Time looks like with the toString method and with the above code.  There may be a way to get KStarsDateTime to do this, but the toString method does not.

Using toString: "Tue Aug 21 18:16:05 2018 GMT"
Using current code: "20180821.181605"

> pino wrote in xplanetimageviewer.cpp:1112-1113
> This is not needed, QFileDialog can do that already, see `setDefaultSuffix()`; this means you cannot use the static method though.

I just tried to implement this.  The code was much longer because I had to add a bunch of lines to give it the same functionality as the static method, but perhaps its better that way I don't know.

> pino wrote in xplanetimageviewer.cpp:1118-1124
> QFileDialog does that by default (provided the file name is not changed).

ok

> pino wrote in xplanetimageviewer.h:75
> This explicit {} initializer is not needed for Qt classes, since most of them (at least all the ones used here) have a default constructor already.

Gotcha, I got carried away when making the change Jasem requested

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