[PATCH] Make Marble optional

[Dirk Hohndel]

Hey Lubomir,

thanks for the great review and feedback. Much appreciated.

my other email focused on the origin of the patch - so Alberto, as you
continue to work on this, please take Lubomir's comments into account as
well.

/D

On Sat, 2014-03-01 at 12:15 +0200, Lubomir I. Ivanov wrote:
> On 1 March 2014 08:15, Alberto <albcoron at gmail.com> wrote:
> > From: Alberto Corona <albcoron at gmail.com>
> >
> > Implemented some changes suggested by glance
> > Building could now be done without having to have Marble installed
> > Fixes #394
> >
> 
> hello,
> this is a fix in the correct direction. i've added some notes bellow.
> 
> > Signed-off-by: Alberto Corona <albcoron at gmail.com>
> > ---
> >  qt-ui/globe.cpp          | 11 +++++++++++
> >  qt-ui/globe.h            | 21 +++++++++++++++++++++
> >  qt-ui/mainwindow.h       |  1 +
> >  subsurface-configure.pri | 10 +++++++---
> >  subsurface.pro           |  2 +-
> >  5 files changed, 41 insertions(+), 4 deletions(-)
> >
> > diff --git a/qt-ui/globe.cpp b/qt-ui/globe.cpp
> > index 496afb4..7837ae7 100644
> > --- a/qt-ui/globe.cpp
> > +++ b/qt-ui/globe.cpp
> > @@ -1,4 +1,5 @@
> >  #include "globe.h"
> > +#ifndef NO_MARBLE
> >  #include "kmessagewidget.h"
> >  #include "mainwindow.h"
> >  #include "ui_mainwindow.h"
> > @@ -297,3 +298,13 @@ void GlobeGPS::resizeEvent(QResizeEvent *event)
> >                 messageWidget->setGeometry(5, 5, size - 10, 0);
> >         messageWidget->setMaximumHeight(500);
> >  }
> > +
> > +#else
> > +
> > +GlobeGPS::GlobeGPS(QWidget *parent) {}
> > +void GlobeGPS::repopulateLabels() {}
> > +void GlobeGPS::centerOn(dive *dive) {}
> > +void GlobeGPS::prepareForGetDiveCoordinates() {}
> > +void GlobeGPS::reload() {}
> > +bool GlobeGPS::eventFilter(QObject *obj, QEvent *ev) {}
> 
> eventFilter() would need to return a bool here, to prevent a warning.
> 
> > +#endif
> > diff --git a/qt-ui/globe.h b/qt-ui/globe.h
> > index 80d9613..e25f244 100644
> > --- a/qt-ui/globe.h
> > +++ b/qt-ui/globe.h
> > @@ -1,5 +1,6 @@
> >  #ifndef GLOBE_H
> >  #define GLOBE_H
> > +#ifndef NO_MARBLE
> >
> >  #include <marble/MarbleWidget.h>
> >  #include <marble/GeoDataCoordinates.h>
> > @@ -41,4 +42,24 @@ slots:
> >         void prepareForGetDiveCoordinates();
> >  };
> >
> > +#else
> > +
> > +#include <QWidget>
> > +
> > +struct dive;
> > +
> > +class GlobeGPS : public QWidget {
> > +        Q_OBJECT
> > +public:
> > +        GlobeGPS(QWidget *parent);
> > +        void reload();
> > +        void repopulateLabels();
> > +        void centerOn(struct dive *dive);
> > +        bool eventFilter(QObject *, QEvent *);
> > +public
> > +slots:
> > +        void prepareForGetDiveCoordinates();
> > +};
> > +
> > +#endif
> 
> the indentation here needs to be with one real-tab instead of 8
> spaces. even if your indentation is shown as 8 spaces the actual tab
> character needs to be inserted.
> you could adjust your text editor as described in CodingStyle.
> 
> >  #endif // GLOBE_H
> > diff --git a/qt-ui/mainwindow.h b/qt-ui/mainwindow.h
> > index 1db3d5a..65a532c 100644
> > --- a/qt-ui/mainwindow.h
> > +++ b/qt-ui/mainwindow.h
> > @@ -9,6 +9,7 @@
> >
> >  #include <QMainWindow>
> >  #include <QAction>
> > +#include <QSettings>
> 
> this seems like a separate change.
> any reason for that?
> 
> >  #include <QUrl>
> >
> >  #include "ui_mainwindow.h"
> > diff --git a/subsurface-configure.pri b/subsurface-configure.pri
> > index 8e6aead..fb48185 100644
> > --- a/subsurface-configure.pri
> > +++ b/subsurface-configure.pri
> > @@ -116,7 +116,6 @@ isEmpty(XML2_CFLAGS)|isEmpty(XML2_LIBS): \
> >  isEmpty(XSLT_CFLAGS)|isEmpty(XSLT_LIBS): \
> >         error("Could not find libxslt. Did you forget to install it?")
> >
> > -
> 
> whitespace changes with not particular meaning in general should be
> avoided as they are like noise near the *actual* changes, even if
> intentional.
> we tend to leave them roaming for a while until a patch is *specific*
> for whitespace cleanup.
> 
> >  QMAKE_CFLAGS *= $$XML2_CFLAGS $$XSLT_CFLAGS
> >  QMAKE_CXXFLAGS *= $$XML2_CFLAGS $$XSLT_CFLAGS
> >  LIBS *= $$XSLT_LIBS $$XML2_LIBS
> > @@ -131,14 +130,19 @@ link_pkgconfig: PKGCONFIG += libzip sqlite3
> >  # Add libiconv if needed
> >  link_pkgconfig: packagesExist(libiconv): PKGCONFIG += libiconv
> >
> > +# Add Marble if present
> > +link_pkgconfig: packagesExist(libmarlbe): DEFINES += NO_MARBLE
> > +
> 
> there seems to be a typo here libmarlbe -> libmarble.
> but this part should be removed...
> the marble plugin does not provide pkg-config entries, which means
> that packagesExist() will always return false unless you have manually
> created a .pc file for it.
> i think that we should always look for marble, unless the user does manually a:
> qmake <someoptions> "DEFINES += NO_MARBLE"
> 
> >  #
> >  # Find libmarble
> >  #
> >  # Before Marble 4.9, the GeoDataTreeModel.h header wasn't installed
> >  # Check if it's present by trying to compile
> >  # ### FIXME: implement that
> > -win32: CONFIG(debug, debug|release): LIBS += -lmarblewidgetd
> > -else: LIBS += -lmarblewidget
> > +!contains(DEFINES, NO_MARBLE) {
> > +        win32: CONFIG(debug, debug|release): LIBS += -lmarblewidgetd
> > +        else: LIBS += -lmarblewidget
> > +}
> >
> 
> looks good to me.
> 
> >  #
> >  # Platform-specific changes
> > diff --git a/subsurface.pro b/subsurface.pro
> > index 81cb906..49865d7 100644
> > --- a/subsurface.pro
> > +++ b/subsurface.pro
> > @@ -4,7 +4,7 @@ QT = core gui network svg
> >  lessThan(QT_MAJOR_VERSION, 5) {
> >         QT += webkit
> >  } else {
> > -       QT += webkitwidgets
> > +       !android: QT += webkitwidgets
> >  }
> 
> any reason for this particular change?
> it will probably be best to separate this patch into 2-3 patches and
> send them in series with separate commit messages.
> 
> >  INCLUDEPATH += qt-ui $$PWD
> >  DEPENDPATH += qt-ui
> > --
> 
> lubomir
> --
> _______________________________________________
> subsurface mailing list
> subsurface at hohndel.org
> http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface