Thanks-to: rkflx
Details
- Reviewers
gateau ckertesz rkflx - Commits
- R260:5ea8b9acd03e: Make CFitsio optional
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.
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 Sidebar → Meta information → More) 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 :) |
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.
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. |