Make CFitsio optional
ClosedPublic

Authored by asturmlechner on Oct 22 2017, 2:07 PM.

Details

Summary

Thanks-to: rkflx

Test Plan

Built with and without cfitsio enabled.

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
asturmlechner created this revision.Oct 22 2017, 2:07 PM
ckertesz accepted this revision.Oct 22 2017, 2:09 PM

I think this is just fine.

This revision is now accepted and ready to land.Oct 22 2017, 2:09 PM
rkflx requested changes to this revision.Oct 23 2017, 4:07 PM

Thanks Andreas, this is hugely appreciated. I kind of expected I had to do it myself in the end…

While I needed slightly more than 2 minutes for a thorough review (ckertesz must have a really fast machine), I could not find anything majorly wrong, just some minor things.

lib/CMakeLists.txt
182

While you just moved fitsplugin.h around, couldn't we remove it completely?

lib/imagemetainfomodel.cpp
321–322

This still shows up in the GUI (Information SidebarMeta informationMore) indicating Gwenview might support FITS' metainfo while it does not. Might be best to also exclude this?

(We could just hide all headers with empty content, but that would be material for a different patch.)

466

Does not really affect the code or the GUI, but would be the only thing not found when grepping for HAVE_FITS. On the other hand, the #ifdefs are kind of ugly. Your call :)

This revision now requires changes to proceed.Oct 23 2017, 4:07 PM
gateau accepted this revision.Oct 24 2017, 4:01 PM

LGTM, but I agree you could get rid of fitsplugin.h in lib/CMakeLists.txt

I left positive feedback pretty quickly because I basically did the same in the very first version of the FITS patch for Gwenview. And I assumed that I need to create this patch, I was surprised that somebody did it so quickly before me.

Addressed review.

rkflx accepted this revision.Oct 28 2017, 12:11 PM

I'm afraid there are still two problems (not your fault):

  • In lib/imagemetainfomodel.cpp we need to #include "config-gwenview.h", otherwise HAVE_FITS won't get defined and Gwenview crashes when accessing metainfo.
  • Without FITS support, Gwenview crashes too when accessing metainfo. This is because how the QVector is used ([] for defining ordering, as well as resize(…) and size() assuming a fully populated vector). Unfortunately, this means we need to add more #ifdefs:
diff --git a/lib/imagemetainfomodel.cpp b/lib/imagemetainfomodel.cpp
index 1a4e7172..4a919b55 100644
--- a/lib/imagemetainfomodel.cpp
+++ b/lib/imagemetainfomodel.cpp
@@ -20,6 +20,7 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
 */
 // Self
 #include "imagemetainfomodel.h"
+#include "config-gwenview.h"
 
 // Qt
 #include <QSize>
@@ -50,7 +51,9 @@ enum GroupRow {
     NoGroup = -1,
     GeneralGroup,
     ExifGroup,
+#ifdef HAVE_FITS
     FitsGroup,
+#endif
     IptcGroup,
     XmpGroup
 };
@@ -307,7 +310,11 @@ ImageMetaInfoModel::ImageMetaInfoModel()
 : d(new ImageMetaInfoModelPrivate)
 {
     d->q = this;
+#ifdef HAVE_FITS
     d->mMetaInfoGroupVector.resize(5);
+#else
+    d->mMetaInfoGroupVector.resize(4);
+#endif
     d->mMetaInfoGroupVector[GeneralGroup] = new MetaInfoGroup(i18nc("@title:group General info about the image", "General"));
     d->mMetaInfoGroupVector[ExifGroup] = new MetaInfoGroup("EXIF");
 #ifdef HAVE_FITS
@@ -455,8 +462,10 @@ void ImageMetaInfoModel::getInfoForKey(const QString& key, QString* label, QStri
         group = d->mMetaInfoGroupVector[GeneralGroup];
     } else if (key.startsWith(QLatin1String("Exif"))) {
         group = d->mMetaInfoGroupVector[ExifGroup];
+#ifdef HAVE_FITS
     } else if (key.startsWith(QLatin1String("Fits"))) {
-        group = d->mMetaInfoGroupVector[FitsGroup]; // HAVE_FITS
+        group = d->mMetaInfoGroupVector[FitsGroup];
+#endif
     } else if (key.startsWith(QLatin1String("Iptc"))) {
         group = d->mMetaInfoGroupVector[IptcGroup];
     } else if (key.startsWith(QLatin1String("Xmp"))) {

If someone is interested, this could be refactored in a later patch. Maybe to a QMap (orders by key already)? This would allow more flexible inserting and to get rid of some of the #ifdefs.

Anyway, for now I'd say you can commit after applying my patch (and probably remove Gentoo's patch :) Thanks again!

lib/imagemetainfomodel.cpp
467–468

Great idea :) Too bad we cannot keep it.

This revision is now accepted and ready to land.Oct 28 2017, 12:11 PM
asturmlechner edited the summary of this revision. (Show Details)

Added fix by rkflx

This revision was automatically updated to reflect the committed changes.
In D8423#161189, @rkflx wrote:

I'm afraid there are still two problems (not your fault)

Thanks for the thorough testing, added with your fix included.