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 <[email protected]> wrote: > > From: Alberto Corona <[email protected]> > > > > 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 <[email protected]> > > --- > > 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 > [email protected] > http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface _______________________________________________ subsurface mailing list [email protected] http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
