Details
- Reviewers
gateau
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
This is imported from KStars but I hope Gwenview supports FITS, it would benefit all users who want to view and/or preview astronomical FITS images!
Looks good, but I think it would be better if most of the work were in https://cgit.kde.org/kimageformats.git/ instead, unless it is not possible for some reason. Doing it that way would avoid adding a bunch of ifs in Gwenview, but more importantly, would bring support to fits format to all KDE applications. It might still be necessary to add some fits-specific code to show the meta information in Gwenview.
| cmake/FindCFitsio.cmake | ||
|---|---|---|
| 73 | File indentation is inconsistent. Better use 4 space indents everywhere. | |
Indentation:
- Fix in the latest update.
#ifdefs:
- I added fits support to be an optional dependency. Now I removed the ifdefs as you requested.
kimageformats:
- I have been thinking about your comment and I checked kimageformats. I would not like to put fits support to kimageformats:
- The direct need of the astronomy community is a handy preview tool to check the quality of the captured star images. Nothing beyond that currently.
- FITS files are only for astronomy. There is no much use outside of this field.
- I don't want to implement saving. Just loading the images and that's it.
- The metadata handling should be still implemented in Gwenview. I don't want to maintain things in multiple places.
Regarding kimageformats:
- You don't need to provide write support: some formats are read-only (see README.md)
- Adding fits format to kimageformats also means Dolphin and the file dialogs can show thumbnails for .fits files, maybe that could be useful?
- It is true that you would have to duplicate some code to add metadata support in Gwenview. The right way to avoid that would be to create a Qt library to handle the fits image format and use it in kimageformats and Gwenview (and KStars maybe?). That's more work to do though, so I can understand if you don't want to go that way, but if you are already duplicating code from KStars to Gwenview to add fits support (I haven't checked if it is the case), then creating a library would make sense.
If you think kimageformats is not the right way, then I would just ask you to move the image decoding code to lib/imageformats inside Gwenview and register the format imageformats.cpp. This way LoadingDocumentImpl does not need a special case for Fits image format. The calls to ImageFormats::registerPlugins will need to be uncommented in app/main.cpp and part/gvpart.cpp.
Also, would be nice if the new files followed kdelibs coding style (https://community.kde.org/Policies/Kdelibs_Coding_Style)
Here it is the new revision:
- The fits decoding is moved to imageformats as you requested.
- I had huge problems with the static plugin registration, the old way did not work at all. I finally figured out a workaround hack, but I had to spend couple of hours on this. Explanation is written in app/CMakeLists.txt.
- If you have some specific problem with the coding style, please tell those in details. I don't want to change bayer.c+bayer.h for sure because they are 3rdparty sources. They are better to leave untouched for future upstream updates.
The fits decoding is moved to imageformats as you requested.
I had huge problems with the static plugin registration, the old way did not work at all. I finally figured out a workaround hack, but I had to spend couple of hours on this. Explanation is written in app/CMakeLists.txt.
Thanks for taking the time to do this.
If you have some specific problem with the coding style, please tell those in details. I don't want to change bayer.c+bayer.h for sure because they are 3rdparty sources. They are better to leave untouched for future upstream updates.
Yes, bayer.c and bayer.h can be left as is, but there are a few places in the C++ code which do not follow the KDE libs coding style, most notably braces: the coding style says one-line blocks should be enclosed in braces: https://community.kde.org/Policies/Kdelibs_Coding_Style#Braces. Switch statements do not follow the coding style as well: https://community.kde.org/Policies/Kdelibs_Coding_Style#Switch_statements
I made a few more inline comments, but they are mostly nitpicks, thanks a lot for your work here!
| app/CMakeLists.txt | ||
|---|---|---|
| 12 | Why is this restricted to Linux? Does it work out of the box on Windows and macOS? | |
| lib/imageformats/fitshandler.cpp | ||
| 37 | style: should be if (!device()) | |
| 56 | indentation is wrong | |
| lib/imageformats/fitshandler.h | ||
| 39 | These methods should be marked as Q_DECL_OVERRIDE, like in fitsplugin.h | |
| lib/imagemetainfomodel.cpp | ||
| 83 | could this be simplfied into value = QString::number(number)? | |
| 88 | Not required: QFile does it in its destructor. | |
Comments addressed.
"Why is this restricted to Linux? Does it work out of the box on Windows and macOS?"
Because Linux, Windows and macOS have different compiler+linker pairs. It is maybe not an issue on other platforms. If you get any linking fails in the future regarding this, you can forward the bug report to me.
Why is this restricted to Linux? Does it work out of the box on Windows and macOS?
Because Linux, Windows and macOS have different compiler+linker pairs. It is maybe not an issue on other platforms.
I would be surprised if the problem was different on other OS, but I can't check.
If you get any linking fails in the future regarding this, you can forward the bug report to me.
I am not Gwenview maintainer anymore, so I don't read bug reports. I am the former maintainer. The current maintainer has disappeared, so I am just making code reviews to encourage people to contribute, in the hope that someone steps in to take over Gwenview maintenance.
Thanks for working on this, getting FITS format support to the wider KDE ecosystem from being KStars-only is appreciated. However, I'm not sure copying the code around is the best idea, as @cfeck mentioned on gwenview-devel.
@bcooksley This has been committed in b49569c97a75, but breaks the CI as the dependency on CFitsio is missing. Could you comment on how the proper process for such a dependency change would have been?
In addition, this does not compile for me even after adding the dependency:
AutoMoc: Error: /home/user/gwenview/app/main.cpp
The file includes the moc file "lib/moc_fitsplugin.cpp", but could not find header "fitsplugin{.h,.hh,.h++,.hm,.hpp,.hxx,.in,.txx}" neither in /home/user/gwenview/app/ nor in /home/user/gwenview/app/lib/Could be related to This hack is needed to include the fitsplugin moc file in main.cpp?
Usually developers will file a task on the build.kde.org board, after which we'll action it.
I'm assuming there is good reason for making this a mandatory dependency.
I've now initiated https://build.kde.org/view/CI%20Management/job/Docker%20Generate%20FedoraQt5.8%20Image/53/ which will bring the new change into effect on the CI System.
Once that is complete, considering the fact the image hasn't been rebuilt in two months we'll likely have to rebuild the Dependency chain as well (fortunately just a case of triggering the two Dependency Build jobs for the FedoraQt5.8 platform).
@bcooksley This has been committed in b49569c97a75, but breaks the CI as the dependency on CFitsio is missing. Could you comment on how the proper process for such a dependency change would have been?
Fitsio was an optional dependency in the original commit and it can be done again. @gateau commented that he did not like the ifdefs in the code that was the reason why it was changed to a hard dependency.
In addition, this does not compile for me even after adding the dependency:
AutoMoc: Error: /home/user/gwenview/app/main.cpp The file includes the moc file "lib/moc_fitsplugin.cpp", but could not find header "fitsplugin{.h,.hh,.h++,.hm,.hpp,.hxx,.in,.txx}" neither in /home/user/gwenview/app/ nor in /home/user/gwenview/app/lib/Could be related to This hack is needed to include the fitsplugin moc file in main.cpp?
Sorry to hear about this. The FITS support was moved to a plugin after some review comments. Gwenview uses static plugins internally, and unfortunately, it does not work on my machine (Ubuntu 16.04, updated with KUbuntu backports PPA, Qt 5.6.1). Please see the following comments about the symptoms in that CMakeLists.txt.
What kind of system (distro, Qt, KDE version?) do you use where the compilation fails like above.
I've also got the compile error in KDE Neon.
#include'ing moc_ files directly is incorrect procedure; they are temporary cache files created by CMake whose location is not set and that cannot be relied on for dependency tracking. If for example, you run CMake from a ./build directory, the moc_ files get created in [source dir]/build/lib/, not [source dir]/lib.
The people seeing this error are the people who (correctly) run CMake from a separate directory to keep build and source files separate. Apparently the Jenkins CI also (correctly) does this.
You need to investigate and fix the link error; #include'ing a moc_ file is not sustainable and can't stay.
Fitsio was an optional dependency in the original commit and it can be done again.
Adding the dependency to the CI is good anyway, to get the code build-tested. As for making it an optional dependency for Gwenview, this would be in line with other deps like KF5KDcraw and probably also preferred by distro packagers (they like separate libs more than 3rd party code duplicates, too). Perhaps Aurélien used the ifdef-argument just to convince you to add it to kimageformats :) ? On the other hand, this Diff was accepted already, so you should probably ignore my comment for now…
@gateau Could you comment on that?
Regarding the moc error, @ngraham is right: this depends on the build layout. D8001 has a fix. However, including the moc instead of fixing the linking is still a hack. I would very much prefer a proper solution. Here are some ideas to get there:
- Re-read and follow the docs on Qt Plugins.
- Try to involve some experts on this topic (ask on IRC or @cfeck whom they would recommend).
- Make it a proper library / move it to kimageformats. There all linking problems should be solved already and you should be able to find some example code, in addition to the advantages mentioned by Aurélien. ← hint, hint :)
@ckertesz What do you think?
Doesn't build for me even with cftsio found and used (openSUSE Tumbleweed):
Also, as a packager, I'd like this to be optional if possible (or moved to kimageformats).
[ 129s] In file included from /home/abuild/rpmbuild/BUILD/gwenview-17.11.80git.20170926T000805~b49569c9/lib/imageformats/fitshandler.cpp:24:0: [ 129s] /home/abuild/rpmbuild/BUILD/gwenview-17.11.80git.20170926T000805~b49569c9/lib/imageformats/fitsformat/fitsdata.h:34:10: fatal error: fitsio.h: No such file or directory [ 129s] #include <fitsio.h> [ 129s] ^~~~~~~~~~ [ 129s] compilation terminated. [ 129s] make[2]: *** [lib/CMakeFiles/gwenviewlib.dir/build.make:1483: lib/CMakeFiles/gwenviewlib.dir/imageformats/fitshandler.cpp.o] Error 1
Should be fine now. As you may have seen, the fix from yesterday even had an include path change (as on openSUSE, headers are in /usr/include/cfitsio).
Also, as a packager, I'd like this to be optional if possible (or moved to kimageformats).
I just looked at the dep freeze date. There's not much time left, but we also should not rush the move to kimageformats (if it is possible, indeed). Perhaps for 17.12 the best we can do is make it optional. Let's see who is actually willing to do the work or help with this.
As there is nothing further for me to sort here, going to remove myself.
I'll prod the Fedora folks as to why the image build is failing. Looks like something in Fedora itself has regressed....
Regarding the moc error, @ngraham is right: this depends on the build layout. D8001 has a fix. However, including the moc instead of fixing the linking is still a hack. I would very much prefer a proper solution. Here are some ideas to get there:
- Re-read and follow the docs on Qt Plugins.
I did it many times and googled back and forth, I spent a day on this problem. I have some expertise on Qt plug-ins, macOS/iOS is also very problematic with static Qt plug-ins.
- Try to involve some experts on this topic (ask on IRC or @cfeck whom they would recommend).
I think the basic problem here that the static Qt plugin is put inside a shared library. If the plugin would be statically linked to the gwenview executable, it would work fine. There are several solutions:
- Make the internal Gwenview plug-ins non-static: I did not go in this way because then it would trigger extra work on the packaging side what I have no idea how much work.
- Some linking (binutils) expert takes a look on the linking problem and craft a quick solution to avoid hiding/keep the needed symbols. I am pretty sure that e.g. a small build script snippet would fix this issue.
I think the basic problem here that the static Qt plugin is put inside a shared library
Maybe it's time to revisit the option of moving the plugin to kimageformat or creating a libfits-qt wrapper, which would solve that bug, and avoid duplication.
So far, nobody seems to have worked to bring this to R287 KImageFormats or to provide another solution. As the dependency freeze for KDE Applications 17.12 is on November 9th, at least we should make CFitsio optional.
@ckertesz Would you be able to provide a patch to that effect, i.e. open a new Differential based on https://phabricator.kde.org/D7305?vs=18227?