New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Synchronize PlotDock with 'Execute SQL' table. #1271

Merged
merged 2 commits into from Jan 15, 2018

Conversation

Projects
None yet
4 participants
@FabianInostroza
Copy link
Contributor

FabianInostroza commented Dec 16, 2017

Add the synchronization feature between plot dock and browse data table to the table shown in the SQL query.

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Dec 17, 2017

Thanks. 😄

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Dec 17, 2017

Great! This is absolutely needed for coherence to the Data Browser table. Thanks! I'll take a look at the code now.

@mgrojo
Copy link
Contributor

mgrojo left a comment

The use of the enum Tab should be changed before merging. The refactoring could done after that, but let's wait to what @MKleusberg says. Thanks for the pull request, Fabian!

// Are there even that many lines?
if(lastLine >= m_browseTableModel->totalRowCount())
return;
if(ui->mainTab->currentIndex() == 1) {

This comment has been minimized.

@mgrojo

mgrojo Dec 17, 2017

Contributor

Here we should use the enum Tabs.


ui->dataTable->selectionModel()->select(QItemSelection(topLeft, bottomRight), QItemSelectionModel::Select | QItemSelectionModel::Rows);
ui->dataTable->selectionModel()->select(QItemSelection(topLeft, bottomRight), QItemSelectionModel::Select | QItemSelectionModel::Rows);
}else if(ui->mainTab->currentIndex() == 3) {

This comment has been minimized.

@mgrojo

mgrojo Dec 17, 2017

Contributor

The same here, better use the enum Tabs.

@@ -694,6 +694,34 @@ void MainWindow::deleteRecord()
}
}

void MainWindow::selectCurrentTabTableLines(int firstLine, int lastLine)

This comment has been minimized.

@mgrojo

mgrojo Dec 17, 2017

Contributor

Part of the code in this function could be refactored with the old code by passing the table model and table widgets as parameters. This could be made after merging, though. It is not indispensable.

This comment has been minimized.

@FabianInostroza

FabianInostroza Dec 18, 2017

Author Contributor

I though doing that would be nice but the method selectTableLine was used in a lot of places so I left it as is.

This comment has been minimized.

@mgrojo

mgrojo Dec 18, 2017

Contributor

Ok, you have my approval 👍

@mgrojo mgrojo requested a review from MKleusberg Dec 17, 2017

@mgrojo

mgrojo approved these changes Dec 18, 2017

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Dec 22, 2017

Thanks, @FabianInostroza 😄 This is really helpful indeed!

There is only one minor problem I see with this: you assume the plot is always drawn from the current tab but that doesn't need to be true. For example you could draw a plot in the Browse Data tab, then change to the Execute SQL tab, and then select a plot point. It's then going to select a row from the wrong table. The same happens if there are multiple SQL tabs opened. I'm not sure how to fix that though. We probably need to store the table widget from which the data comes from.

@mgrojo Or do you have any other idea? If not, I would suggest to just merge this and refactor the plot dock to store the required information.

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Dec 22, 2017

I think the plot is cleared when you change the tab, so it might be impossible to come to that situation, unless reaching in other way that I cannot currently visualise.

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Dec 22, 2017

Is it? For me it seems to stay even when changing tabs 😕 Especially if there are multiple SQL tabs I don't think the plot would be cleared when changing between them.

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Dec 22, 2017

You're right. It is only cleared when the list of selected columns does not match with the new table or query. So the issue can actually happen.

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Dec 22, 2017

Ah that makes sense 😃

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Jan 15, 2018

I'll merge this now and then try to address the issue about the selection in the appropriate table widget. Thanks again @FabianInostroza for this contribution!

@mgrojo mgrojo merged commit dbbd268 into sqlitebrowser:master Jan 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Jan 15, 2018

Yep, I agree. I should have merged this earlier. Thanks @FabianInostroza and @mgrojo! 😃

mgrojo added a commit that referenced this pull request Jan 15, 2018

Synchronize PlotDock with 'Execute SQL' table. Continuation of PR #1271
Make use of signals to connect the selection in plot to the associated
table widget. Every time that the plot is updated from the Main Window
the table widget associated to the table or query is connected to the plot
and the previous widget is disconnected. This allows the selection of the
correct table widget.

Line selection methods moved to the Extended Table Widget to be used as
slots for this connection.

The destroyed signal is also connected for resetting the plot. This fixes
a crash that already existed before this PR, when closing a SQL tab while
the plot is still associated to the table results model.
@FabianInostroza

This comment has been minimized.

Copy link
Contributor Author

FabianInostroza commented Jan 17, 2018

Thank you!

@FabianInostroza FabianInostroza deleted the FabianInostroza:plot_table_sync branch Jan 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment