A number of changes to make the PGC experience smooth (+ a couple bug…

…fixes)

Requesting @VBoettcher to review changes that touch his code (`SkyLabeler` and `CatalogsComponent`, especially)

The key changes here are:
1. Refactor how `CatalogsComponent::draw` works by handling objects of known and unknown magnitudes separately. This allows us to give precedence to labels attached to objects of known magnitude. It isn't yet perfect (i.e. labels are prioritized first by trixel and then by magnitude rather than by magnitude alone), but this is the best we can do without some stronger guarantees on memory availability.
2. Tweak heuristics for displaying objects of unknown magnitude.
3. Tweak labeler heuristics [Reviewers: Check that labels display as expected without PGC catalog]
4. Make find dialog DB queries happen in a separate thread (and not the GUI thread), to keep the UI responsive [Reviewers: Check that Find Dialog works as expected and does not crash]
5. Some minor improvements to the observation planner
6. Change the default policy on `SkyMapComposite::findByName` to do exact matches. [Reviewers: Check that Ekos works as expected]

kstars/catalogsdb/catalogsdb.cpp

@@ -173,6 +173,8 @@ DBManager::DBManager(const QString &filename)
 
     m_q_cat_by_id         = make_query(m_db, SqlStatements::get_catalog_by_id, true);
     m_q_obj_by_trixel     = make_query(m_db, SqlStatements::dso_by_trixel, false);
+    m_q_obj_by_trixel_no_nulls = make_query(m_db, SqlStatements::dso_by_trixel_no_nulls, false);
+    m_q_obj_by_trixel_null_mag = make_query(m_db, SqlStatements::dso_by_trixel_null_mag, false);
     m_q_obj_by_name       = make_query(m_db, SqlStatements::dso_by_name, true);
     m_q_obj_by_name_exact = make_query(m_db, SqlStatements::dso_by_name_exact, true);
     m_q_obj_by_maglim     = make_query(m_db, SqlStatements::dso_by_maglim, true);
@@ -434,35 +436,35 @@ CatalogObject DBManager::read_catalogobject(const QSqlQuery &query) const
              flux,       m_db_file };
 }
 
-CatalogObjectVector DBManager::get_objects_in_trixel(const int trixel)
+CatalogObjectVector DBManager::_get_objects_in_trixel_generic(QSqlQuery& query, const int trixel)
 {
     QMutexLocker _{ &m_mutex }; // this costs ~ .1ms which is ok
-    m_q_obj_by_trixel.bindValue(0, trixel);
+    query.bindValue(0, trixel);
 
-    if (!m_q_obj_by_trixel.exec()) // we throw because this is not recoverable
+    if (!query.exec()) // we throw because this is not recoverable
         throw DatabaseError(
-            QString("The query m_by_trixel_query for objects in trixel=%1 failed.")
+            QString("The by-trixel query for objects in trixel=%1 failed.")
                 .arg(trixel),
-            DatabaseError::ErrorType::UNKNOWN, m_q_obj_by_trixel.lastError());
+            DatabaseError::ErrorType::UNKNOWN, query.lastError());
 
     CatalogObjectVector objects;
     size_t count =
-        count_rows(m_q_obj_by_trixel); // this also moves the query head to the end
+        count_rows(query); // this also moves the query head to the end
 
     if (count == 0)
     {
-        m_q_obj_by_trixel.finish();
+        query.finish();
         return objects;
     }
 
     objects.reserve(count);
 
-    while (m_q_obj_by_trixel.previous())
+    while (query.previous())
     {
-        objects.push_back(read_catalogobject(m_q_obj_by_trixel));
+        objects.push_back(read_catalogobject(query));
     }
 
-    m_q_obj_by_trixel.finish();
+    query.finish();
 
     // move semantics baby!
     return objects;

kstars/catalogsdb/catalogsdb.h

@@ -15,9 +15,11 @@
 #include <catalogsdb_debug.h>
 #include <QSqlQuery>
 #include <QMutex>
