<table><tr><td style="">lancaster marked 19 inline comments as done.<br />lancaster added inline comments.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D14939">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D14939#inline-79859">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pino</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xplanetimageviewer.cpp:688</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Sure, and this is why I said that kstars can have a longer timeout on its own.<br />
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...</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D14939#inline-79852">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pino</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xplanetimageviewer.cpp:80-81</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Given what <tt style="background: #ebebeb; font-size: 13px;">resizeEvent()</tt> does, this looks effectively dead code.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D14939#inline-79853">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pino</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xplanetimageviewer.cpp:184-185</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">It is already initialized to 0, so there is no need to set it explicitly.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Yes that is true now that I changed the header file to set it to zero.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D14939#inline-79869">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pino</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xplanetimageviewer.cpp:854-857</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">You can use qBound here...</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Nice feature, didn't know about it</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D14939#inline-79872">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pino</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xplanetimageviewer.cpp:902-918</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">As I said in the previous review, this whole method can use <tt style="background: #ebebeb; font-size: 13px;">toString()</tt> provided by KStarsDateTime.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</p>

<p style="padding: 0; margin: 8px;">Using toString: "Tue Aug 21 18:16:05 2018 GMT"<br />
Using current code: "20180821.181605"</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D14939#inline-79848">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pino</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xplanetimageviewer.cpp:1112-1113</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">This is not needed, QFileDialog can do that already, see <tt style="background: #ebebeb; font-size: 13px;">setDefaultSuffix()</tt>; this means you cannot use the static method though.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D14939#inline-79849">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pino</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xplanetimageviewer.cpp:1118-1124</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">QFileDialog does that by default (provided the file name is not changed).</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">ok</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D14939#inline-79865">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pino</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xplanetimageviewer.h:75</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">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.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Gotcha, I got carried away when making the change Jasem requested</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R321 KStars</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D14939">https://phabricator.kde.org/D14939</a></div></div><br /><div><strong>To: </strong>lancaster, yurchor, pino, mutlaqja, kde-edu<br /><strong>Cc: </strong>kde-edu, mutlaqja, pino, yurchor, narvaez, apol<br /></div>