detach additional database connection #2241

horst-p-w-neubauer · 2020-05-12T00:01:50Z

detach additional database connection

added popupAction to DbTreeWidget
added popupAction to treeSchemaDock
added detach() to sqlitedb
implements Detach Database #2239

src/sqlitedb.cpp

horst-p-w-neubauer · 2020-05-12T17:11:41Z

Be aware, I've changed attach behavior too in my last commit, please consider on review!

horst-p-w-neubauer · 2020-05-12T19:26:18Z

Need help! What did I break?

horst-p-w-neubauer · 2020-05-13T17:40:21Z

learned from @justinclift:

Ahhh, seems to just be a failure in the TravisCI infrastructure rather than a problem with the PR. Clicking on the "Details" link on the line in the PR where it says it failed leads to this page: https://travis-ci.org/github/sqlitebrowser/sqlitebrowser/jobs/686209893
Scrolling through the content, it seems to have built ok, but failed near the end of the process where it uploads the AppImage for people. The failure message was:

The job exceeded the maximum time limit for jobs, and has been terminated.

That kind of thing happens occasionally. It's not really something we can control.

mgrojo · 2020-05-13T19:40:01Z

The same happened to my last pull request. I restarted the jobs and the second time they finished on time.

horst-p-w-neubauer · 2020-05-14T08:35:55Z

I need your advice, please.
I'm no c++ developer (not yet), so I have to be careful what I change and would be happy to get your opinions on some code change suggestions.

In this PR I allready suggest to add to MainWindow.h (and I’m not happy with my naming):
QString getDbStructureObjectTypeFromTreeViewSelected(QTreeView* treeView);

Meanwhile I realised, it's a matter of OOP. What I would like to have is a a set of protected read only methods to get all the usefull information from both of the structure-Widgets .
e.g.

    QString dbTreeSelectedObjectType(QTreeView* dbTree = ui->dbTreeWidget);
    QString dbTreeSelectedName(QTreeView* dbTree);
    QString dbTreeSelectedDiplayName(QTreeView* dbTree);
    QString dbTreeSelectedSchema(QTreeView* dbTree);
    sqlb::ObjectIdentifier dbTreeSelected(QTreeView* dbTree);

Please, give me feedback if it’s ok when I go this way.

scottfurry · 2020-05-14T08:50:21Z

@horst-p-w-neubauer - Have a look at the QTreeView API page. Extracting useful information would have to be done from a QTreeViewWidgetItem. A numerical index (or get that index via a search for a string name) would be required to get a View Item from Tree Model. The QTreeView description has links to overviews of Qt's Model/View Architecture. I'd encourage you to read through these pages first.

horst-p-w-neubauer · 2020-05-14T08:58:28Z

@scottfurry , I struggle with all those longliners:

    sqlb::ObjectIdentifier name(ui->dbTreeWidget->model()->data(ui->dbTreeWidget->currentIndex().sibling(ui->dbTreeWidget->currentIndex().row(), DbStructureModel::ColumnSchema), Qt::EditRole).toString().toStdString(),
                                ui->dbTreeWidget->model()->data(ui->dbTreeWidget->currentIndex().sibling(ui->dbTreeWidget->currentIndex().row(), DbStructureModel::ColumnName), Qt::EditRole).toString().toStdString());

In my opinion MainWindow should encapsulate the access details to getters. I've come upon this because there is indifferent use of the role attribute in the actual code and I've had some time to figure out they all do the same.

scottfurry · 2020-05-14T09:08:49Z

And that's part of the reason I'm not big fan of Qt-Creator, but that is what is used for the project. A type alias may somehow alleviate the situation, but no promises.

horst-p-w-neubauer · 2020-05-14T09:22:54Z

A type alias may somehow alleviate the situation, but no promises.

sorry, don't understand that

chrisjlocke · 2020-05-14T09:58:32Z

I come from the world of VB .Net, but in that, you can create your own class, which does all the gruntwork of pulling from various 'longer' sources. A wrapper, if you like. So still OOP(sie daisy) but just 'cleaner'. The world of Qt might be totally different though...
So you could have a class of 'UI' which pulls together variables and values from the UI as the user sees it, but then a 'DB' class which holds the properties of the database(s).

horst-p-w-neubauer · 2020-05-14T19:20:01Z

A yes - a facade class came into my mind too - I was just not brave enough to suggest. But if nobody mind I'd be fine with writing a facade to the treeViews holding DbStructureModels.

justinclift · 2020-05-14T20:01:05Z

I struggle with all those longliners

Yeah, similar here when I was doing C++ on DB4S too.

