[PATCH] Make Marble optional
Dirk Hohndel
dirk at hohndel.org
Fri Mar 7 08:21:42 PST 2014
Please stop top posting.
Either get a decent email client, or figure out how to do inline
responses with your existing client.
The occasional top post from some of the old hands, when answering
questions from a phone for example, is acceptable. But what you do
completely disrupts the flow of conversation.
On Fri, 2014-03-07 at 01:10 -0600, Alberto Corona wrote:
> I'll make sure to console the rest and see how they think this should
> be implemented.
I think Anton gave you some very clear feedback
> 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?
That was the point of making it optional. Some users do not track
locations for their dives (I talked to one user who says "every single
dive is at the exact same spot, 200m from my house - why do I need a map
widget?). Others do not want to deal with the massive size of
dependencies. Either way, there is value to be able to build without
marble, but that needs to be
- clean
- work on all our platforms
- not leave an artifact on the screen
If you don't know any Qt and cannot figure out how deal with the handful
of changes that are required to remove the empty widget, then maybe you
picked the wrong task.
This is about learning to contribute, learning to accept and implement
the things you receive in feedback. Ultimately the goal is to convince
us that we want to work with you through the next five months as part of
GSoC.
/D
> 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
>
>
> _______________________________________________
> subsurface mailing list
> subsurface at hohndel.org
> http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
More information about the subsurface
mailing list