+#include <QObject>
 
 #include "polyfills/qstring_hash.h"
 #include <unordered_map>
+#include <queue>
 
 #include <unordered_set>
 #include <utility>
@@ -200,6 +202,8 @@ class DBManager
         swap(m_db, other.m_db);
         swap(m_q_cat_by_id, other.m_q_cat_by_id);
         swap(m_q_obj_by_trixel, other.m_q_obj_by_trixel);
+        swap(m_q_obj_by_trixel_no_nulls, other.m_q_obj_by_trixel_no_nulls);
+        swap(m_q_obj_by_trixel_null_mag, other.m_q_obj_by_trixel_null_mag);
         swap(m_q_obj_by_name, other.m_q_obj_by_name);
         swap(m_q_obj_by_name_exact, other.m_q_obj_by_name_exact);
         swap(m_q_obj_by_maglim, other.m_q_obj_by_maglim);
@@ -244,7 +248,23 @@ class DBManager
     /**
      * @return return a vector of objects in the trixel with \p id.
      */
-    CatalogObjectVector get_objects_in_trixel(const int trixel);
+    inline CatalogObjectVector get_objects_in_trixel(const int trixel) {
+        return _get_objects_in_trixel_generic(m_q_obj_by_trixel, trixel);
+    }
+
+    /**
+     * @return return a vector of objects of known mag in the trixel with \p id.
+     */
+    inline CatalogObjectVector get_objects_in_trixel_no_nulls(const int trixel) {
+        return _get_objects_in_trixel_generic(m_q_obj_by_trixel_no_nulls, trixel);
+    }
+
+    /**
+     * @return return a vector of objects of unknown mag in the trixel with \p id.
+     */
+    inline CatalogObjectVector get_objects_in_trixel_null_mag(const int trixel) {
+        return _get_objects_in_trixel_generic(m_q_obj_by_trixel_null_mag, trixel);
+    }
 
     /**
      * \brief Find an objects by name.
@@ -541,6 +561,8 @@ class DBManager
 
     QSqlQuery m_q_cat_by_id;
     QSqlQuery m_q_obj_by_trixel;
+    QSqlQuery m_q_obj_by_trixel_null_mag;
+    QSqlQuery m_q_obj_by_trixel_no_nulls;
     QSqlQuery m_q_obj_by_name;
     QSqlQuery m_q_obj_by_name_exact;
     QSqlQuery m_q_obj_by_maglim;
@@ -634,6 +656,11 @@ class DBManager
      */
     std::pair<bool, QString> remove_catalog_force(const int id);
 
+    /**
+     *
+     */
+    CatalogObjectVector _get_objects_in_trixel_generic(QSqlQuery &query, const int trixel);
+
     /**
      * A list of database paths. The index gets stored in the
      * `CatalogObject` and can be used to retrieve the path to the
@@ -689,4 +716,175 @@ QString dso_db_path();
 /** \returns true and a catalog if the catalog metadata (name, author,
     ...) can be read */
 std::pair<bool, Catalog> read_catalog_meta_from_file(const QString &path);
