<table><tr><td style="">lancaster marked an inline comment 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-79576">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;"><a href="https://phabricator.kde.org/D14813" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: line-through;">D14813: Adding XPlanet Viewer</a> adds this option... as string?! if it's a number, then just use the proper type, i.e. <tt style="background: #ebebeb; font-size: 13px;">Int</tt>.</p>
<p style="padding: 0; margin: 8px;">Also, what is the use case to make this configurable? Why should users change/edit it?<br />
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).</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Yes, you are correct, it should be int.</p>
<p style="padding: 0; margin: 8px;">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.</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-79577">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pino</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xplanetimageviewer.cpp:708-712</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">This is synchronous, and ugly.</p>
<p style="padding: 0; margin: 8px;">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.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">True, that would probably be better</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-79578">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pino</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xplanetimageviewer.cpp:749</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">fd is leaked. Most probably you want to set it for the QFile object? (see <tt style="background: #ebebeb; font-size: 13px;">QFile::open()</tt> that takes a fd.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</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-79600">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mutlaqja</span> wrote in <span style="color: #4b4d51; font-weight: bold;">opsxplanet.ui:431</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">As noted previously, Requires Maps --> requires maps</p>
<p style="padding: 0; margin: 8px;">Also at least on Linux, there is xplanet-images package, is there more to it than that?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</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>