D14813: Adding XPlanet Viewer

[Pino Toscano]

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


  - all the visible UI strings **must** be translatable using i18n & friends
  - please cleanup the commit message, all the "merge branch" bits are not helpful (and beside that, the canonical location of kstars is git.kde.org, not github); also, please write more details on what is the feature, which code changes were done (e.g. code that was moved away form `SkyMap`), etc
  - all the `this->` stuff are not needed
  
  Also, IMHO a good idea is to use Designer to create the .ui file for the dialog, so it is easier to edit.

INLINE COMMENTS

> xplanetimageviewer.cpp:49-50
> +    setSizePolicy(QSizePolicy::Preferred, QSizePolicy::Expanding);
> +    setFrameStyle(QFrame::StyledPanel | QFrame::Plain);
> +    setLineWidth(2);
> +#endif

You are setting a non-empty frame, but then you are neither taking this into account when calculating the actual size in `resizeEvent`, nor avoid to painting on the frame.

> xplanetimageviewer.cpp:70
> +
> +void XPlanetImageLabel::paintEvent(QPaintEvent *)
> +{

You can use the paint even object to know which parts were "damaged", and thus limit the `drawPixmap` below to the interested region(s). This will speed up drawing a bit.

> xplanetimageviewer.cpp:75-77
> +    int x = 0;
> +    if (pix.width() < width())
> +        x = (width() - pix.width()) / 2;

Considering that `resizeEvent` resizes the pixmap to the window size, then this code is effectively dead...

> xplanetimageviewer.cpp:88
> +
> +    if (event->size().width() == w && event->size().height() == h)
> +        return;

Just compare the sizes, eg

  if (event->size() == pix.size())

> xplanetimageviewer.cpp:132
> +
> +    objectSelector->addItem("Sun");
> +

No way to get all the objects from some other kstars API, instead of hardcoding them all?

> xplanetimageviewer.cpp:139
> +    objectSelector->addItem("Earth");
> +    objectSelector->addItem("  Moon");
> +

This is not exactly an ideal way to indent, and it will not work with some languages (beside that, translators can think the extra spaces are typos, and ingore them).
Most probably you can get a similar effect by playing with the item model of the combobox, adding children to the top-level items.

> xplanetimageviewer.cpp:225
> +
> +    timeEdit = new QLineEdit("0");
> +    timeEdit->setMaximumWidth(50);

If this is supposed to be an integer value, then use the proper widget for it: QSpinBox.

> xplanetimageviewer.cpp:234-239
> +    timeUnitsSelect->addItem("years");
> +    timeUnitsSelect->addItem("months");
> +    timeUnitsSelect->addItem("days");
> +    timeUnitsSelect->addItem("hours");
> +    timeUnitsSelect->addItem("mins");
> +    timeUnitsSelect->addItem("secs");

Why not just use an enum for the time scale, adding each item to the combobox with its enum value?
String comparison is not very efficient, and this will break with translations anyway.

> xplanetimageviewer.cpp:259
> +
> +    rotateEdit = new QLineEdit("0");
> +    rotateEdit->setMaximumWidth(50);

QSpinBox...

> xplanetimageviewer.cpp:296-299
> +    //If the caption is wider than the image, try to shrink the font a bit
> +    QFont capFont = m_Caption->font();
> +    capFont.setPointSize(capFont.pointSize() - 2);
> +    m_Caption->setFont(capFont);

Playing with fonts is not a good idea, since it could make things unreadable.
You might want to look at KSqueezedTextLabel (part of the KWidgetAddons framework).

> xplanetimageviewer.cpp:340
> +    // Create xplanet process
> +    QProcess *xplanetProc = new QProcess;
> +

This is leaked at every invocation.

> xplanetimageviewer.cpp:496
> +    //qDebug() << "Run:" << xplanetProc->program() << args.join(" ");
> +    if (xplanetProc)
> +    {

This is always true.

> xplanetimageviewer.cpp:499
> +       xplanetProc->waitForFinished(1000);
> +       if(FOV == "")
> +           m_Caption->setText("XPlanet View: " + object + ",  " + dateText);

`isEmpty()`

> xplanetimageviewer.cpp:555-570
> +    QString year, month, day, hour, minute, second;
> +
> +    if (year.setNum(time.date().year()).size() == 1)
> +        year.push_front('0');
> +    if (month.setNum(time.date().month()).size() == 1)
> +        month.push_front('0');
> +    if (day.setNum(time.date().day()).size() == 1)

I see this was copied from skymap.cpp, but it is ugly and inefficient anyway.
Since KStarsDateTime is a QDateTime, then just use its `toString()` method to format this properly.

Also, `date` is used only as argument for xplanet in `startXplanet()`, so just create & use it there directly, without storing it as class variable.

> xplanetimageviewer.cpp:675
> +    else
> +        KMessageBox::error(nullptr, file.errorString(), i18n("Image Viewer"));
> +#endif

A bit more verbose text for the error mesage box would be useful, otherwise users just get e.g. "Input/output error" and that's it.

> xplanetimageviewer.cpp:732-734
> +    bool initialLoad = false;
> +    if(this->isVisible() == false)
> +        initialLoad = true;

const bool initialLoad = !isVisible();

much easier...

> xplanetimageviewer.cpp:748-749
> +        resize(w, image.height());
> +    resize(this->width() - 1, this->height() - 1 );
> +    resize(this->width() + 1, this->height() + 1 );
> +

Why this sequence of resize()?

> xplanetimageviewer.cpp:766
> +
> +    QUrl newURL =
> +        dialog.getSaveFileUrl(KStars::Instance(), i18n("Save Image"), lastURL); // save-dialog with default filename

Considering non-local URLs as save destination are not supported, then just use file paths (and QString) instead of urls (and QUrl). This will be less confusing, and simplify things a bit.

> xplanetimageviewer.cpp:767
> +    QUrl newURL =
> +        dialog.getSaveFileUrl(KStars::Instance(), i18n("Save Image"), lastURL); // save-dialog with default filename
> +    if (!newURL.isEmpty())

`getSaveFileUrl()` is static, so call it as such, instead of constructing a QFileDialog on the stack (that won't be used, anyway).

> xplanetimageviewer.cpp:774
> +        {
> +            int r = KMessageBox::warningContinueCancel(static_cast<QWidget *>(parent()),
> +                                                       i18n("A file named \"%1\" already exists. "

Why the static_cast? Just use `parentWidget()` instead.

> xplanetimageviewer.h:43
> +
> +    QImage m_Image; // XPlanetImageViewer needs access to the image in order to modify it
> +  protected:

It does not, since there is `setImage()` already.

> xplanetimageviewer.h:76
> +    /** Create xplanet image viewer from Object */
> +    explicit XPlanetImageViewer(QString obj, QWidget *parent = nullptr);
> +

`const QString &` please.

> xplanetimageviewer.h:106
> +
> +    const QUrl m_ImageUrl;
> +    bool fileIsImage { false };

This is not set anywhere...

> xplanetimageviewer.h:115-116
> +
> +    // Share among image viewers
> +    static QUrl lastURL;
> +

Why should different viewers share their last save location?

- I open a viewer, and save to some location
- I open another view, and save to a different location
- now if I go back to the first viewer, and try to save again, I get the location of the second, which looks odd to me

> xplanetimageviewer.h:143-144
> +
> +    /** Saves file to disc. */
> +    void saveFileToDisc();
> +

FYI, "disc" is usually the CD/DVD-ROM. The hard drive is "disk".

> xplanetimageviewer.h:151
> +    void updateXPlanetDate(int timeShift);
> +    void updateXPlanetObject(QString obj);
> +    void updateXPlanetTimeUnits(QString units);

`const QString &`

> xplanetimageviewer.h:152
> +    void updateXPlanetObject(QString obj);
> +    void updateXPlanetTimeUnits(QString units);
> +    void updateXPlanetTimeEdit();

`const QString &`

REPOSITORY
  R321 KStars

REVISION DETAIL
  https://phabricator.kde.org/D14813

To: lancaster, mutlaqja, pino
Cc: pino, mutlaqja, kde-edu, narvaez, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20180814/cb2e5faa/attachment-0001.html>