D14939: Making more recommended xplanet changes and fixing bugs:
Pino Toscano
noreply at phabricator.kde.org
Tue Aug 21 05:56:08 BST 2018
pino requested changes to this revision.
pino added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> xplanetimageviewer.cpp:80-81
> int x = 0;
> - if (pix.width() < width())
> - x = (width() - pix.width()) / 2;
> - p.drawPixmap(x, 0, pix);
> + if (m_Pix.width() < width())
> + x = (width() - m_Pix.width()) / 2;
> + p.drawPixmap(x, 0, m_Pix);
Given what `resizeEvent()` does, this looks effectively dead code.
> xplanetimageviewer.cpp:184-185
> + m_FOV = KStars::Instance()->map()->fov();
> else
> - FOV = 0;
> + m_FOV = 0;
>
It is already initialized to 0, so there is no need to set it explicitly.
> xplanetimageviewer.cpp:200-206
> + m_ObjectNames << i18n("Sun") << i18n("Mercury") << i18n("Venus");
> + m_ObjectNames << i18n("Earth") << i18n("Moon");
> + m_ObjectNames << i18n("Mars") << i18n("Phobos") << i18n("Deimos");
> + m_ObjectNames << i18n("Jupiter") << i18n("Ganymede") << i18n("Io") << i18n("Callisto") << i18n("Europa");
> + m_ObjectNames << i18n("Saturn") << i18n("Titan") << i18n("Mimas") << i18n("Enceladus") << i18n("Tethys") << i18n("Dione") << i18n("Rhea") << i18n("Hyperion") << i18n("Iapetus") << i18n("Phoebe");
> + m_ObjectNames << i18n("Uranus") << i18n("Umbriel") << i18n("Ariel") << i18n("Miranda") << i18n("Titania") << i18n("Oberon");
> + m_ObjectNames << i18n("Neptune") << i18n("Triton");
As I mentioned already in the previous review, I'm pretty sure kstars knows about these names already. Isn't it possible to get them from KStarsData, instead of duplicating them?
> xplanetimageviewer.cpp:235-253
> + m_LatDisplay = new QLabel(this);
> + m_LatDisplay->setToolTip(i18n("XPlanet Latitude, only valid when viewing the object from the same object"));
> + m_LatDisplay->setText(QString::number(m_lat));
> + m_LatDisplay->setDisabled(true);
> + selectorsLayout->addWidget(m_LatDisplay);
>
> selectorsLayout->addWidget(new QLabel(",", this));
Instead of this sequence of 5 labels (two of which are static ","), it'd be much better to have a single label, updating its text at once. This way, the layout spacing wouldnot make them look like different strings, while in reality this is a single location string. Also, the "," labels are not disabled, making the whole set of widgets look awkward when m_latDisplay/etc are disabled.
> xplanetimageviewer.cpp:257-259
> + m_FreeRotate->setAttribute(Qt::WA_LayoutUsesWidgetRect);
> + m_FreeRotate->setMaximumSize(QSize(32,32));
> + m_FreeRotate->setMinimumSize(QSize(32,32));
As I mentioned already, hardcoding these attributes is a bad UI. Please remove these three lines in all the buttons, let the style decide (following user preferences, which include accessibility).
> xplanetimageviewer.cpp:390
> +
> + m_XPlanetTimeDisplay->setText(m_XPlanetTime.date().toString() + ", " + m_XPlanetTime.time().toString());
> +
Needs i18n(), since it is a UI string.
> xplanetimageviewer.cpp:412-413
> + m_TimeUnitsSelect->addItem(i18n("hours"));
> + m_TimeUnitsSelect->addItem(i18n("mins"));
> + m_TimeUnitsSelect->addItem(i18n("secs"));
> + m_TimeUnitsSelect->setCurrentIndex(MINS);
No contractions please.
> xplanetimageviewer.cpp:683
> + fifoImageLoadWatcher.setFuture(QtConcurrent::run(this, &XPlanetImageViewer::loadImage));
> + QTimer::singleShot(timeout,&fifoImageLoadWatcher,SLOT(cancel()));
> + connect(&fifoImageLoadWatcher,SIGNAL(finished()),this,SLOT(showImage()));
A single-shot timer here is a bad idea, you must track the timer to propertly stop it when the xplanet execution finishes. Otherwise you can have the following scenario:
- start a rendering, a single-shot timer is fired
- the rendering ends, but the timer still runs
- start a new rendering, a new single-shot timer is fired
- immediately the first timer fires, the second rendering is cancelled
> xplanetimageviewer.cpp:722
> {
> - QDir writableDir;
> - QString xPlanetDirPath = KSPaths::writableLocation(QStandardPaths::GenericDataLocation) + "xplanet";
> - writableDir.mkpath(xPlanetDirPath);
> - file.setFileName(xPlanetDirPath + QDir::separator() + object + ".png");
> + QString text = i18n("XPlanet Failed to generate the image for object %1 before the timeout expired. Perhaps you need a longer timeout or there is something wrong with your XPlanet installation?", m_ObjectName);
> + KMessageBox::error(this, text);
"failed"
> xplanetimageviewer.cpp:819
> setXPlanetDate(shiftedXPlanetTime);
> - dateText = shiftedXPlanetTime.date().toString() + ", " + shiftedXPlanetTime.time().toString();
> - timeEdit->setValue(timeShift);
> + m_DateText = shiftedXPlanetTime.date().toString() + ", " + shiftedXPlanetTime.time().toString();
> + m_TimeEdit->setValue(timeShift);
Needs i18n(), since it is a UI string.
> xplanetimageviewer.cpp:854-857
> if(newLat > 90)
> newLat = 90;
> if(newLat < -90)
> newLat = -90;
You can use qBound here...
> xplanetimageviewer.cpp:902-918
> KStarsDateTime utTime = KStarsData::Instance()->geo()->LTtoUT(time);
> QString year, month, day, hour, minute, second;
>
> if (year.setNum(utTime.date().year()).size() == 1)
> year.push_front('0');
> if (month.setNum(utTime.date().month()).size() == 1)
> month.push_front('0');
As I said in the previous review, this whole method can use `toString()` provided by KStarsDateTime.
> xplanetimageviewer.cpp:956
> + m_XPlanetTime = timedialog->selectedDateTime();
> + m_XPlanetTimeDisplay->setText(m_XPlanetTime.date().toString() + ", " + m_XPlanetTime.time().toString());
> int timeShift = 0;
Needs i18n(), since it is a UI string.
> xplanetimageviewer.cpp:970
> + m_XPlanetTime = KStarsData::Instance()->lt();
> + m_XPlanetTimeDisplay->setText(m_XPlanetTime.date().toString() + ", " + m_XPlanetTime.time().toString());
> int timeShift = 0;
Needs i18n(), since it is a UI string.
> xplanetimageviewer.cpp:1112-1113
> {
> - if (newURL.toLocalFile().endsWith(QLatin1String(".png")) == false)
> - newURL.setPath(newURL.toLocalFile() + ".png");
> + if (newFileName.endsWith(QLatin1String(".png")) == false)
> + newFileName + ".png";
>
This is not needed, QFileDialog can do that already, see `setDefaultSuffix()`; this means you cannot use the static method though.
> xplanetimageviewer.cpp:1118-1124
> int r = KMessageBox::warningContinueCancel(parentWidget(),
> i18n("A file named \"%1\" already exists. "
> "Overwrite it?",
> - newURL.fileName()),
> + newFileName),
> i18n("Overwrite File?"), KStandardGuiItem::overwrite());
> if (r == KMessageBox::Cancel)
> return;
QFileDialog does that by default (provided the file name is not changed).
> lancaster wrote in xplanetimageviewer.cpp:688
> 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.
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...
> lancaster wrote in xplanetimageviewer.cpp:749
> Nope, I just went back and checked. fd here is not a leaked object, it is simply an int, the return value of a computation. It isn't the file or the filename. If it is 0, that means the command succeeded and -1 means it failed.
Then please rename the `fd` variable to something else, since it is clearly not a file descriptor.
> xplanetimageviewer.h:75
> //Image related
> - QPixmap pix;
> - QImage m_Image;
> + QPixmap m_Pix {};
> + QImage m_Image {};
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.
> lancaster wrote in xplanetimageviewer.h:148-155
> Except that I used the enum values in a couple of places, particularly for Earth.
This is not what I said. The enum is not referenced anywhere in this .h file, so it can be declared locally in the .cpp file.
> kstars.kcfg:1086
> </entry>
> <entry name="XplanetAnimationDelay" type="String">
> <label>XPlanet Animation Delay</label>
UInt here too
> kstars.kcfg:1091
> </entry>
> <entry name="XplanetWidth" type="String">
> <label>Width of xplanet window</label>
Ditto
> kstars.kcfg:1096
> </entry>
> <entry name="XplanetHeight" type="String">
> <label>Height of xplanet window</label>
Ditto
> 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?
> As noted previously, Requires Maps --> requires maps
Still not changed (yet the comment was marked as "done"...).
> Also at least on Linux, there is xplanet-images package, is there more to it than that?
This package split exists only on Debian, and derivatives.
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/a144c39d/attachment-0001.html>
More information about the kde-edu
mailing list