| mutlaqja | |
| pino |
Merge remote-tracking branch 'refs/remotes/origin/master'
Merge branch 'master' of https://github.com/KDE/kstars
Merge branch 'master' of https://github.com/KDE/kstars
Merge branch 'master' of https://github.com/KDE/kstars
Adding XPlanet Viewer
Merge branch 'master' of https://github.com/KDE/kstars
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
Wow this is great and it's pretty much feature-complete! A few comments:
Also, IMHO a good idea is to use Designer to create the .ui file for the dialog, so it is easier to edit.
| kstars/auxiliary/xplanetimageviewer.cpp | ||
|---|---|---|
| 50–51 | 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. | |
| 71 | 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. | |
| 76–78 | Considering that resizeEvent resizes the pixmap to the window size, then this code is effectively dead... | |
| 89 | Just compare the sizes, eg if (event->size() == pix.size()) | |
| 133 | No way to get all the objects from some other kstars API, instead of hardcoding them all? | |
| 140 | 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). | |
| 226 | If this is supposed to be an integer value, then use the proper widget for it: QSpinBox. | |
| 235–240 | Why not just use an enum for the time scale, adding each item to the combobox with its enum value? | |
| 260 | QSpinBox... | |
| 297–300 | Playing with fonts is not a good idea, since it could make things unreadable. | |
| 341 | This is leaked at every invocation. | |
| 497 | This is always true. | |
| 500 | isEmpty() | |
| 556–571 | I see this was copied from skymap.cpp, but it is ugly and inefficient anyway. Also, date is used only as argument for xplanet in startXplanet(), so just create & use it there directly, without storing it as class variable. | |
| 676 | 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. | |
| 733–735 | const bool initialLoad = !isVisible(); much easier... | |
| 749–750 | Why this sequence of resize()? | |
| 767 | 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. | |
| 768 | getSaveFileUrl() is static, so call it as such, instead of constructing a QFileDialog on the stack (that won't be used, anyway). | |
| 775 | Why the static_cast? Just use parentWidget() instead. | |
| kstars/auxiliary/xplanetimageviewer.h | ||
| 44 | It does not, since there is setImage() already. | |
| 77 | const QString & please. | |
| 107 | This is not set anywhere... | |
| 116–117 | Why should different viewers share their last save location?
| |
| 144–145 | FYI, "disc" is usually the CD/DVD-ROM. The hard drive is "disk". | |
| 152 | const QString & | |
| 153 | const QString & | |
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
| kstars/auxiliary/xplanetimageviewer.cpp | ||
|---|---|---|
| 50–51 | I didn't write this code, it was in ImageViewer. But I can look at it. | |
| 71 | I didn't write this code, it was in ImageViewer. But this is a very good idea. | |
| 76–78 | I didn't write this code, it was in ImageViewer. But I can look at it. You are probably right | |
| 89 | I didn't write this code, it was in ImageViewer. But you are right about it, it is simpler | |
| 133 | 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. | |
| 140 | good idea | |
| 226 | VERY good idea | |
| 235–240 | I will try it | |
| 260 | yep | |
| 297–300 | I didn't write this code, it was in Skymap before I moved it to XPlanetViewer. But I can look at it. | |
| 341 | 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 | |
| 497 | True, because the new process was created a few lines before. I believe this was already in Skymap before I moved it here. | |
| 500 | ok | |
| 556–571 | 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. | |
| 676 | I didn't write this code, it was in ImageViewer. But I can look at it. | |
| 733–735 | true | |
| 749–750 | 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. | |
| 767 | I didn't write this code, it was in ImageViewer. But I can look at it. | |
| 768 | I didn't write this code, it was in ImageViewer. But I can look at it. | |
| 775 | I didn't write this code, it was in ImageViewer. But I can look at it. | |
| kstars/auxiliary/xplanetimageviewer.h | ||
| 44 | I didn't write this code, it was in ImageViewer. But I can look at it. | |
| 77 | no problem | |
| 107 | I didn't write this code, it was in ImageViewer. But I can look at it. | |
| 116–117 | I didn't write this code, it was in ImageViewer. But I can look at it. | |
| 144–145 | I didn't write this code, it was in ImageViewer. But we can certainly change it | |
| 152 | no problem | |
| 153 | no problem | |
I made a bunch of Pino's and Jasem's changes, as well as adding much more functionality and improving the interface. But it looks like when I updated this, Pino's comments got shifted around somehow, so now I can't see what code they were originally for.
| kstars/auxiliary/xplanetimageviewer.cpp | ||
|---|---|---|
| 749–750 | Ok I went back and looked at the issue. It was because the resize event was required to update the picture in the image viewer class since the picture never changed like it does in xplanet viewer. I fixed this just now by providing a different method. | |
FWIW, there are still lots of my concerns that remain unfixed -- sad to see that this patch was committed like this.
Also there are more concerns in latest revisions, for example:
@mutlaqja what about reverting this, and fixing the issues before? Otherwise, reviewing patches does not make much sense, if problems are let in like this.
I can still make more changes to the code if needed. Many of the things you mention though that are left are not actually my doing due to the fact that it was already that way in the existing code before I started. If they need to be changed in this code, they should be changed elsewhere as well. I did these things the way I thought we were already doing them.
i18n string puzzles
I'm not sure what you mean by this. I think I did the i18n translations for the tooltips and other messages the way they were done elsewhere. I got rid of the words on the buttons. There might be some more needed in the xplanet options. Is that what you mean? I'm not sure what you mean by puzzles.
hardcoded /tmp, totally unsafe temporary file name creation for the FIFO
I got this method of creating the temp fifo file from Servermanager.cpp. I thought I did it the same way. If it is wrong here, it is probably wrong there.
wrong handling of non local URLs (which in fact that not supported)
This code was already in the ImageViewer for saving the file. I didn't change it much. Would it be better to copy the code for saving from one of the other places files are saved in KStars such as fits viewer? If so, which code do you consider good?
hardcoded size for buttons
Jasem has set the minimum and maximum size for the buttons to either 32 or 22 for a very large number of buttons in Ekos. Basically any time he put icons on the buttons. Is this not the way we are doing it?
| kstars/auxiliary/xplanetimageviewer.cpp | ||
|---|---|---|
| 685 | Word puzzle. See https://techbase.kde.org/Development/Tutorials/Localization/i18n_Mistakes | |
| 687 | Word puzzle. See above. | |
Ah, I see, that is simpler. Does it not work the other way, or is this just easier to read?
I understand that, OTOH it makes no sense to carry on bad code with copy&paste just because "somebody else did it".
i18n string puzzles
I'm not sure what you mean by this. I think I did the i18n translations for the tooltips and other messages the way they were done elsewhere. I got rid of the words on the buttons. There might be some more needed in the xplanet options. Is that what you mean? I'm not sure what you mean by puzzles.
@yurchor already replied on this.
Also, your code still does lookup/comparison of translated strings, which is very fragile (and the wrong thing to do anyway).
hardcoded /tmp, totally unsafe temporary file name creation for the FIFO
I got this method of creating the temp fifo file from Servermanager.cpp. I thought I did it the same way. If it is wrong here, it is probably wrong there.
Yes, hardcoding any directory like this is definitely wrong.
wrong handling of non local URLs (which in fact that not supported)
This code was already in the ImageViewer for saving the file. I didn't change it much. Would it be better to copy the code for saving from one of the other places files are saved in KStars such as fits viewer? If so, which code do you consider good?
I already said why it is not correct: you ask the user for a remote URL, but the actual code does not support non-local URLs (and in fact it will break in that case).
If you want to handle only local files, then ask the users for local paths, and handle strings (and not URLs) for paths.
hardcoded size for buttons
Jasem has set the minimum and maximum size for the buttons to either 32 or 22 for a very large number of buttons in Ekos. Basically any time he put icons on the buttons. Is this not the way we are doing it?
Hardcoding sizes for UI elements like icons, fonts, and colors is definitely a wrong idea: other than the possibility for users to customize them according to their liking, it matters a lot for accessibility purposes. This is also why I mentioned the "do not play with fonts" earlier.
| kstars/xplanet/opsxplanet.ui | ||
|---|---|---|
| 431 | XPlanet Requires Maps -> XPlanet requires maps <a href =http://xplanet.sourceforge.net/maps.php> - wrong formatting, should be <a href="http://xplanet.sourceforge.net/maps.php"> | |