refactoring / code quality improvements

[mispp]

some ideas:

• take out QSqlQuery in it's own class called Query which should implement functions

  • QList<QStandardItem*> nextResultsChunk(int chunkSize)
  • void stopQuery()
  • bool reconnectDatabase()
  • executeQuery(QString query)

• not sure how to handle this QAction thing for the QToolButton

  • currently there is a slot in ConnnectionManager that takes .data() from sender() -> i have a bad feeling about this implementation and no idea how to make it better

• take out the loop from refreshConnectionActions() from MainWindow and move it to connection manager. i guess function has to stay here to first clear old actions before loading new ones

[mispp]

@asw-dev since you're much better than me at this stuff, does this makes sense to you? have some hints what's good/bad with this and maybe some ideas what else should i do?

i'd like to avoid having to clean it up later when it further grows, but as said, not a c++ developer and very little experience in this kind of design.

[asw-dev]

I wouldn't call myself an expert at Qt but here is my 2 cents.

QSqlQuery -> Query: seems fine, it's very close to a refactor I was thinking of.

QAction & QToolButton: using data to hold an id is fine, but ideally you want to avoid ConnnectionManager having to know anything about the caller. Normally you could have the signal also pass data, but in this case that is probably not an option. Instead you can create a new slot in MainWindow (the current owner of QToolButton, but it could also be subclassed if you wanted to) that then calls m_connnectionManager.openConnection() using data() to get the arg to call with.

refreshConnectionActions: The original idea of a "refresh" was to be a quick way to do all the things. A more clean (but verbose) way of doing this is to add all the individual state changes of ConnnectionManager as signals.

• connectionAdded(Connection connection)
• connectionUpdated(Connection oldValue, Connection newValue)
• connectionRemoved(QString connectionId)

Then use emit to raise the signals when you change state, and connect them as need (and emit any additional signals as state is changed in those objects, etc.).

Another refactor that comes to mind would be to take the ConnectionDialog and extract a ConnectionWidget that can be reused for both ConnectionDialog & ConnectionManagerDialog.

[mispp]

I wouldn't call myself an expert at Qt but here is my 2 cents.

but you do java development, so OOP is something you know much better than me and i'm trying to learn what good design looks like

QSqlQuery -> Query: seems fine, it's very close to a refactor I was thinking of.

what do you think about something like this: https://github.com/mispp/goat/tree/refactoring

Made a custom class Query with member QSqlQuery. Didn't want to inherit QSqlQuery because i wanted that Query handles cloning of connection upon every execution. So basically Query::execute() should handle cloning of connection and internally setup results and threading.

It compiles and works, but not sure if this is something decent.

[mispp]

Some fixes were needed in the new branch, so now it works better.

Problem: since every tab (actually Query) has it's own connection, during exit this needs to be released in a way that warning is not shown. Currently, in MainWindow exit function, there is a loop that closes all open connections and then removes them.

[mispp]

Example warning:

QSqlDatabasePrivate::removeDatabase: connection 'CLONED_{f360678c-5fb5-4c9d-a19d-eb176985dbbf}' is still in use, all queries will cease to work.

[asw-dev]

From what I can see it looks reasonable.

I saw that switchDatabase() & reconnectDatabase() are closing the db before m_query is deconstructed so it will cause warnings when removeDatabase() is called. Might require m_query being a pointer so you delete it when you want.

The reuse of Query across threads would cause issues if you ever had two query threads running. It would be unlikely but if you failed to cancel without an error it would re-enable the query button for a second thread to work on the same Query reference. Not sure if it's worth considering at this point.

[mispp]

Thanks for the review.

Might require m_query being a pointer so you delete it when you want.

Suspected the same thing, but didn't want to touch it. Oh well.

The reuse of Query across threads would cause issues if you ever had two query threads running. It would be unlikely but if you failed to cancel without an error it would re-enable the query button for a second thread to work on the same Query reference. Not sure if it's worth considering at this point.

Yes, this is an issue and it must not happen. Says in the docu that forward-only queries should not have any other queries ran on the same connection. This i know is a problem since ctrl-enter doesn't get disabled, so it can be triggered twice. Going to do the following and see if it works:

• rename Query to QueryManager
• QueryManager will be stack allocated within a QueryTab
• QueryManager will create new QSqlQuery object on heap and manage that object
• QueryManager will have QThread member and QSqlQuery will be moved to the new thread
• QueryManager should have a switchConnection(QSqlDatabase clonedDatabase) method (dependency injection?)
• QueryManager will also have a QMutex or some similar locking mechanism so it cannot be ran twice until it is finished (ctrl-enter prevention).

This way, each QueryTab will have its own thread for running QSqlQuery.
Not sure if 'scroll to the end of QTableView' event could cause issues due to possibility of triggering it twice...

