Skip to content
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

FabianInostroza
Copy link
Contributor

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

@justinclift justinclift added the enhancement Feature requests. label Dec 17, 2017
@justinclift
Copy link
Member

justinclift commented Dec 17, 2017

Thanks. 😄

@mgrojo
Copy link
Member

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.

Copy link
Member

@mgrojo mgrojo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same here, better use the enum Tabs.

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

void MainWindow::selectCurrentTabTableLines(int firstLine, int lastLine)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, you have my approval 👍

@MKleusberg
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

Ah that makes sense 😃

@mgrojo
Copy link
Member

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
@MKleusberg
Copy link
Member

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
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
Copy link
Contributor Author

Thank you!

@FabianInostroza FabianInostroza deleted the plot_table_sync branch January 17, 2018 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants