Skip to content

Conversation

@mgrojo
Copy link
Member

@mgrojo mgrojo commented Aug 12, 2020

since the data wouldn't be available yet.

See issue #2286

since the data wouldn't be available yet.

See issue #2286
@mgrojo
Copy link
Member Author

mgrojo commented Aug 12, 2020

I open this pull request, because there could be a better solution, which might imply refactoring in the signals emitted by the SqliteTableModel class.

@mgrojo mgrojo requested a review from MKleusberg August 12, 2020 22:37
@justinclift
Copy link
Member

Cool. This seems like it would have been super annoying to track down. 😄

@MKleusberg
Copy link
Member

Thanks, @mgrojo! As Justin says, this must have been awful to track down. I think this solution is the best for now. We could think about moving the if statement into SqliteTableModel::handleFinishedFetch() and don't emit the finishedFetch signal at all when only the row count is available and make use of the finishedRowCount signal for the Execute SQL code. But honestly I don't know if any other part of the code relies on these signals as they work now. So especially for a point release this is definitely better 😄

@MKleusberg MKleusberg closed this Aug 15, 2020
@MKleusberg MKleusberg reopened this Aug 15, 2020
@MKleusberg MKleusberg merged commit 20e9498 into master Aug 15, 2020
@MKleusberg MKleusberg deleted the issue_2286 branch August 15, 2020 15:06
@justinclift
Copy link
Member

@mgrojo @MKleusberg Should this be merged into 3.12.1 as well?

@MKleusberg
Copy link
Member

Yes, good point! This bug was introduced in 1f9101a so it is present in 3.12.0.

@MKleusberg MKleusberg added the bug Confirmed bugs or reports that are very likely to be bugs. label Aug 15, 2020
@MKleusberg MKleusberg added this to the 3.12.1 milestone Aug 15, 2020
@mgrojo
Copy link
Member Author

mgrojo commented Aug 16, 2020

Thanks, @mgrojo! As Justin says, this must have been awful to track down. I think this solution is the best for now. We could think about moving the if statement into SqliteTableModel::handleFinishedFetch() and don't emit the finishedFetch signal at all when only the row count is available and make use of the finishedRowCount signal for the Execute SQL code. But honestly I don't know if any other part of the code relies on these signals as they work now. So especially for a point release this is definitely better 😄

Thanks. I tried some changes in the connection or emission of signals (not sure about that specific case though), and they either gave other problems, or did not solve the issue). We should take a look again when we have to change something related to that code.

@MKleusberg
Copy link
Member

We should take a look again when we have to change something related to that code.

Yeah, I agree 👍

@mgrojo mgrojo restored the issue_2286 branch October 14, 2023 21:27
@mgrojo mgrojo deleted the issue_2286 branch October 14, 2023 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Confirmed bugs or reports that are very likely to be bugs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants