[PATCH] Make Marble optional
Alberto Corona
albcoron at gmail.com
Thu Mar 6 23:10:11 PST 2014
I'll make sure to console the rest and see how they think this should be
implemented. I haven't yet attempted a cross compile using this method
(without using marble), I'll also make sure to try to test that as best I
could both for the cross compiliing scripts and native. In the sense of
compliling for Windows natively I mean compiling native binaries on Windows
using the qmake flags for the no-marble configs. For the latter (optional
marble dependencies for the user), that actually seems a bit interesting.
If a user cares little to none about the marble widget then why should they
have to have those dependencies installed?
On Fri, Mar 7, 2014 at 1:02 AM, Anton Lundin <glance at acc.umu.se> wrote:
> On 06 March, 2014 - Alberto Corona wrote:
>
> > Compiling on other systems should not be affected as long as the name for
> > the marble library is the same on those systems.
>
> There are support for a different library name. Have you tried to
> investigate why its there?
>
> If its not needed anymore, remove it, but don't leave it there half
> broken.
>
> > Again, like I said before,
> > I'm far too unfamiliar with Qt to be able to make the changes to the main
> > window to hide the dummy widget.
>
> As said before, if you can't hide the panel, replace it with the text
> "MARBLE DISABLED" or something instead. Just having a blank panel there
> is the worst outcome of those tree options.
>
> > Compiling on Windows natively is already a
> > HUGE pain, and it already seems cross compiling is the better solution,
> and
> > I don't know if many people do that. What I'm trying to ask though is if
> > it's really worth fixing the main window for a completely optional
> compile
> > time option (as it is)?
>
> What? When did this become something about compiling natively on
> windows?
>
> > Or, should this go into a different implementation
> > to where the user could technically have subsurface installed and have
> the
> > marble widget load as an optional dependency? As I understood, this was
> > initially an option solely for being able to develop subsurface without
> > having to install marble and the dependency load it brings
> >
>
> This sounds like a really bad idea. Compile time switch is just fine.
>
>
> I'm going diving for a week, so, hopefully others can help in the
> process.
>
> So if/when the rest of the list is happy with your changes to my old
> code, feel free to add a sob to it and have it merged.
>
>
> //Anton
>
>
> > On Thu, Mar 6, 2014 at 1:06 PM, Anton Lundin <glance at acc.umu.se> wrote:
> >
> > > On 05 March, 2014 - Alberto Corona wrote:
> > >
> > > > I hate gmail, still need to configure my mutt. For the list:
> > > >
> > > > Sure, the reason I didn't send the patch initially was to see if you
> were
> > > > alright with the changes. Would a warning or verbose message that
> > > > subsurface was build without marble really be necessary if the user
> has
> > > to
> > > > run qmake with "CONFIG += no-marble"? Also, is there different
> naming for
> > > > the marble library on Windows as in order to link up marble you have
> to
> > > > pass "-lmarblewidget" and NOT "-lmarblewidgetd"? So that is, that
> config
> > > > works for compiling without marble on Linux. Also, I'm sorry if I was
> > > > unclear again and didn't ask if I could continue using some of your
> > > work,
> > > > the reason for me continuing this thread was to communicate with you
> and
> > > > then see if you'd like me to ammend the commit message and or send
> you
> > > the
> > > > patch for your SOB.
> > > >
> > >
> > > I would say that so long we have a blank panel there instead of a
> marble
> > > widget we should say something about why its blank.
> > >
> > > The patch you suggested doesn't say anything about only working on
> Linux
> > > and I think it would be a bad thing if it doesn't work across all our
> > > different platforms. I would much prefer the original solution.
> > >
> > >
> > > //Anton
> > >
> > >
> > >
> > > > On Wed, Mar 5, 2014 at 11:24 AM, Alberto Corona <albcoron at gmail.com>
> > > wrote:
> > > >
> > > > > Sure, the reason I didn't send the patch initially was to see if
> you
> > > were
> > > > > alright with the changes. Would a warning or verbose message that
> > > > > subsurface was build without marble really be necessary if the user
> > > has to
> > > > > run qmake with "CONFIG += no-marble"? Also, is there different
> naming
> > > for
> > > > > the marble library on Windows as in order to link up marble you
> have to
> > > > > pass "-lmarblewidget" and NOT "-lmarblewidgetd"? So that is, that
> > > config
> > > > > works for compiling without marble on Linux. Also, I'm sorry if I
> was
> > > > > unclear again and didn't ask if I could continue using some of
> your
> > > work,
> > > > > the reason for me continuing this thread was to communicate with
> you
> > > and
> > > > > then see if you'd like me to ammend the commit message and or send
> you
> > > the
> > > > > patch for your SOB.
> > > > >
> > > > >
> > > > > On Tue, Mar 4, 2014 at 5:34 PM, Anton Lundin <glance at acc.umu.se>
> > > wrote:
> > > > >
> > > > >> On 04 March, 2014 - Alberto Corona wrote:
> > > > >>
> > > > >> > Right, first off I fixed the warnings by the compiler by
> returning
> > > the
> > > > >> > object and event. I also fixed the qmake config (it was
> previously
> > > > >> > completely broken). Also added the instructions to compile
> without
> > > in
> > > > >> > the INSTALL.
> > > > >> >
> > > > >>
> > > > >> First, There are nothing about attribution about where most of the
> > > code
> > > > >> comes from. If you base your code on my code, write that in the
> commit
> > > > >> message. Add also a Signed-off-by: from me as the first of the
> lines
> > > in
> > > > >> the "audit-trail".
> > > > >> When you continue on someone else work its a really good ask that
> > > person
> > > > >> for a ACK that it looks sane.
> > > > >>
> > > > >> For example when Dirk sees that you have bin continuing on
> something i
> > > > >> wrote, he usually would like my feedback before merging that code.
> > > > >>
> > > > >> https://www.kernel.org/doc/Documentation/SubmittingPatches have a
> > > quite
> > > > >> good explanation how to think surrounding how and when stuff
> should be
> > > > >> signed.
> > > > >>
> > > > >>
> > > > >> If we don't completely remove that frame from the main window it
> > > should
> > > > >> be replaced with something saying that marble is disabled at
> least.
> > > > >>
> > > > >>
> > > > >> Then the "LIBS -= -lmarblewidget", if you look up a couple of
> rows, it
> > > > >> might be named -lmarblewidgetd .
> > > > >>
> > > > >>
> > > > >> Could you elaborate on the: "QSettings
> > > > >> had to be included into maintab.h in order for QSettings to be
> > > defined."
> > > > >>
> > > > >> I can't find any reason for that change to interact with the rest
> of
> > > the
> > > > >> code.
> > > > >>
> > > > >>
> > > > >> //Anton
> > > > >>
> > > > >>
> > > > >> > On Tue, Mar 04, 2014 at 09:33:13AM +0100, Anton Lundin wrote:
> > > > >> > > On 03 March, 2014 - Alberto Corona wrote:
> > > > >> > >
> > > > >> > > > Here's the link again as I forgot to include the list
> > > > >> > > >
> > > > >>
> > >
> https://github.com/0x1A/subsurface/commit/eb215101097120f8c62339fd767b71574cdd0bbd
> > > > >> > > >
> > > > >> > > > Like Anton said, this is a bit hacky, though I don't think
> > > there's
> > > > >> a very
> > > > >> > > > easy way to do so without reworking quite a bit of the
> > > mainwindow
> > > > >> (for
> > > > >> > > > which I'm too inexperienced with Qt for to change).
> > > > >> > > >
> > > > >> > >
> > > > >> > > When resubmitting a patch, its a god practice to resubmit it
> in
> > > the
> > > > >> same
> > > > >> > > way as you submitted your first patch.
> > > > >> > >
> > > > >> > > Its also a really god idea to include a changelog addressing
> the
> > > > >> review
> > > > >> > > comments from the last submission.
> > > > >> > >
> > > > >> > >
> > > > >> > > So, pleas go back and look at the comments from the last
> > > submission
> > > > >> and
> > > > >> > > tell us how you have addressed those.
> > > > >> > >
> > > > >> > >
> > > > >> > > //Anton
> > > > >> > >
> > > > >> > >
> > > > >> > > > On Mon, Mar 3, 2014 at 8:00 PM, Alberto Corona <
> > > albcoron at gmail.com>
> > > > >> wrote:
> > > > >> > > >
> > > > >> > > > >
> > > > >> > > > > The changes are here
> > > > >> > > > >
> > > > >>
> > >
> https://github.com/0x1A/subsurface/commit/eb215101097120f8c62339fd767b71574cdd0bbd
> > > > >> > > > >
> > > > >> > > > >
> > > > >> > > > > On Mon, Mar 3, 2014 at 7:29 PM, Dirk Hohndel <
> > > dirk at hohndel.org>
> > > > >> wrote:
> > > > >> > > > >
> > > > >> > > > >> On Mon, 2014-03-03 at 18:17 -0600, Alberto Corona wrote:
> > > > >> > > > >> > Ok so I've got things working now, though I'd like to
> see
> > > if
> > > > >> Anton is
> > > > >> > > > >> > ok with the changes. Unfortunately, I'm not familiar
> enough
> > > > >> with Qt to
> > > > >> > > > >> > change the way the splitters work in order to get rid
> of
> > > the
> > > > >> blank
> > > > >> > > > >> > dummy widget.
> > > > >> > > > >> >
> > > > >> > > > >>
> > > > >> > > > >> And where would we find those changes?
> > > > >> > > > >>
> > > > >> > > > >> /D
> > > > >> > > > >>
> > > > >> > > > >>
> > > > >> > > > >>
> > > > >> > > > >
> > > > >> > >
> > > > >> > > > _______________________________________________
> > > > >> > > > subsurface mailing list
> > > > >> > > > subsurface at hohndel.org
> > > > >> > > >
> http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
> > > > >> > >
> > > > >> > >
> > > > >> > > --
> > > > >> > > Anton Lundin +46702-161604
> > > > >>
> > > > >> > >From eb215101097120f8c62339fd767b71574cdd0bbd Mon Sep 17
> 00:00:00
> > > 2001
> > > > >> > From: Alberto Corona <albcoron at gmail.com>
> > > > >> > Date: Mon, 3 Mar 2014 13:49:21 -0600
> > > > >> > Subject: [PATCH] Make Marble optional at compile
> > > > >> >
> > > > >> > Make Marble optional at compile by creating a dummy widget.
> > > QSettings
> > > > >> > had to be included into maintab.h in order for QSettings to be
> > > defined.
> > > > >> > Add instructions in INSTALL for compiling without Marble.
> > > > >> >
> > > > >> > Signed-off-by: Alberto Corona <albcoron at gmail.com>
> > > > >> > ---
> > > > >> > INSTALL | 1 +
> > > > >> > qt-ui/globe.cpp | 12 ++++++++++++
> > > > >> > qt-ui/globe.h | 21 +++++++++++++++++++++
> > > > >> > qt-ui/maintab.h | 1 +
> > > > >> > subsurface-configure.pri | 6 ++++++
> > > > >> > 5 files changed, 41 insertions(+)
> > > > >> >
> > > > >> > diff --git a/INSTALL b/INSTALL
> > > > >> > index be3c5ed..0c192e6 100644
> > > > >> > --- a/INSTALL
> > > > >> > +++ b/INSTALL
> > > > >> > @@ -74,6 +74,7 @@ $ sudo make install
> > > > >> > To compile Subsurface:
> > > > >> > $ git clone git://subsurface.hohndel.org/subsurface.git
> > > > >> > $ cd subsurface
> > > > >> > + - You can use "CONFIG += no-marble" to compile without the
> Marble
> > > > >> widget -
> > > > >> > $ qmake # qmake-qt4 on some flavors of Linux
> > > > >> > $ make
> > > > >> > $ sudo make install [optionally, add: prefix=/usr/local]
> > > > >> > diff --git a/qt-ui/globe.cpp b/qt-ui/globe.cpp
> > > > >> > index 496afb4..69a12c5 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,14 @@ void GlobeGPS::resizeEvent(QResizeEvent
> *event)
> > > > >> > messageWidget->setGeometry(5, 5, size - 10, 0);
> > > > >> > messageWidget->setMaximumHeight(500);
> > > > >> > }
> > > > >> > +
> > > > >> > +#else
> > > > >> > +
> > > > >> > +GlobeGPS::GlobeGPS(QWidget *parent) {}
> > > > >> > +void GlobeGPS::reload() {}
> > > > >> > +void GlobeGPS::repopulateLabels() {}
> > > > >> > +void GlobeGPS::centerOn(struct dive *dive) {}
> > > > >> > +void GlobeGPS::prepareForGetDiveCoordinates() {}
> > > > >> > +bool GlobeGPS::eventFilter(QObject *obj, QEvent *ev) {return
> > > > >> QObject::eventFilter(obj, ev);}
> > > > >> > +
> > > > >> > +#endif
> > > > >> > diff --git a/qt-ui/globe.h b/qt-ui/globe.h
> > > > >> > index 80d9613..fb57d2e 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
> > > > >> > +
> > > > >> > +// Dummy widget in place of Marble widget
> > > > >> > +
> > > > >> > +#include <QWidget>
> > > > >> > +
> > > > >> > +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
> > > > >> > #endif // GLOBE_H
> > > > >> > diff --git a/qt-ui/maintab.h b/qt-ui/maintab.h
> > > > >> > index 946b673..fda74dc 100644
> > > > >> > --- a/qt-ui/maintab.h
> > > > >> > +++ b/qt-ui/maintab.h
> > > > >> > @@ -9,6 +9,7 @@
> > > > >> >
> > > > >> > #include <QTabWidget>
> > > > >> > #include <QDialog>
> > > > >> > +#include <QSettings>
> > > > >> > #include <QMap>
> > > > >> >
> > > > >> > #include "models.h"
> > > > >> > diff --git a/subsurface-configure.pri b/subsurface-configure.pri
> > > > >> > index 8e6aead..8d38934 100644
> > > > >> > --- a/subsurface-configure.pri
> > > > >> > +++ b/subsurface-configure.pri
> > > > >> > @@ -148,6 +148,12 @@ win32 {
> > > > >> > DEFINES -= UNICODE
> > > > >> > }
> > > > >> >
> > > > >> > +# Optional Marble
> > > > >> > +no-marble {
> > > > >> > + DEFINES += NO_MARBLE
> > > > >> > + LIBS -= -lmarblewidget
> > > > >> > +}
> > > > >> > +
> > > > >> > #
> > > > >> > # misc
> > > > >> > #
> > > > >> > --
> > > > >> > 1.9.0
> > > > >> >
> > > > >>
> > > > >>
> > > > >> --
> > > > >> Anton Lundin +46702-161604
> > > > >>
> > > > >
> > > > >
> > >
> > > --
> > > Anton Lundin +46702-161604
> > >
>
> --
> Anton Lundin +46702-161604
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.hohndel.org/pipermail/subsurface/attachments/20140307/ee9bdcab/attachment-0001.html>
More information about the subsurface
mailing list