[mispp]

Further down the road, each tab should have a Queue for data export. Each export should have its own thread.

[mispp]

https://stackoverflow.com/questions/48827940/qsqldatabaseprivateremovedatabase-connection-myconnectionname-is-still-in-u

{
    QSqlDatabase db = QSqlDatabase::database("sales");
    QSqlQuery query("SELECT NAME, DOB FROM EMPLOYEES", db);
}
// Both "db" and "query" are destroyed because they are out of scope
QSqlDatabase::removeDatabase("sales"); // correct

Instead of object, i guess only connectionId has to be moved around so this error is avoided.

[mispp]

What i had in mind with QueryTab is done. It should be better now because it's decoupled, but still not the best.

Not using QThread as mentioned before, but QtConcurrent::run

[mispp]

Instead you can create a new slot in MainWindow (the current owner of QToolButton, but it could also be subclassed if you wanted to) that then calls m_connnectionManager.openConnection() using data() to get the arg to call with.

Great advice. QMenu has a triggered(QAction*) signal, so i hooked that one to call openConnection which picks up data from QAction.

[mispp]

finally managed to make QThread setup work here https://github.com/mispp/goat/tree/refactoring2

@asw-dev mind if you take a look and give me some feedback?

it looks quite clean for me, so now that i figured it out, the next two things need to be done in the same way:

• query stopper
• exporting export results support #55
• button management depending on query status

after both are done, should be merged into master.

testing is not fully done, so i'm not sure how it works with multiple connections, disconnection, closing tab while query is running etc.

Several issues are also present:

• open connections are not done yet in a proper way
• static method in connection manager doesnt work for cloning, so i c/p it into query class.

[mispp]

this now works (branch refactoring2), including querystopper and export.

what i plan to do further:

• set error messages to be as they are now in master
• query and queryexporter are similar, i'll try to set up an abstract class that would unify what can be unified.
  • maybe split finished signals into two, one for results, one for (error)message
• not sure if Csv could be generalized somehow in Exporter class and this class could then support CSV, XML and other formats through inheritance
• query needs to have better statuses and buttons have to be linked to statuses (have to look here and here). for example:
  • run button and actions have to be enabled only when there are active connections in a tab
  • export buttons need to be enabled only if there is a select statement in m_query
  • ...
• connection manager should have static method for cloning (cloneDatabase? cloneConnection?), the one i did doesnt work. due to nature of the application, we need to clone connections. naming should maybe be changed where Connection is what would be saved on drive (in connections.ini) and Database should be and instance, when connection gets open with an id. once opened this connectionId would be renamed (considered) as databaseId? if it makes sense
• make sure that closing tab stops everything and gives warning before it actually does it.

[asw-dev]

Not sure if I was able to follow all the code changes, but here is what I saw. Looks like you are already aware of some of it.

ConnectionManager::cloneConnection(conn, id) duplicates most of ConnectionManager::openConnection(conn) (and again in Query, QueryExporter, and QueryStopper) I'd probably change ConnectionManager::cloneConnection() to createQSqlDatabase() and use it everywhere you need a new/cloned QSqlDatabase. I have the db.connectionId be the pair of connection.id & new uuid joined by ":" in my branch for any "cloned" db.

in Query::executeSql() the line emit queryExecutionFinished(m_query.isSelect(), m_query.record(), message); is sending record from an active query to another thread (gui thread). I'm not versed enough in Qt to know if this will be an issue.

~QueryTab() should be calling delete (deleteLater? or setting parent in constructor?) on m_query and m_queryThread. Or they could be non-pointers.

m_queryThread->quit(); will stop the thread in an unclean state. If a tab is being closed (or the application for that matter) while a query is running it might be better to try to cancel it (or warning the user and provide a cancel-close, kill-query, force-close option).

QueryExporter::executeSqlAndExport() has a local-scoped mutex, no other thread has visiblity of it, so it is not protecting anything and can be removed.

[mispp]

I have the db.connectionId be the pair of connection.id & new uuid joined by ":" in my branch for any "cloned" db.

i'll take a look. you usually use whatever is in Connection object to pass to openConnection or addConnection, so i'd need to update it somehow before passing.

is sending record from an active query to another thread

let's keep it like this until it causes problems.

QueryTab() should be calling delete (deleteLater? or setting parent in constructor?) on m_query and m_queryThread. Or they could be non-pointers.

there is a connect signal which says on QThread::finished call QThread::deleteLater. same is done also for Query, so as far as i understand, this should initiate delete for both objects on Tab closing.

m_queryThread->quit(); will stop the thread in an unclean state

will do this. had this in mind already.

has a local-scoped mutex

will convert this then to signal/slot instead of direct call.

thanks for the review!

[mispp]

is sending record from an active query to another thread

this probably can be changed to QList since it has type information.