Add feedback to executeSql #38870
Add feedback to executeSql #38870
Conversation
Fixes qgis#38092 by adding an optional QgsFeedback argument to the executeSql method and by implementing the PQCancel method in the PG provider internals. While the cancellation works well for all supported provider while fetching results in the loop, the cancellation of a running query is now implemented for the postgres provider connection only because the GPKG and GDAL both rely on GDALDatasetExecuteSQL which cannot be interrupted. This PR also introduce a few optimizations in the PG DB-Manager code that should probably fix also other "slowness" issues that were reported after 3.x during PG query execution. A small UX change in th SQL dialog makes it evident to the user that a cancellation request has been sent to the backend: the button text is changed to "Cancellation requested, please wait..." so that for provider connections that are not able to interrupt the running query and must wait for the fetching loop to exit from the exeuteSql call the user knows that something is happening and that a cancellation request has been successfully sent.
elpaso
added
DB Manager
| @@ -381,6 +381,7 @@ def updateUiWhileSqlExecution(self, status): | |||
|
|
|||
| def executeSqlCanceled(self): | |||
| self.btnCancel.setEnabled(False) | |||
| self.btnCancel.setText(self.tr("Canceling, please wait ...")) | |||
There was a problem hiding this comment.
What about just "Canceling" and leave the "please wait" bit for a tooltip? Otherwise the button will grow very large for this lengthy string
There was a problem hiding this comment.
I won't put up an argument but here are two reasons and a half for leaving as it is:
0.5: after OSGeo/gdal#2953 the cancellation will happen so fast for GPKG/SL that most of the times nobody will notice the button change, for PG the cancellation will already be immediate after this PR
- there is plenty of horizontal space in that part of the dialog, at the left of the button there is a huge progress bar
- by changing the size and the phrasing of the button drastically we reduce the likelihood that the user misses the change
but, as I said it's not so important to me.
There was a problem hiding this comment.
Let's let @nirvn make the call?
There was a problem hiding this comment.
+1 to "Canceling" without the please wait suffix.
There was a problem hiding this comment.
+1 and I think we should keep the ellipsis … as an indicator that something is going on (unless we can add an animated spinner).
There was a problem hiding this comment.
For the spinner, I suspect the cursor is already set to spinner when we click on [ Execute SQL ]. If it's not, +1 to spinner too.
There was a problem hiding this comment.
Ideally, the cursor shouldn't spin after a click onto "Execute SQL" if it's still possible to click on "Cancel". If any, I would suggest to add spinners and ellipseses directly onto GUI elements to put them into context.
| } | ||
|
|
||
| // This is gross but I tried with both conn and a context QObject without success: the lambda is never called. | ||
| QMetaObject::Connection moConn; |
There was a problem hiding this comment.
Could you rename this to remove the initial "m" prefix? Its confusing as it looks like a member variable at first glance.
|
Nice! I guess this can also be used to send back other information / warnings from the db directly to where the command was launched (e.g. db manager) instead of the global message bar / message log. |
elpaso
force-pushed
the
bugfix-gh38092-executesql-feedback
branch
from
1c076f6
to
6d866e3
Compare
|
@m-kuhn this PR is the GDAL part of the bugfix, we will eventually be able to use it when released OSGeo/gdal#2953 (comment) |
|
Thanks @elpaso good job! |
Fixes #38092 by adding an optional QgsFeedback argument to
the executeSql method and by implementing the PQCancel
method in the PG provider internals.
While the cancellation works well for all supported provider while
fetching results in the loop, the cancellation of a running query is now
implemented for the postgres provider connection only because the GPKG
and GDAL both rely on GDALDatasetExecuteSQL which cannot be interrupted.
This PR also introduce a few optimizations in the PG DB-Manager
code that should probably fix also other "slowness" issues that
were reported after 3.x during PG query execution.
A small UX change in the SQL dialog makes it evident to the user that
a cancellation request has been sent to the backend: the button text
is changed to "Cancelling, please wait..." so that for
provider connections that are not able to interrupt the running query
and must wait for the fetching loop to exit from the executeSql call
the user knows that something is happening and that a cancellation
request has been successfully sent.