Personally, I tend to break things up into discrete units, as it's easier to mentally parse. It also means breakpoints can be set on individual lines when needing to debug / step through code and figure out what's going wrong. 😄

eg, something like this:

someModel := ui->dbTreeWidget->model();
currIndex := ui->dbTreeWidget->currentIndex();
schemaObj := someModel->data(currIndex.sibling(currIndex.row(), DbStructureModel::ColumnSchema), Qt::EditRole)
schemaStr := schemaObj.toString().toStdString();
nameObj := someModel->data(currIndex.sibling(currIndex.row(), DbStructureModel::ColumnName), Qt::EditRole);
nameStr :=  nameObj.toString().toStdString();
sqlb::ObjectIdentifier name(schemaStr,nameStr);

Compilers generate the same code anyway, so it's not "more efficient" to have everything in long lines. 😄

horst-p-w-neubauer · 2020-05-14T21:13:03Z

But copy down a block of lines each time you just want to know which object type the actual selected dbStructure node does hold is boring - and as I've seen - not free of errors. If we hide all of this behind a tiny facade the code will look obvious and you don't get disturbed by all this utility stuff.

    // on creation:
    DbStructureTreeViewFacade dbMainSelected = DbStructureTreeViewFacet(ui->dbTreeWidget);
  ...
   // using later
    schema = dbMainSelected.schema();
    name = dbMainSelected.name();
    displayName = dbMainSelected.displayName();

justinclift · 2020-05-15T08:14:43Z

Sounds like an improvement. 😄

@mgrojo @MKleusberg ?

horst-p-w-neubauer · 2020-05-15T17:08:25Z

Here's a bit of UML to show what I mean.

DbStructureQItemViewFacade provides a simple read only interface to a QAbstractItemView (e.g. QTreeView) that holds a model() of type DBStructureModel.

It is designed to simplify the access to the actual selected node.

The Class follows the facade design pattern.
But it doesn't control the lifecycle of the itemView it is connected to.

Not in the picture: I forgot:

sqlb::ObjectIdentifier DbStructureQItemViewFacade::object(); QString DbStructureQItemViewFacade::sql()

horst-p-w-neubauer · 2020-05-15T23:48:32Z

to solve #2242 "add path info to attached databases" I consider two different ways.
actually ColumnName for attached databases holds text and ColumnSchema is empty
To detach a database I've used dbSelected.name(). No other code seams to use the database node at least not in MainWindow.cpp - but maybe in others.

solution
set ColumnName for attached databases to <schema> '['<filepath>']'
set ColumnSchema to and use dbSelected.schema() instead of dbSelected.name() for detach of databases.
solution
add ColumnUrl to DbStrutcureModel::Columns, fill it with <filepath> and let DbStrutcureModel::data() decorate DisplayName() with <name> '['<filepath>']'
but even with this solution we should set ColumnSchema to <schema>.

Which way should we go?
I'd prefer the 2.

Trying to understand the code I came up with this understanding of Colums (including a suggestion for 2. solution). Did I mix something up?

    /*!
     * \enum DbStructureModel::Columns
     *
     *  The Columns of a DbStructureModel Item.
     *
     * \value ColumnName Name of the datbase structure object (e.g. tablename, fieldname, indexname, databasename (alias, not filename!)
     * \value ColumnObjectType Type of the datbase structure object as QString ("table", "view", "index", "trigger", "field")
     * \value ColumnDataType Datatype of the database structure object itself. todo: investigate and document fully
     * \value ColumnSQL Original sql statement of the database structure object.
     * \value ColumnSchema Name of the datbase to which the database structure object belongs (alias, not filename!).
     * \value ColumnUrl Filename or url of the datbase of the object that represents the database.
     */

mgrojo · 2020-05-16T15:38:01Z

And what about setting the file information to 'Name' column's tooltip? Visually I'd prefer it to be set in the Schema column, but that would be inappropriate with the column naming. Adding another column would make it to be either too far to the right (when appended) or displace other columns too far to the right (when inserted).

horst-p-w-neubauer · 2020-05-18T17:45:04Z

Thanks to @scottfurry, I've realised we shouldn't allow to detach from the temp database.

MKleusberg · 2020-05-19T09:59:48Z

Looks like a good start to me 👍 One minor thing I just noticed is that the new files aren't yet added to the CMakeLists.txt file. Another thing is that this PR is doing much more than just adding a detach database feature by now. I don't know how familiar you are with Git but if you are, can you rewrite the commits for easier reviewing at some point?

horst-p-w-neubauer · 2020-05-23T14:53:33Z

@mgrojo, I wonder about Your hint:

And what about setting the file information to 'Name' column's tooltip? Visually I'd prefer it to be set in the Schema column, but that would be inappropriate with the column naming. Adding another column would make it to be either too far to the right (when appended) or displace other columns too far to the right (when inserted).

I thought ColumnXy of the model is only a structured datastore of the model. How it is shown on the front is a different thing. I'd not planed to bring ColumnUrl onto the tree as another column. I thougt to pick the data from this column and use it for columnName when it is asked in its DisplayRole. If this is not the right use of the columns in here, please tell me and I'm happy to change it.

horst-p-w-neubauer · 2020-05-23T15:06:21Z

@MKleusberg

Another thing is that this PR is doing much more than just adding a detach database feature by now. I don't know how familiar you are with Git but if you are, can you rewrite the commits for easier reviewing at some point?

which of them do you mean?
I'm fine to reset the renaming from database to schema, but taking off the QObjectFacade would be a heavy thing.

horst-p-w-neubauer · 2020-05-23T15:30:58Z

@MKleusberg

I just noticed is that the new files aren't yet added to the CMakeLists.txt file

I'm not sure where to put this file. What is the difference between "set(SQLB_HDR" and "set(SQLB_MOC_HDR" in CMakeLists.txt file?

scottfurry

@MKleusberg

I just noticed is that the new files aren't yet added to the CMakeLists.txt file

I'm not sure where to put this file. What is the difference between "set(SQLB_HDR" and "set(SQLB_MOC_HDR" in CMakeLists.txt file?

For educational purposes, a MOC_XXXX file is apart of Qt5 build sequence. The Meta-Object Compiler generates *.h and *.cxx or *.cpp files from the XML-like UI files created by Qt Creator and such. Without them, the program has no hooks in the QT5 ecosystem.

src/MainWindow.ui

scottfurry · 2020-05-19T08:28:38Z

src/sqltextedit.cpp

    registerImage(SqlUiLexer::ApiCompleterIconIdTable, QImage(":/icons/table"));
    registerImage(SqlUiLexer::ApiCompleterIconIdColumn, QImage(":/icons/field"));
-    registerImage(SqlUiLexer::ApiCompleterIconIdSchema, QImage(":/icons/database"));
+    registerImage(SqlUiLexer::ApiCompleterIconIdSchema, QImage(":/icons/schema")); // ?hn rename to "schema"


The reference to this file resource has to exist in the directory tree.
In this case, you need the file [proj root]/src/icons/database.png

this is a relict from Revert "rename "database" to "schema" internally" their shouldn't be a issue anymore - right?

scottfurry · 2020-05-19T08:29:18Z

src/VacuumDialog.cpp

        QTreeWidgetItem* item = new QTreeWidgetItem(ui->treeDatabases);
        item->setText(0, QString::fromStdString(it.first));
-        item->setIcon(0, QIcon(QString(":icons/database")));
+        item->setIcon(0, QIcon(QString(":icons/schema"))); // ?hn rename to "schema"


Same as comment at end - resource needs to exist in tree.

this is a relict from Revert "rename "database" to "schema" internally" their shouldn't be a issue anymore - right?

scottfurry · 2020-05-19T08:31:34Z

src/DbStructureModel.cpp

        // for schemata != "main"). For the normal structure branch of the tree we don't want to add the schema name because it's already obvious from
        // the position of the item in the tree.
+        if(index.column() == ColumnName && data(index.sibling(index.row(), ColumnObjectType), Qt::EditRole).toString() =="schema" ){
+            return item->text(index.column()) + " []"; // ?hn - remainder to add fileinfo later


This may be "problematic". You may want to read up on QAbstractItemModel to better understand why this column name is not a simple matter.

The QT view/item model is quirky but is fundamental to how qt manages data to/from a data source.

Scott. Sorry. You're commenting outdated changes.
take a look on: Revert "rename "database" to "schema" internally", please.

Yeah. GitHub is showing a little "[Outdated]" tag next to the source code snippet. I have no idea why it's showing people the old code, rather than the latest. Doesn't seem useful. 😦

mgrojo · 2020-05-24T20:13:21Z

@mgrojo, I wonder about Your hint:

And what about setting the file information to 'Name' column's tooltip? Visually I'd prefer it to be set in the Schema column, but that would be inappropriate with the column naming. Adding another column would make it to be either too far to the right (when appended) or displace other columns too far to the right (when inserted).

I thought ColumnXy of the model is only a structured datastore of the model. How it is shown on the front is a different thing. I'd not planed to bring ColumnUrl onto the tree as another column. I thougt to pick the data from this column and use it for columnName when it is asked in its DisplayRole. If this is not the right use of the columns in here, please tell me and I'm happy to change it.

Sorry, I thought you were suggesting to bring that as a new column to the view too. I still prefer to have the file path as a tooltip. I think it's not something you need to have permanently visible and it'd make the column too wide and too "noisy".

horst-p-w-neubauer · 2020-05-25T08:28:06Z

Sorry, I thought you were suggesting to bring that as a new column to the view too. I still prefer to have the file path as a tooltip. I think it's not something you need to have permanently visible and it'd make the column too wide and too "noisy".

I've allready detached #2242 form this PR but you can see my UI proposal there.

MKleusberg

Here's my full review 😄 These are all minor points which should be easy to fix.

MKleusberg · 2020-06-03T07:48:11Z

src/MainWindow.cpp

-        QString type = ui->dbTreeWidget->model()->data(ui->dbTreeWidget->currentIndex().sibling(ui->dbTreeWidget->currentIndex().row(), DbStructureModel::ColumnObjectType)).toString();
-        if(type == "table" || type == "view")
+        QString type = dbSelected->objectType();
+        if(type  == "table" || type == "view")


Extra whitespace added here 😉

MKleusberg · 2020-06-03T07:51:29Z

src/MainWindow.cpp

-        ui->editDeleteObjectAction->setIcon(QIcon(QString(":icons/%1_delete").arg(type)));
-        ui->editModifyObjectAction->setIcon(QIcon(QString(":icons/%1_modify").arg(type)));
-    }
+        {


Extra indentation added for the following lines.

MKleusberg · 2020-06-03T07:54:07Z

src/MainWindow.cpp

+
+void MainWindow::fileDetachTreeSchemaDock()
+{
+    fileDetachTreeViewSelected(ui->treeSchemaDock);


You should be able to use a single function here (e.g. just fileDetachTreeViewSelected) instead of two (fileDetachDbTree and fileDetachTreeSchemaDock) by using the sender() function and casting the result into a QTreeView* pointer. I'll add an example below.

MKleusberg · 2020-06-03T07:56:08Z

src/MainWindow.cpp

+}
+
+void MainWindow::fileDetachTreeViewSelected(QTreeView* treeView)
+{


Here you remove the parameter, then add this at the beginning of the function to save the two extra functions above:

QTreeView* treeView = qobject_cast<QTreeView*>(sender());

MKleusberg · 2020-06-03T07:58:22Z

src/MainWindow.h


 #include <memory>
 #include <QMainWindow>
+#include <QTreeView>


No need to include QTreeView here. We try to keep the number of includes as low as possible to reduce the compile time. You can just add class QTreeView; a few lines below.

MKleusberg · 2020-06-03T08:04:28Z

src/dbstructureqitemviewfacade.h

+{
+    Q_OBJECT
+public:
+    explicit DbStructureQItemViewFacade(QAbstractItemView *aitemView = new QTreeView());


I don't understand why you are specifying a default value here. I thought there is no point creating a DbStructureQItemViewFacade object without a tree view object. But if there is no point in doing so, this should be made clear to the user of the class by not even allowing it 😄

MKleusberg · 2020-06-03T08:07:15Z

src/dbstructureqitemviewfacade.h

+     *
+     * @return displayName of selectedItem
+     */
+    QString displayName();


All these functions (displayName, name, objectType, schema, sql, object) should be marked as const.

MKleusberg · 2020-06-03T08:09:10Z

src/dbstructureqitemviewfacade.h

+    sqlb::ObjectIdentifier object();
+
+private:
+    QAbstractItemView *m_itemView;


This could be a reference instead of a pointer I guess.

MKleusberg · 2020-06-03T08:10:48Z

src/sqlitedb.cpp

+//    });
+
+//    if(ok == false)
+//        return false;


All this code should be removed instead of being put in a comment.

MKleusberg · 2020-06-03T08:11:28Z

src/MainWindow.cpp

    popupSchemaDockMenu = new QMenu(this);
    popupSchemaDockMenu->addAction(ui->actionPopupSchemaDockBrowseTable);
+    popupSchemaDockMenu->addAction(ui->actionPopupSchemaDockDetachDatabase);
+    // popupSchemaDockMenu->addAction(ui->fileDetachAction);


Old comment here which looks like it can be removed.

MKleusberg · 2020-08-21T14:06:12Z

I have just pulled out all the changes which aren't directly related to the detach feature (the facade, the removed double attach check, and the removed dirty check for the attach action) and merged them into master. The remaining changes are rebased and squashed into a single commit. So this should be pretty straightforward to review now 😄

See issue #2239

MKleusberg · 2020-08-22T13:02:22Z

I have just fixed another issue which I probably introduced myself when rebasing this branch 😉 Looks good now. Thanks, @horst-p-w-neubauer! 👍

horst-p-w-neubauer requested review from MKleusberg and mgrojo and removed request for MKleusberg May 12, 2020 00:01

chrisjlocke reviewed May 12, 2020

View reviewed changes

src/sqlitedb.cpp Outdated Show resolved Hide resolved

horst-p-w-neubauer requested a review from chrisjlocke May 12, 2020 17:11

horst-p-w-neubauer marked this pull request as draft May 14, 2020 08:46

horst-p-w-neubauer self-assigned this May 14, 2020

This was linked to issues May 14, 2020

Detach Database #2239

Closed

add path info to attached databases #2242

Open

horst-p-w-neubauer linked an issue May 16, 2020 that may be closed by this pull request

rename internally ColumnObjectType values „database“ to „schema“ #2247

Closed

horst-p-w-neubauer mentioned this pull request May 18, 2020

enable "Attach Database" on In-Memory main Database #2255

Closed

horst-p-w-neubauer linked an issue May 18, 2020 that may be closed by this pull request

enable "Attach Database" on In-Memory main Database #2255

Closed

This was unlinked from issues May 23, 2020

rename internally ColumnObjectType values „database“ to „schema“ #2247

Closed

add path info to attached databases #2242

Open

horst-p-w-neubauer marked this pull request as ready for review May 23, 2020 16:31

scottfurry reviewed May 24, 2020

View reviewed changes

MKleusberg reviewed Jun 3, 2020

View reviewed changes

MKleusberg force-pushed the detach-database branch from 28b2a15 to 32c7c5e Compare August 21, 2020 14:02

detach additional database connection

8c79177

See issue #2239

MKleusberg force-pushed the detach-database branch from 32c7c5e to 8c79177 Compare August 22, 2020 13:00

MKleusberg added the enhancement Feature requests. label Aug 22, 2020

MKleusberg added this to the 3.13.0 - Future release milestone Aug 22, 2020

MKleusberg merged commit 29635e9 into master Aug 22, 2020

MKleusberg deleted the detach-database branch August 22, 2020 13:02

Uh oh!

detach additional database connection #2241

detach additional database connection #2241

Uh oh!

Conversation

horst-p-w-neubauer commented May 12, 2020

Uh oh!

Uh oh!

horst-p-w-neubauer commented May 12, 2020

Uh oh!

horst-p-w-neubauer commented May 12, 2020

Uh oh!

horst-p-w-neubauer commented May 13, 2020

Uh oh!

mgrojo commented May 13, 2020 via email

Uh oh!

horst-p-w-neubauer commented May 14, 2020

Uh oh!

scottfurry commented May 14, 2020

Uh oh!

horst-p-w-neubauer commented May 14, 2020

Uh oh!

scottfurry commented May 14, 2020

Uh oh!

horst-p-w-neubauer commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisjlocke commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

horst-p-w-neubauer commented May 14, 2020

Uh oh!

justinclift commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

horst-p-w-neubauer commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinclift commented May 15, 2020

Uh oh!

horst-p-w-neubauer commented May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

horst-p-w-neubauer commented May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgrojo commented May 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

horst-p-w-neubauer commented May 18, 2020

Uh oh!

MKleusberg commented May 19, 2020

Uh oh!

horst-p-w-neubauer commented May 23, 2020

Uh oh!

horst-p-w-neubauer commented May 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

horst-p-w-neubauer commented May 23, 2020

Uh oh!

scottfurry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

horst-p-w-neubauer May 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

horst-p-w-neubauer May 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

horst-p-w-neubauer May 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

horst-p-w-neubauer commented May 14, 2020 •

edited

Loading

chrisjlocke commented May 14, 2020 •

edited

Loading

justinclift commented May 14, 2020 •

edited

Loading

horst-p-w-neubauer commented May 14, 2020 •

edited

Loading

horst-p-w-neubauer commented May 15, 2020 •

edited

Loading

horst-p-w-neubauer commented May 15, 2020 •

edited

Loading

mgrojo commented May 16, 2020 •

edited

Loading

horst-p-w-neubauer commented May 23, 2020 •

edited

Loading

horst-p-w-neubauer May 24, 2020 •

edited

Loading

horst-p-w-neubauer May 24, 2020 •

edited

Loading

horst-p-w-neubauer May 24, 2020 •

edited

Loading

justinclift May 24, 2020 •

edited

Loading