+
+
+
+
+/**
+ * A concurrent wrapper around \sa CatalogsDB::DBManager
+ *
+ * This wrapper can be instantiated from the main thread. It spawns
+ * its own thread and moves itself to the thread, allowing the main
+ * thread to call DB operations without blocking the user
+ * interface. It provides a generic wrapper interface, \sa
+ * AsyncDBManager::execute(), to call the methods of DBManager.
+ *
+ * Since it is hard to use signal-slot communication with a generic
+ * wrapper like \sa AsyncDBManager::execute(), a wrapper is provided
+ * for the two most likely overloads of \sa
+ * DBManager::find_objects_by_name, which are time-consuming and
+ * frequently used, and these can be directly invoked from
+ * QMetaObject::invokeMethod or an appropriate signal
+ *
+ * The void override of \sa AsyncDBManager::init() does the most
+ * commonly done thing, which is to open the DSO database
+ *
+ */
+class AsyncDBManager : public QObject {
+    // Note: Follows the active object pattern described here
+    // https://youtu.be/SncJ3D-fO7g?list=PL6CJYn40gN6jgr-Rpl3J4XDQYhmUnxb-g&t=272
+    Q_OBJECT
+
+private:
+    std::shared_ptr<DBManager> m_manager;
+    std::queue<std::unique_ptr<CatalogObjectList>> m_result;
+    std::shared_ptr<QThread> m_thread;
+    QMutex m_resultMutex;
+
+public:
+    template <typename... Args>
+    using DBManagerMethod = CatalogObjectList (DBManager::*)(Args...);
+
+    /**
+     * Constructor, does nothing. Call init() to do the actual setup after the QThread stars
+     */
+    template <typename... Args>
+    AsyncDBManager(Args... args)
+        : QObject(nullptr)
+        , m_manager(nullptr)
+        , m_thread(nullptr)
+    {
+        m_thread.reset(new QThread);
+        moveToThread(m_thread.get());
+        connect(m_thread.get(), &QThread::started, [&]() {
+            init(args...);
+        });
+        m_thread->start();
+    }
+
+    ~AsyncDBManager()
+    {
+        QMetaObject::invokeMethod(this, "cleanup");
+        m_thread->wait();
+    }
+
+    /**
+     * Return a pointer to the DBManager, for non-DB functionalities
+     */
+    inline std::shared_ptr<DBManager> manager() { return m_manager; }
+
+    /**
+     * Return a pointer to the QThread object
+     */
+    inline std::shared_ptr<QThread> thread() { return m_thread; }
+
+    /**
+     * Construct the DBManager object
+     *
+     * Should be done in the thread corresponding to this object
+     */
+    template <typename... Args> void init(Args&&... args)
+    {
+        m_manager = std::make_shared<DBManager>(std::forward<Args>(args)...);
+    }
+
+    /**
+     * A generic wrapper to call any method on DBManager that returns a CatalogObjectList
+     *
+     * For practical use examples, see \sa
+     * AsyncDBManager::find_objects_by_name below which uses this
+     * wrapper
+     */
+    template <typename... Args>
+    void execute(DBManagerMethod<Args...> dbManagerMethod, Args... args)
+    {
+        // c.f. https://stackoverflow.com/questions/25392935/wrap-a-function-pointer-in-c-with-variadic-template
+
+        // N.B. The perfect forwarding idiom is not used because we
+        // also want to be able to bind to lvalue references, whereas
+        // the types deduced for the arguments has to match the type
+        // deduced to identify the function overload to be used.
+        QMutexLocker _{&m_resultMutex};
+        m_result.emplace(
+            std::make_unique<CatalogObjectList>((m_manager.get()->*dbManagerMethod)(args...))
+            );
+        emit resultReady();
+    }
+
+signals:
+    void resultReady(void);
+    void threadReady(void);
+
+
+public slots:
+
+    void init()
+    {
+        m_manager = std::make_shared<DBManager>(dso_db_path());
+    }
+
+    /**
+     * Calls the given DBManager method, storing the result for later retrieval
+     *
+     * \p dbManagerMethod method to execute (must return a CatalogObjectList)
+     * \p args arguments to supply to the method
+     */
+
+    void find_objects_by_name(const QString& name, const int limit = -1)
+    {
+        // N.B. One cannot have a function pointer to a function with
+        // default arguments, so all arguments must be supplied here
+        execute<const QString&, const int, const bool>(
+            &DBManager::find_objects_by_name,
+            name, limit, false);
+    }
+
+    void find_objects_by_name_exact(const QString &name)
+    {
+        execute<const QString&, const int, const bool>(
+            &DBManager::find_objects_by_name,
+            name, 1, true);
+    }
+
+    /**
+     * Returns the result of the previous call, or a nullptr if none exists
+     */
+    std::unique_ptr<CatalogObjectList> result()
+    {
+        QMutexLocker _{&m_resultMutex};
+        if (m_result.empty())
+        {
+            return std::unique_ptr<CatalogObjectList>();
+        }
+        std::unique_ptr<CatalogObjectList> result = std::move(m_result.front());
+        m_result.pop();
+        return result;
+    }
+
+private slots:
+
+    void cleanup()
+    {
+        Q_ASSERT(m_manager.use_count() == 1);
+        m_manager.reset();
+        m_thread->quit();
+    }
+
+    void emitReady()
+    {
+        emit threadReady();
+    }
+
+};
+
 } // namespace CatalogsDB

