D14813: Adding XPlanet Viewer

Tue Aug 14 21:16:50 BST 2018

lancaster added a comment.

  Pino,

  Thank you for your feedback.

  I will add the translations stuff soon, I was working on the interface, its not quite done yet.  Jasem said that he wanted me to submit what I had done so far here when I asked him if   he could look at it and provide feedback.

  Yes the UI designer is nice, but the ImageViewer class didn't start with one and I started from that class.  I find that it can be easier sometimes when I am making all kinds of changes and the interface is very fluid to just code it all in the same class.  That way, adding a new set of controls is just a matter of copy and paste instead of having to edit two or three files separately.  I probably will use the UI designer once I get the UI worked out a bit more and its not changing so much.

  The commit message was such a mess because I'm new to using Phabricator and arcanist and was having some trouble getting my code to work with arc diff.  I was editing in the wrong branch so then my commit was in the wrong place and then I moved it to a new branch and then I was having trouble with the message.  arc diff wouldn't even run on my code because of the branch issues.

  Yes I can get rid of "this->"

  Thanks,

  Rob

INLINE COMMENTS

> pino wrote in xplanetimageviewer.cpp:49-50
> 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.

I didn't write this code, it was in ImageViewer.  But I can look at it.

> pino wrote in xplanetimageviewer.cpp:70
> 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.

I didn't write this code, it was in ImageViewer.  But this is a very good idea.

> pino wrote in xplanetimageviewer.cpp:75-77
> Considering that `resizeEvent` resizes the pixmap to the window size, then this code is effectively dead...

I didn't write this code, it was in ImageViewer.  But I can look at it.  You are probably right

> pino wrote in xplanetimageviewer.cpp:88
> Just compare the sizes, eg
> 
>   if (event->size() == pix.size())

I didn't write this code, it was in ImageViewer.  But you are right about it, it is simpler

> pino wrote in xplanetimageviewer.cpp:132
> No way to get all the objects from some other kstars API, instead of hardcoding them all?

Most of them do not exist in KStars since they are moons and KStars doesn't currently handle them.  The planets, Sun, and Moon could come from KStars' planet list, but then they wouldn't be in the right order with the moons.

> pino wrote in xplanetimageviewer.cpp:139
> 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.

good idea

> pino wrote in xplanetimageviewer.cpp:225
> If this is supposed to be an integer value, then use the proper widget for it: QSpinBox.

VERY good idea

> pino wrote in xplanetimageviewer.cpp:234-239
> 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.

I will try it

> pino wrote in xplanetimageviewer.cpp:259
> QSpinBox...

yep

> pino wrote in xplanetimageviewer.cpp:296-299
> 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).

I didn't write this code, it was in Skymap before I moved it to XPlanetViewer.  But I can look at it.

> pino wrote in xplanetimageviewer.cpp:340
> This is leaked at every invocation.

I didn't write this code, it was in Skymap before I moved it to XPlanetViewer.  But you are probably right, that is easily fixed

> pino wrote in xplanetimageviewer.cpp:496
> This is always true.

True, because the new process was created a few lines before.  I believe this was already in Skymap before I moved it here.

> pino wrote in xplanetimageviewer.cpp:499
> `isEmpty()`

ok

> pino wrote in xplanetimageviewer.cpp:555-570
> 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.

True, it is ugly, but that is why I had put it in this separate method, to clean up the code and get that mess out of there and keep it all together.  If there is a simple way to get the date all correct in the right format for xplanet, we should explore that.

> pino wrote in xplanetimageviewer.cpp:675
> 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.

I didn't write this code, it was in ImageViewer.  But I can look at it.

> pino wrote in xplanetimageviewer.cpp:732-734
>   const bool initialLoad = !isVisible();
> 
> much easier...

true

> pino wrote in xplanetimageviewer.cpp:748-749
> Why this sequence of resize()?

An interesting question.  I found that I was having trouble getting the view (imagelabel) to display and update without using the resize method.  repaint, paint, update, did not work.  But this did work.  I can try again.

> pino wrote in xplanetimageviewer.cpp:766
> 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.

I didn't write this code, it was in ImageViewer. But I can look at it.

> pino wrote in xplanetimageviewer.cpp:767
> `getSaveFileUrl()` is static, so call it as such, instead of constructing a QFileDialog on the stack (that won't be used, anyway).

I didn't write this code, it was in ImageViewer. But I can look at it.

> pino wrote in xplanetimageviewer.cpp:774
> Why the static_cast? Just use `parentWidget()` instead.

I didn't write this code, it was in ImageViewer. But I can look at it.

> pino wrote in xplanetimageviewer.h:43
> It does not, since there is `setImage()` already.

I didn't write this code, it was in ImageViewer.  But I can look at it.

> pino wrote in xplanetimageviewer.h:76
> `const QString &` please.

no problem

> pino wrote in xplanetimageviewer.h:106
> This is not set anywhere...

I didn't write this code, it was in ImageViewer.  But I can look at it.

> pino wrote in xplanetimageviewer.h:115-116
> 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

I didn't write this code, it was in ImageViewer.  But I can look at it.

> pino wrote in xplanetimageviewer.h:143-144
> FYI, "disc" is usually the CD/DVD-ROM. The hard drive is "disk".

I didn't write this code, it was in ImageViewer.  But we can certainly change it

> pino wrote in xplanetimageviewer.h:151
> `const QString &`

no problem

> pino wrote in xplanetimageviewer.h:152
> `const QString &`

no problem

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/7806e473/attachment-0001.html>