From 4e1db5c83786fcb6673ff97a6bf868d658890940 Mon Sep 17 00:00:00 2001 From: Klaus Basan Date: Sun, 18 Jan 2015 22:02:07 +0100 Subject: [PATCH] Fixes of MS'review as proposed https://dev.vatsim-germany.org/issues/368#change-2380 (1,2) https://dev.vatsim-germany.org/issues/364#change-2379 (1-11,13) --- src/blackcore/context_ownaircraft_impl.cpp | 4 +- src/blackcore/context_simulator_impl.cpp | 2 +- src/blackgui/components/mappingcomponent.cpp | 2 +- src/blackmisc/avaircrafticao.cpp | 2 +- src/blackmisc/avcallsign.cpp | 3 +- src/blackmisc/nwaircraftmapping.cpp | 2 +- src/blackmisc/nwaircraftmappinglist.cpp | 2 +- src/blackmisc/nwaircraftmappinglist.h | 2 +- src/blackmisc/nwuser.cpp | 46 +++++++++++++++---- src/blackmisc/nwuser.h | 5 +- src/blackmisc/pixmap.cpp | 17 ++++++- src/blackmisc/pixmap.h | 9 ++++ src/blackmisc/pqstring.cpp | 16 ++++--- .../simulation/aircraftmodellist.cpp | 13 ++---- src/blackmisc/simulation/aircraftmodellist.h | 2 +- .../simulation/simulatedaircraft.cpp | 9 ++-- src/blackmisc/simulation/simulatedaircraft.h | 6 +-- src/blackmisc/threadedreader.h | 1 - .../fscommon/aircraftcfgentrieslist.h | 4 +- 19 files changed, 100 insertions(+), 47 deletions(-) diff --git a/src/blackcore/context_ownaircraft_impl.cpp b/src/blackcore/context_ownaircraft_impl.cpp index d094d3c4b..7206a337b 100644 --- a/src/blackcore/context_ownaircraft_impl.cpp +++ b/src/blackcore/context_ownaircraft_impl.cpp @@ -144,7 +144,7 @@ namespace BlackCore emit this->changedAircraft(aircraft, originator); // now set value - this->m_ownAircraft.update(aircraft); + this->m_ownAircraft.setAircraft(aircraft); changed = true; } return changed; @@ -421,7 +421,7 @@ namespace BlackCore */ const CAircraft &CContextOwnAircraft::getAviationAircraft() const { - return static_cast(this->m_ownAircraft); + return this->m_ownAircraft; } } // namespace diff --git a/src/blackcore/context_simulator_impl.cpp b/src/blackcore/context_simulator_impl.cpp index d941600d3..07e2adf41 100644 --- a/src/blackcore/context_simulator_impl.cpp +++ b/src/blackcore/context_simulator_impl.cpp @@ -291,7 +291,7 @@ namespace BlackCore if (m_simulator) { // depending on shutdown order, network might already have been deleted - //! \todo why do we need to disconnet context when we unload driver? + //! \todo link with airspace monitor to be reviewed when airspace monitor becomes context if (this->getRuntime()->getIContextNetwork()->isUsingImplementingObject()) { CContextNetwork *network = this->getRuntime()->getCContextNetwork(); diff --git a/src/blackgui/components/mappingcomponent.cpp b/src/blackgui/components/mappingcomponent.cpp index 68d928a33..e5816a1c6 100644 --- a/src/blackgui/components/mappingcomponent.cpp +++ b/src/blackgui/components/mappingcomponent.cpp @@ -291,7 +291,7 @@ namespace BlackGui this->ui->tvp_AircraftModels->updateContainer(ml); // model completer - this->m_modelCompleter->setModel(new QStringListModel(ml.getModelStrings(), this->m_modelCompleter)); + this->m_modelCompleter->setModel(new QStringListModel(ml.getSortedModelStrings(), this->m_modelCompleter)); this->m_modelCompleter->setModelSorting(QCompleter::CaseInsensitivelySortedModel); this->m_modelCompleter->setCaseSensitivity(Qt::CaseInsensitive); this->m_modelCompleter->setWrapAround(true); diff --git a/src/blackmisc/avaircrafticao.cpp b/src/blackmisc/avaircrafticao.cpp index 22cfd5008..565a01b63 100644 --- a/src/blackmisc/avaircrafticao.cpp +++ b/src/blackmisc/avaircrafticao.cpp @@ -37,7 +37,7 @@ namespace BlackMisc bool CAircraftIcao::hasKnownAircraftDesignator() const { - return (this->getAircraftDesignator() != "ZZZZ"); + return (this->hasAircraftDesignator() && this->getAircraftDesignator() != "ZZZZ"); } QString CAircraftIcao::asString() const diff --git a/src/blackmisc/avcallsign.cpp b/src/blackmisc/avcallsign.cpp index ec1d0c69e..d06127f14 100644 --- a/src/blackmisc/avcallsign.cpp +++ b/src/blackmisc/avcallsign.cpp @@ -179,8 +179,7 @@ namespace BlackMisc */ bool CCallsign::isValidCallsign(const QString &callsign) { - //! \todo sometimes callsigns such as 12345, really correct? - // static QRegularExpression regexp("^[A-Z]+[A-Z0-9]*$"); + // We allow all number callsigns static QRegularExpression regexp("^[A-Z0-9]*$"); if (callsign.length() < 2 || callsign.length() > 10) { return false; } return (regexp.match(callsign).hasMatch()); diff --git a/src/blackmisc/nwaircraftmapping.cpp b/src/blackmisc/nwaircraftmapping.cpp index 736a5b605..d38575f5f 100644 --- a/src/blackmisc/nwaircraftmapping.cpp +++ b/src/blackmisc/nwaircraftmapping.cpp @@ -23,7 +23,7 @@ namespace BlackMisc * Constructor */ CAircraftMapping::CAircraftMapping(const QString &source, const QString &packageName, const QString &aircraftDesignator, const QString &airlineDesignator, const QString &model) : - m_source(source.trimmed()), m_packageName(packageName.trimmed()), m_icao(CAircraftIcao(aircraftDesignator, airlineDesignator)), m_model(BlackMisc::Simulation::CAircraftModel(model, BlackMisc::Simulation::CAircraftModel::TypeModelMapping)) + m_source(source.trimmed()), m_packageName(packageName.trimmed()), m_icao(aircraftDesignator, airlineDesignator), m_model(model, BlackMisc::Simulation::CAircraftModel::TypeModelMapping) { } /* diff --git a/src/blackmisc/nwaircraftmappinglist.cpp b/src/blackmisc/nwaircraftmappinglist.cpp index ec2ec1289..4da8cca3c 100644 --- a/src/blackmisc/nwaircraftmappinglist.cpp +++ b/src/blackmisc/nwaircraftmappinglist.cpp @@ -72,7 +72,7 @@ namespace BlackMisc return this->findBy(&CAircraftMapping::getIcao, searchIcao); } - CAircraftMappingList CAircraftMappingList::findByModelString(const QString modelString, Qt::CaseSensitivity sensitivity) const + CAircraftMappingList CAircraftMappingList::findByModelString(const QString &modelString, Qt::CaseSensitivity sensitivity) const { return this->findBy([ = ](const CAircraftMapping & mapping) { diff --git a/src/blackmisc/nwaircraftmappinglist.h b/src/blackmisc/nwaircraftmappinglist.h index f07c63fd5..b74090f00 100644 --- a/src/blackmisc/nwaircraftmappinglist.h +++ b/src/blackmisc/nwaircraftmappinglist.h @@ -49,7 +49,7 @@ namespace BlackMisc CAircraftMappingList findByIcaoCodeExact(const BlackMisc::Aviation::CAircraftIcao &searchIcao) const; //! Find by model string - CAircraftMappingList findByModelString(const QString modelString, Qt::CaseSensitivity sensitivity = Qt::CaseInsensitive) const; + CAircraftMappingList findByModelString(const QString &modelString, Qt::CaseSensitivity sensitivity = Qt::CaseInsensitive) const; //! \copydoc CValueObject::toQVariant virtual QVariant toQVariant() const override { return QVariant::fromValue(*this); } diff --git a/src/blackmisc/nwuser.cpp b/src/blackmisc/nwuser.cpp index 1c9b58bfa..d3249a20a 100644 --- a/src/blackmisc/nwuser.cpp +++ b/src/blackmisc/nwuser.cpp @@ -52,10 +52,8 @@ namespace BlackMisc void CUser::setRealName(const QString &realname) { - // try to strip homebase - // I understand the limitations, but we will have more correct hits as failures I assume - const QRegularExpression reg("(-\\s*|\\s)([A-Z]{4})$"); - QString rn = realname.trimmed().simplified(); + + QString rn(realname.trimmed().simplified()); if (rn.isEmpty()) { this->m_realname = ""; @@ -64,8 +62,12 @@ namespace BlackMisc if (!this->hasValidHomebase()) { - //! only apply stripping if home base is not explicitly given - QRegularExpressionMatch match = reg.match(rn); + // only apply stripping if home base is not explicitly given + // try to strip homebase: I understand the limitations, but we will have more correct hits as failures I assume + static QThreadStorage tsRegex; + if (! tsRegex.hasLocalData()) { tsRegex.setLocalData(QRegularExpression("(-\\s*|\\s)([A-Z]{4})$")); } + const auto ®ex = tsRegex.localData(); + QRegularExpressionMatch match = regex.match(rn); if (match.hasMatch()) { int pos = match.capturedStart(0); @@ -74,7 +76,9 @@ namespace BlackMisc this->setHomebase(CAirportIcao(icao)); } } - this->m_realname = rn; + + // do not beautify before stripping home base + this->m_realname = beautifyRealName(rn); } CStatusMessageList CUser::validate() const @@ -140,6 +144,31 @@ namespace BlackMisc return i >= 100000 && i <= 9999999; } + QString CUser::beautifyRealName(const QString &realName) + { + QString newRealName(realName.simplified().trimmed().toLower()); + if (newRealName.isEmpty()) { return ""; } + + // simple title case + QString::Iterator i = newRealName.begin(); + bool upperNextChar = true; + while (i != newRealName.end()) + { + if (i->isSpace()) + { + upperNextChar = true; + } + else if (upperNextChar) + { + QChar u(i->toUpper()); + *i = u; + upperNextChar = false; + } + ++i; + } + return newRealName; + } + /* * Property by index */ @@ -166,9 +195,6 @@ namespace BlackMisc } } - /* - * Set property as index - */ void CUser::setPropertyByIndex(const CVariant &variant, const BlackMisc::CPropertyIndex &index) { if (index.isMyself()) diff --git a/src/blackmisc/nwuser.h b/src/blackmisc/nwuser.h index 4d809891a..1e217e6d1 100644 --- a/src/blackmisc/nwuser.h +++ b/src/blackmisc/nwuser.h @@ -52,7 +52,7 @@ namespace BlackMisc CUser(const QString &id, const QString &realname, const BlackMisc::Aviation::CCallsign &callsign); //! Constructor. - CUser(const QString &id, const QString &realname, const QString &email = "", const QString &password = "", const BlackMisc::Aviation::CCallsign &callsign = BlackMisc::Aviation::CCallsign()); + CUser(const QString &id, const QString &realname, const QString &email = "", const QString &password = "", const BlackMisc::Aviation::CCallsign &callsign = {}); //! Get full name. QString getRealName() const { return m_realname; } @@ -132,6 +132,9 @@ namespace BlackMisc //! Valid VATSIM id static bool isValidVatsimId(const QString &id); + //! Beautify real name, e.g. "JOE DoE" -> "Joe Doe"; + static QString beautifyRealName(const QString &realName); + protected: //! \copydoc CValueObject::convertToQString virtual QString convertToQString(bool i18n = false) const override; diff --git a/src/blackmisc/pixmap.cpp b/src/blackmisc/pixmap.cpp index 10d1b212f..6d462ca5e 100644 --- a/src/blackmisc/pixmap.cpp +++ b/src/blackmisc/pixmap.cpp @@ -9,6 +9,7 @@ #include "pixmap.h" #include +#include namespace BlackMisc { @@ -18,9 +19,22 @@ namespace BlackMisc this->fillByteArray(); } + CPixmap::CPixmap(const CPixmap &other) : CValueObjectStdTuple(), m_pixmap(other.m_pixmap), m_hasCachedPixmap(other.m_hasCachedPixmap), m_array(other.m_array) + {} + + CPixmap &CPixmap::operator =(const CPixmap &other) + { + std::tie(m_pixmap, m_hasCachedPixmap, m_array) = std::tie(other.m_pixmap, other.m_hasCachedPixmap, other.m_array); + return (*this); + } + const QPixmap &CPixmap::pixmap() const { + QWriteLocker(&this->m_lock); if (this->m_hasCachedPixmap) { return this->m_pixmap; } + + // this part here becomes relevant when marshalling via DBus is used + // in this case only the array is transferred this->m_hasCachedPixmap = true; if (this->m_array.isEmpty()) { return this->m_pixmap; } bool s = this->m_pixmap.loadFromData(this->m_array, "PNG"); @@ -31,13 +45,14 @@ namespace BlackMisc bool CPixmap::isNull() const { + QReadLocker(&this->m_lock); if (this->m_hasCachedPixmap) { return false; } return (this->m_array.isEmpty() || this->m_array.isNull()); } CPixmap::operator QPixmap() const { - return QPixmap(pixmap()); + return pixmap(); } QString CPixmap::convertToQString(bool i18n) const diff --git a/src/blackmisc/pixmap.h b/src/blackmisc/pixmap.h index 240df1a56..4a8bff7f8 100644 --- a/src/blackmisc/pixmap.h +++ b/src/blackmisc/pixmap.h @@ -14,6 +14,7 @@ #include "valueobject.h" #include +#include namespace BlackMisc { @@ -28,6 +29,12 @@ namespace BlackMisc //! Constructor. CPixmap(const QPixmap &pixmap); + //! Copy constructor (because of mutex) + CPixmap(const CPixmap &other); + + //! Copy assignment (because of mutex) + CPixmap &operator =(const CPixmap &other); + //! Corresponding pixmap const QPixmap &pixmap() const; @@ -52,6 +59,8 @@ namespace BlackMisc mutable QPixmap m_pixmap; //!< cached pixmap, mutable because of lazy initialization mutable bool m_hasCachedPixmap = false; //!< pixmap? Mutable because of lazy initialization + mutable QReadWriteLock m_lock; //!< lock (because of mutable members) + QByteArray m_array; //!< data of pixmap }; } // namespace diff --git a/src/blackmisc/pqstring.cpp b/src/blackmisc/pqstring.cpp index 4fbceb13c..10ca73f54 100644 --- a/src/blackmisc/pqstring.cpp +++ b/src/blackmisc/pqstring.cpp @@ -10,6 +10,7 @@ #include "pqstring.h" #include "tuple.h" #include "pqallquantities.h" +#include namespace BlackMisc { @@ -28,24 +29,27 @@ namespace BlackMisc */ CVariant CPqString::parseToVariant(const QString &value, SeparatorMode mode) { - static QRegExp rx("([-+]?[0-9]*[\\.,]?[0-9]+)\\s*(\\D*)$"); CVariant v; // fine tuning of the string QString vs = value.trimmed().simplified(); // check - if (vs.isEmpty()) return v; + if (vs.isEmpty()) { return v; } - if (rx.indexIn(value) < 0) return v; // not a valid number - QString unit = rx.cap(2).trimmed(); + static QThreadStorage tsRegex; + if (! tsRegex.hasLocalData()) { tsRegex.setLocalData(QRegExp("([-+]?[0-9]*[\\.,]?[0-9]+)\\s*(\\D*)$")); } + const auto ®ex = tsRegex.localData(); + + if (regex.indexIn(value) < 0) { return v; } // not a valid number + QString unit = regex.cap(2).trimmed(); QString number = QString(value).replace(unit, ""); unit = unit.trimmed(); // trim after replace, not before - if (unit.isEmpty() || number.isEmpty()) return v; + if (unit.isEmpty() || number.isEmpty()) { return v; } bool success; double numberD = parseNumber(number, success, mode); - if (!success) return v; + if (!success) {return v; } if (CMeasurementUnit::isValidUnitSymbol(unit)) { diff --git a/src/blackmisc/simulation/aircraftmodellist.cpp b/src/blackmisc/simulation/aircraftmodellist.cpp index d04cd6537..974286d51 100644 --- a/src/blackmisc/simulation/aircraftmodellist.cpp +++ b/src/blackmisc/simulation/aircraftmodellist.cpp @@ -44,18 +44,13 @@ namespace BlackMisc CAircraftModelList CAircraftModelList::findModelsStartingWith(const QString &modelString, Qt::CaseSensitivity sensitivity) const { - CAircraftModelList ml; - for (const CAircraftModel &model : (*this)) + return this->findBy([ = ](const CAircraftModel & model) { - if (model.getModelString().startsWith(modelString, sensitivity)) - { - ml.push_back(model); - } - } - return ml; + return model.getModelString().startsWith(modelString, sensitivity); + }); } - QStringList CAircraftModelList::getModelStrings() const + QStringList CAircraftModelList::getSortedModelStrings() const { QStringList ms; for (const CAircraftModel &model : (*this)) diff --git a/src/blackmisc/simulation/aircraftmodellist.h b/src/blackmisc/simulation/aircraftmodellist.h index 18c115d37..3991a78d6 100644 --- a/src/blackmisc/simulation/aircraftmodellist.h +++ b/src/blackmisc/simulation/aircraftmodellist.h @@ -48,7 +48,7 @@ namespace BlackMisc CAircraftModelList findModelsStartingWith(const QString &modelString, Qt::CaseSensitivity sensitivity = Qt::CaseInsensitive) const; //! Model strings - QStringList getModelStrings() const; + QStringList getSortedModelStrings() const; //! \copydoc CValueObject::convertFromQVariant virtual void convertFromQVariant(const QVariant &variant) override { BlackMisc::setFromQVariant(this, variant); } diff --git a/src/blackmisc/simulation/simulatedaircraft.cpp b/src/blackmisc/simulation/simulatedaircraft.cpp index ea06dbc05..5f6d56cc7 100644 --- a/src/blackmisc/simulation/simulatedaircraft.cpp +++ b/src/blackmisc/simulation/simulatedaircraft.cpp @@ -121,10 +121,13 @@ namespace BlackMisc } //! \todo Smarter way to do this? - void CSimulatedAircraft::update(const CAircraft &aircraft) + void CSimulatedAircraft::setAircraft(const CAircraft &aircraft) { - // override - (*this) = CSimulatedAircraft(aircraft, this->getModel(), this->getClient()); + static_cast(*this) = aircraft; + this->m_model.setCallsign(aircraft.getCallsign()); + this->m_client.setAircraftModel(this->getModel()); + this->m_client.setUser(aircraft.getPilot()); + this->m_client.setUserCallsign(aircraft.getCallsign()); } QString CSimulatedAircraft::convertToQString(bool i18n) const diff --git a/src/blackmisc/simulation/simulatedaircraft.h b/src/blackmisc/simulation/simulatedaircraft.h index c0b1f5e12..9522aa485 100644 --- a/src/blackmisc/simulation/simulatedaircraft.h +++ b/src/blackmisc/simulation/simulatedaircraft.h @@ -38,8 +38,8 @@ namespace BlackMisc //! Constructor. CSimulatedAircraft(const BlackMisc::Aviation::CAircraft &aircraft, - const BlackMisc::Simulation::CAircraftModel &model = BlackMisc::Simulation::CAircraftModel(), - const BlackMisc::Network::CClient &client = BlackMisc::Network::CClient()); + const BlackMisc::Simulation::CAircraftModel &model = {}, + const BlackMisc::Network::CClient &client = {}); //! \copydoc CValueObject::propertyByIndex virtual CVariant propertyByIndex(const BlackMisc::CPropertyIndex &index) const override; @@ -75,7 +75,7 @@ namespace BlackMisc void setEnabled(bool enabled) { m_enabled = enabled; } //! Update from aviation aircraft - void update(const BlackMisc::Aviation::CAircraft &aircraft); + void setAircraft(const BlackMisc::Aviation::CAircraft &aircraft); protected: //! \copydoc CValueObject::convertToQString() diff --git a/src/blackmisc/threadedreader.h b/src/blackmisc/threadedreader.h index 54cf78306..d83501569 100644 --- a/src/blackmisc/threadedreader.h +++ b/src/blackmisc/threadedreader.h @@ -119,7 +119,6 @@ namespace BlackMisc private: QDateTime m_updateTimestamp; //!< when was file / resource read - QFuture m_pendingFuture; //!< optional future to be stopped QNetworkReply *m_pendingNetworkReply = nullptr; //!< optional network reply to be stopped }; diff --git a/src/blacksim/fscommon/aircraftcfgentrieslist.h b/src/blacksim/fscommon/aircraftcfgentrieslist.h index ee182764a..4d6afed77 100644 --- a/src/blacksim/fscommon/aircraftcfgentrieslist.h +++ b/src/blacksim/fscommon/aircraftcfgentrieslist.h @@ -96,13 +96,13 @@ namespace BlackSim //! Do not include the following directories for FS static const QStringList &excludeDirectories() { - static const QStringList exclude( + static const QStringList exclude { "SimObjects/Animals", "SimObjects/Misc", "SimObjects/GroundVehicles", "SimObjects/Boats" - }); + }; return exclude; }