kstars/catalogsdb/sqlstatements.cpp

@@ -322,10 +322,19 @@ inline const QString dso_by_catalog(int catalog_id)
 // Nulls last because we load the objects in reverse :P
 
 const QString _dso_by_trixel = "SELECT %1 FROM master WHERE trixel = "
-                               ":trixel ORDER BY %2, major_axis ASC";
+                               ":trixel ORDER BY %2";
 
 const QString dso_by_trixel = QString(_dso_by_trixel).arg(object_fields).arg(mag_desc);
 
+const QString _dso_by_trixel_null_mag = "SELECT %1 FROM master WHERE trixel = "
+                                        ":trixel AND magnitude IS NULL";
+const QString dso_by_trixel_null_mag = QString(_dso_by_trixel_null_mag).arg(object_fields);
+
+const QString _dso_by_trixel_no_nulls = "SELECT %1 FROM master WHERE trixel = "
+                                        ":trixel AND magnitude IS NOT NULL ORDER"
+                                        " BY magnitude DESC";
+const QString dso_by_trixel_no_nulls = QString(_dso_by_trixel_no_nulls).arg(object_fields);
+
 const QString _dso_by_oid = "SELECT %1 FROM master WHERE oid = :id LIMIT 1";
 
 const QString dso_by_oid = QString(_dso_by_oid).arg(object_fields);
@@ -338,29 +347,29 @@ inline const QString dso_by_oid_and_catalog(const int id)
 };
 
 const QString _dso_by_name =
-    "SELECT %1, instr(lower(name), lower(:name)) AS in_name, instr(lower(long_name), "
-    "lower(:name)) AS in_lname FROM master WHERE in_name "
+    "SELECT %1, name like \"%\" || :name || \"%\" AS in_name, long_name like "
+    "\"%\" || :name || \"%\" AS in_lname FROM master WHERE in_name "
     "OR in_lname "
     "ORDER BY name, long_name, "
     "%2 LIMIT :limit";
 
-const QString _dso_by_name_exact = "SELECT %1 FROM master WHERE lower(name) = lower(:name) LIMIT 1";
+const QString _dso_by_name_exact = "SELECT %1 FROM master WHERE name = :name LIMIT 1";
 
 const QString dso_by_name       = QString(_dso_by_name).arg(object_fields).arg(mag_asc);
 const QString dso_by_name_exact = QString(_dso_by_name_exact).arg(object_fields);
 
 inline const QString dso_by_name_and_catalog(const int id)
 {
-    return QString("SELECT %1 FROM cat_%2 WHERE instr(name, :name) "
-                   "OR instr(long_name, :name) OR instr(catalog_identifier, :name)"
+    return QString("SELECT %1 FROM cat_%2 WHERE name like \"%\" || :name || \"%\" "
+                   "OR long_name like \"%\" || :name || \"%\" OR catalog_identifier like \"%\" || :name || \"%\""
                    "ORDER BY %3 LIMIT :limit")
            .arg(object_fields)
            .arg(id)
            .arg(mag_asc);
 }
 
 const QString _dso_by_wildcard = "SELECT %1 FROM master WHERE name LIKE :wildcard LIMIT "
-                                 ":limit ODRER BY CAST(name AS INTEGER)";
+                                 ":limit ORDER BY CAST(name AS INTEGER)";
 
 inline const QString dso_by_wildcard()
 {