Dear Fabien,

I fixed these items on Monday. Do you approve a merge now?

Kind regards,
Georg

On Mo, 21.07.2014, 19:07, Fabien Chéreau wrote:
> Review: Needs Fixing
>
> Please see comments
>
> Diff comments:
>
>> === modified file 'src/core/StelCore.cpp'
>> --- src/core/StelCore.cpp	2014-04-14 17:55:46 +0000
>> +++ src/core/StelCore.cpp	2014-07-20 13:42:31 +0000
>> @@ -114,7 +114,13 @@
>>  	defaultLocationID =
>> conf->value("init_location/location","error").toString();
>>  	bool ok;
>>  	StelLocationMgr* locationMgr =
>> &StelApp::getInstance().getLocationMgr();
>> -	StelLocation location =
>> locationMgr->locationForString(defaultLocationID);
>> +	StelLocation location;
>> +	if (conf->value("init_location/use_ip_geolocation_if_available",
>> "false").toBool())
>> +	{
>> +		locationMgr->locationFromIP();
>> +	}
>> +
>> +	else location = locationMgr->locationForString(defaultLocationID);
>>  	if (!location.isValid())
>>  	{
>>  		qWarning() << "Warning: location" << defaultLocationID << "is
>> unknown.";
>>
>> === modified file 'src/core/StelLocation.cpp'
>> --- src/core/StelLocation.cpp	2014-02-15 15:37:23 +0000
>> +++ src/core/StelLocation.cpp	2014-07-20 13:42:31 +0000
>> @@ -125,3 +125,10 @@
>>  	return loc;
>>  }
>>
>> +// Compute great-circle distance between two locations
>> +float StelLocation::distanceDegrees(const float long1, const float
>> lat1, const float long2, const float lat2)
>> +{
>> +	const float DEGREES=M_PI/180.0f;
>> +	return std::acos( std::sin(lat1*DEGREES)*std::sin(lat2*DEGREES) +
>> +
>> std::cos(lat1*DEGREES)*std::cos(lat2*DEGREES)*std::cos((long1-long2)*DEGREES)
>> ) / DEGREES;
>> +}
>>
>> === modified file 'src/core/StelLocation.hpp'
>> --- src/core/StelLocation.hpp	2014-04-15 17:10:07 +0000
>> +++ src/core/StelLocation.hpp	2014-07-20 13:42:31 +0000
>> @@ -74,6 +74,9 @@
>>  	//! Parse a location from a line serialization
>>  	static StelLocation createFromLine(const QString& line);
>>
>> +	//! Compute great-circle distance between two locations
>> +	static float distanceDegrees(const float long1, const float lat1,
>> const float long2, const float lat2);
>> +
>>  	//! Used privately by the StelLocationMgr
>>  	bool isUserLocation;
>>
>>
>> === modified file 'src/core/StelLocationMgr.cpp'
>> --- src/core/StelLocationMgr.cpp	2014-06-12 15:47:22 +0000
>> +++ src/core/StelLocationMgr.cpp	2014-07-20 13:42:31 +0000
>> @@ -17,6 +17,7 @@
>>   */
>>
>>  #include "StelApp.hpp"
>> +#include "StelCore.hpp"
>>  #include "StelFileMgr.hpp"
>>  #include "StelLocationMgr.hpp"
>>  #include "StelUtils.hpp"
>> @@ -25,6 +26,12 @@
>>  #include <QDebug>
>>  #include <QFile>
>>  #include <QDir>
>> +#include <QtNetwork/QNetworkInterface>
>> +#include <QtNetwork/QNetworkAccessManager>
>> +#include <QNetworkRequest>
>> +#include <QNetworkReply>
>> +#include <QUrl>
>> +#include <QUrlQuery>
>>
>>  StelLocationMgr::StelLocationMgr()
>>  {
>> @@ -36,9 +43,12 @@
>>
>>  	modelAllLocation = new QStringListModel(this);
>>  	modelAllLocation->setStringList(locations.keys());
>> +	modelPickedLocation = new QStringListModel(this); // keep empty for
>> now.
>>
>>  	// Init to Paris France because it's the center of the world.
>>  	lastResortLocation = locationForString("Paris, France");
>> +	networkAccessMgr = new QNetworkAccessManager(this);
>
> use the common instance of QNetworkAccessManager instead
> (StelApp::getInstance().getNetworkAccessManager()), and connect only the
> signals from the reply object itself
>
>> +	connect(networkAccessMgr, SIGNAL(finished(QNetworkReply*)), this,
>> SLOT(changeLocationFromNetworkLookup(QNetworkReply*)));
>>  }
>>
>>  void StelLocationMgr::generateBinaryLocationFile(const QString&
>> fileName, bool isUserLocation, const QString& binFilePath) const
>> @@ -144,6 +154,9 @@
>>
>>  StelLocationMgr::~StelLocationMgr()
>>  {
>> +	delete modelPickedLocation;
>> +	delete modelAllLocation;
>> +	delete networkAccessMgr;
>
> Please set to NULL after delete, also this last line becomes useless after
> applying previous comment
>
>>  }
>>
>>  static float parseAngle(const QString& s, bool* ok)
>> @@ -307,3 +320,71 @@
>>  	sourcefile.close();
>>  	return true;
>>  }
>> +
>> +// lookup location from IP address.
>> +void StelLocationMgr::locationFromIP()
>> +{
>> +    QNetworkRequest req( QUrl( QString("http://freegeoip.net/csv/") )
>> );
>> +    networkAccessMgr->get(req);
>> +}
>> +
>> +// slot that receives IP-based location data from the network.
>> +void StelLocationMgr::changeLocationFromNetworkLookup(QNetworkReply
>> *reply)
>> +{
>> +    StelLocation location;
>> +    if (reply->error() == QNetworkReply::NoError) {
>> +        //success
>
> you probably need to add more error checking code here. You have to be
> very careful as you are using replies from a service which may change,
> fail etc so it needs to be very robust, at least check that the format is
> what you expect.. Also please do test without network, etc..
>
>> +        QByteArray answer=reply->readAll();
>> +        const QStringList& splitline = QString(answer).split(",");
>> +        float latitude=splitline.at(7).mid(1,
>> splitline.at(7).length()-2).toFloat();
>> +        float longitude=splitline.at(8).mid(1,
>> splitline.at(8).length()-2).toFloat();
>> +        // answer/splitline example:
>> "222.222.222.222","AT","Austria","","","","","47.3333","13.3333","",""
>> +        // The parts from freegeoip are:
>> ip,country_code,country_name,region_code,region_name,city,zipcode,latitude,longitude,metro_code,area_code
>> +        QString locLine= // we re-pack into a new line that will be
>> parsed back by StelLocation...
>
> this is a bit dirty.. but well..
>
>> +                QString("%1\t%2\t%3\t%4\t%5\t%6\t%7\t0")
>> +                    .arg(splitline.at(5).length()>2 ?
>> splitline.at(5).mid(1, splitline.at(5).length()-2)  :
>> QString("IP%1").arg(splitline.at(0).mid(1, splitline.at(0).length()-2)))
>> +                    .arg(splitline.at(4).length()>2 ?
>> splitline.at(4).mid(1, splitline.at(4).length()-2)  : "IPregion")
>> +                    .arg(splitline.at(2).length()>2 ?
>> splitline.at(2).mid(1, splitline.at(2).length()-2) : "IPcountry") //
>> country
>> +                    .arg("X") // role: X=user-defined
>> +                    .arg(0)   // population: unknown
>> +                    .arg(latitude<0 ? QString("%1S").arg(-latitude, 0,
>> 'f', 6) : QString("%1N").arg(latitude, 0, 'f', 6))
>> +                    .arg(longitude<0 ? QString("%1W").arg(-longitude,
>> 0, 'f', 6) : QString("%1E").arg(longitude, 0, 'f', 6));
>> +        location=StelLocation::createFromLine(locLine);
>> +        StelCore *core=StelApp::getInstance().getCore();
>> +        core->moveObserverTo(location, 0.0f, 0.0f);
>> +    }
>> +    else {
>> +        qDebug() << "Failure getting IP-based location: \n\t"
>> <<reply->errorString();
>> +    }
>> +    reply->deleteLater();
>> +}
>> +
>> +void StelLocationMgr::pickLocationsNearby(const QString planetName,
>> const float longitude, const float latitude, const float radiusDegrees)
>
> unnecessary const in function parameters
>
>> +{
>> +    pickedLocations.clear();
>> +    QMapIterator<QString, StelLocation> iter(locations);
>> +    while (iter.hasNext())
>> +    {
>> +        iter.next();
>> +        StelLocation loc=iter.value();
>> +        if ( (loc.planetName == planetName) &&
>> +             (StelLocation::distanceDegrees(longitude, latitude,
>> loc.longitude, loc.latitude) <= radiusDegrees) )
>> +        {
>> +            pickedLocations.insert(iter.key(), iter.value());
>> +        }
>> +    }
>> +    modelPickedLocation->setStringList(pickedLocations.keys());
>> +}
>> +
>> +void StelLocationMgr::pickLocationsInCountry(const QString country)
>> +{
>> +    pickedLocations.clear();
>> +    QMapIterator<QString, StelLocation> iter(locations);
>> +    while (iter.hasNext())
>> +    {
>> +        iter.next();
>> +        StelLocation loc=iter.value();
>
> use const & to avoid copy
>
>> +        if (loc.country == country) pickedLocations.insert(iter.key(),
>> iter.value());
>> +    }
>> +    modelPickedLocation->setStringList(pickedLocations.keys());
>> +}
>>
>> === modified file 'src/core/StelLocationMgr.hpp'
>> --- src/core/StelLocationMgr.hpp	2013-10-14 07:43:06 +0000
>> +++ src/core/StelLocationMgr.hpp	2014-07-20 13:42:31 +0000
>> @@ -26,6 +26,8 @@
>>  #include <QMap>
>>
>>  class QStringListModel;
>> +class QNetworkReply;
>> +class QNetworkAccessManager;
>>
>>  //! @class StelLocationMgr
>>  //! Manage the list of available location.
>> @@ -41,6 +43,8 @@
>>
>>  	//! Return the model containing all the city
>>  	QStringListModel* getModelAll() {return modelAllLocation;}
>> +	//! Return the model containing picked (nearby) cities or cities from
>> a single country, or other preselection.
>> +	QStringListModel* getModelPicked() {return modelPickedLocation;}
>>
>>  	//! Return the list of all loaded locations
>>  	QList<StelLocation> getAll() const {return locations.values();}
>> @@ -70,6 +74,20 @@
>>  	//! @param id the location ID
>>  	bool deleteUserLocation(const QString& id);
>>
>> +	//! Find location via online lookup of IP address
>> +	void locationFromIP();
>> +
>> +	//! Preselect list of locations within @param radiusDegrees of
>> selected (usually screen-clicked) coordinates.
>> +	//! The list can be retrieved by calling @name getModelPicked().
>> +	void pickLocationsNearby(const QString planetName, const float
>> longitude, const float latitude, const float radiusDegrees);
>
> somehow these are 2 independent features, it would be cleaner to merge
> this in 2 times
>
>> +	//! Preselect list of locations in a particular country only.
>> +	//! The list can be retrieved by calling @name getModelPicked().
>> +	void pickLocationsInCountry(const QString country);
>> +
>> +public slots:
>> +	//! Process answer from online lookup of IP address
>> +	void changeLocationFromNetworkLookup(QNetworkReply *reply);
>> +
>>  private:
>>  	void generateBinaryLocationFile(const QString& txtFile, bool
>> isUserLocation, const QString& binFile) const;
>>
>> @@ -79,11 +97,19 @@
>>
>>  	//! Model containing all the city information
>>  	QStringListModel* modelAllLocation;
>> +	//! Model containing selected city information
>> +	QStringListModel* modelPickedLocation;
>>
>>  	//! The list of all loaded locations
>>  	QMap<QString, StelLocation> locations;
>> +	//! A list of locations generated on-the-fly by filtering from @name
>> locations
>> +	QMap<QString, StelLocation> pickedLocations;
>>
>>  	StelLocation lastResortLocation;
>> +
>> +	//!	for IP-based location lookup
>> +	QNetworkAccessManager *networkAccessMgr;
>> +
>>  };
>>
>>  #endif // _STELLOCATIONMGR_HPP_
>>
>> === modified file 'src/gui/LocationDialog.cpp'
>> --- src/gui/LocationDialog.cpp	2014-03-21 18:03:55 +0000
>> +++ src/gui/LocationDialog.cpp	2014-07-20 13:42:31 +0000
>> @@ -41,6 +41,7 @@
>>  #include <QTimer>
>>  #include <QStringListModel>
>>
>> +#include <QMessageBox>
>>  LocationDialog::LocationDialog(QObject* parent) : StelDialog(parent),
>> isEditingNew(false)
>>  {
>>  	ui = new Ui_locationDialogForm;
>> @@ -100,6 +101,9 @@
>>
>>  	connect(ui->addLocationToListPushButton, SIGNAL(clicked()), this,
>> SLOT(addCurrentLocationToList()));
>>  	connect(ui->deleteLocationFromListPushButton, SIGNAL(clicked()), this,
>> SLOT(deleteCurrentLocationFromList()));
>> +	connect(ui->ipQueryPushButton, SIGNAL(clicked()), this,
>> SLOT(ipQueryLocation()));
>> +	connect(ui->resetListPushButton, SIGNAL(clicked()), this,
>> SLOT(resetCompleteList()));
>> +	connect(ui->countryNameComboBox, SIGNAL(activated(const QString &)),
>> this, SLOT(filterSitesByCountry()));
>>
>>  	StelCore* core = StelApp::getInstance().getCore();
>>  	const StelLocation& currentLocation = core->getCurrentLocation();
>> @@ -235,7 +239,7 @@
>>  // Update the map for the given location.
>>  void LocationDialog::setMapForLocation(const StelLocation& loc)
>>  {
>> -	// Avoids usless processing
>> +	// Avoids useless processing
>>  	if (lastPlanet==loc.planetName)
>>  		return;
>>
>> @@ -358,6 +362,15 @@
>>  	loc.longitude = longitude;
>>  	setFieldsFromLocation(loc);
>>  	StelApp::getInstance().getCore()->moveObserverTo(loc, 0.);
>> +	// GZ: Filter location list for nearby sites. I assume Earth locations
>> are better known. With only few locations on other planets in the list,
>> 30 degrees seem OK.
>> +	StelApp::getInstance().getLocationMgr().pickLocationsNearby(loc.planetName,
>> longitude, latitude, loc.planetName=="Earth" ? 3.0f: 30.0f);
>> +	QSortFilterProxyModel *proxyModel = new QSortFilterProxyModel(this);
>> +	proxyModel->setSourceModel((QAbstractItemModel*)StelApp::getInstance().getLocationMgr().getModelPicked());
>> +	proxyModel->sort(0, Qt::AscendingOrder);
>> +	proxyModel->setFilterCaseSensitivity(Qt::CaseInsensitive);
>> +	ui->citiesListView->setModel(proxyModel);
>> +	ui->citySearchLineEdit->clear();
>> +	connect(ui->citySearchLineEdit, SIGNAL(textChanged(const QString&)),
>> proxyModel, SLOT(setFilterWildcard(const QString&)));
>>  }
>>
>>  // Called when the planet name is changed by hand
>> @@ -380,6 +393,24 @@
>>  				ls->setCurrentLandscapeID(ls->getDefaultLandscapeID());
>>  		}
>>
>> +		// GZ populate site list with sites only from that planet, or full
>> list for Earth (faster than removing the ~50 non-Earth positions...).
>> +		StelLocationMgr &locMgr=StelApp::getInstance().getLocationMgr();
>> +		QSortFilterProxyModel *proxyModel = new QSortFilterProxyModel(this);
>> +		if (loc.planetName == "Earth")
>> +		{
>> +			proxyModel->setSourceModel((QAbstractItemModel*)locMgr.getModelAll());
>> +		}
>> +		else
>> +		{
>> +			locMgr.pickLocationsNearby(loc.planetName, 0.0f, 0.0f, 180.0f);
>> +			proxyModel->setSourceModel((QAbstractItemModel*)locMgr.getModelPicked());
>> +		}
>> +		proxyModel->sort(0, Qt::AscendingOrder);
>> +		proxyModel->setFilterCaseSensitivity(Qt::CaseInsensitive);
>> +		ui->citiesListView->setModel(proxyModel);
>> +		ui->citySearchLineEdit->clear();
>> +		connect(ui->citySearchLineEdit, SIGNAL(textChanged(const QString&)),
>> proxyModel, SLOT(setFilterWildcard(const QString&)));
>> +		ui->citySearchLineEdit->setFocus();
>>  	}
>>  	// Planet transition time also set to null to prevent uglyness when
>>  	// "use landscape location" is enabled for that planet's landscape.
>> --BM
>> @@ -429,7 +460,7 @@
>>  	ui->deleteLocationFromListPushButton->setEnabled(locationMgr.canDeleteUserLocation(loc.getID()));
>>  }
>>
>> -// Called when the user clic on the save button
>> +// Called when the user clicks on the save button
>>  void LocationDialog::addCurrentLocationToList()
>>  {
>>  	const StelLocation& loc = locationFromFields();
>> @@ -487,3 +518,42 @@
>>  	ui->useAsDefaultLocationCheckBox->setEnabled(!currentIsDefault);
>>  	ui->pushButtonReturnToDefault->setEnabled(!currentIsDefault);
>>  }
>> +
>> +// called when the user clicks on the IP Query button
>> +void LocationDialog::ipQueryLocation()
>> +{
>> +	StelLocationMgr &locMgr=StelApp::getInstance().getLocationMgr();
>> +	locMgr.locationFromIP(); // This just triggers asynchronous lookup.
>> +	ui->citySearchLineEdit->setFocus();
>> +}
>> +
>> +// called when user clicks "reset list"
>> +void LocationDialog::resetCompleteList()
>> +{
>> +	QSortFilterProxyModel *proxyModel = new QSortFilterProxyModel(this);
>> +	proxyModel->setSourceModel((QAbstractItemModel*)StelApp::getInstance().getLocationMgr().getModelAll());
>> +	proxyModel->sort(0, Qt::AscendingOrder);
>> +	proxyModel->setFilterCaseSensitivity(Qt::CaseInsensitive);
>> +	ui->citiesListView->setModel(proxyModel);
>> +	ui->citySearchLineEdit->clear();
>> +	connect(ui->citySearchLineEdit, SIGNAL(textChanged(const QString&)),
>> proxyModel, SLOT(setFilterWildcard(const QString&)));
>> +	ui->citySearchLineEdit->setFocus();
>> +}
>> +
>> +// called when user clicks in the country combobox and selects a
>> country. The locations in the list are updated to select only sites in
>> that country.
>> +void LocationDialog::filterSitesByCountry()
>> +{
>> +	QString country=ui->countryNameComboBox->currentData().toString();
>> +	QSortFilterProxyModel *proxyModel = new QSortFilterProxyModel(this);
>> +	StelLocationMgr &locMgr=StelApp::getInstance().getLocationMgr();
>> +
>> +	locMgr.pickLocationsInCountry(country);
>> +	proxyModel->setSourceModel((QAbstractItemModel*)locMgr.getModelPicked());
>> +
>> +	proxyModel->sort(0, Qt::AscendingOrder);
>> +	proxyModel->setFilterCaseSensitivity(Qt::CaseInsensitive);
>> +	ui->citiesListView->setModel(proxyModel);
>> +	ui->citySearchLineEdit->clear();
>> +	connect(ui->citySearchLineEdit, SIGNAL(textChanged(const QString&)),
>> proxyModel, SLOT(setFilterWildcard(const QString&)));
>> +	ui->citySearchLineEdit->setFocus();
>> +}
>>
>> === modified file 'src/gui/LocationDialog.hpp'
>> --- src/gui/LocationDialog.hpp	2013-10-14 09:27:54 +0000
>> +++ src/gui/LocationDialog.hpp	2014-07-20 13:42:31 +0000
>> @@ -82,6 +82,7 @@
>>  	void updateFromProgram(const StelLocation& location);
>>
>>  	//! Called when the map is clicked.
>> +	//! GZ_New: create new list for places nearby and feed into location
>> list box.
>>  	void setPositionFromMap(double longitude, double latitude);
>>
>>  	//! Called when the user activates an item from the locations list.
>> @@ -97,6 +98,15 @@
>>
>>  	//! Called when the user clicks on the delete button
>>  	void deleteCurrentLocationFromList();
>> +
>> +	//! filter city list to show entries from single country only
>> +	void filterSitesByCountry();
>> +
>> +	//! reset city list to complete list (may have been reduced to picked
>> list)
>> +	void resetCompleteList();
>> +
>> +	//! called when the user clicks on the IP Query button
>> +	void ipQueryLocation();
>>
>>  	//! Called when the user wants to use the current location as default
>>  	void setDefaultLocation();
>>
>> === modified file 'src/gui/locationDialogGui.ui'
>> --- src/gui/locationDialogGui.ui	2014-06-16 13:43:31 +0000
>> +++ src/gui/locationDialogGui.ui	2014-07-20 13:42:31 +0000
>> @@ -359,6 +359,16 @@
>>                </property>
>>               </widget>
>>              </item>
>> +            <item>
>> +             <widget class="QPushButton" name="resetListPushButton">
>> +              <property name="toolTip">
>> +               <string>Reset location list to show all known
>> locations</string>
>> +              </property>
>> +              <property name="text">
>> +               <string>Reset Location List</string>
>> +              </property>
>> +             </widget>
>> +            </item>
>>             </layout>
>>            </widget>
>>           </item>
>> @@ -449,6 +459,22 @@
>>               </spacer>
>>              </item>
>>              <item>
>> +             <widget class="QPushButton" name="ipQueryPushButton">
>> +              <property name="minimumSize">
>> +               <size>
>> +                <width>30</width>
>> +                <height>0</height>
>> +               </size>
>> +              </property>
>> +              <property name="toolTip">
>> +               <string>Use online service to find approximate location
>> based on your IP address</string>
>> +              </property>
>> +              <property name="text">
>> +               <string>IP Query</string>
>> +              </property>
>> +             </widget>
>> +            </item>
>> +            <item>
>>               <widget class="QPushButton"
>> name="deleteLocationFromListPushButton">
>>                <property name="enabled">
>>                 <bool>false</bool>
>>
>
>
> --
> https://code.launchpad.net/~georg-zotti/stellarium/ip-query-location/+merge/227418
> You are the owner of lp:~georg-zotti/stellarium/ip-query-location.
>