QProgressDialog estimates the time the operation will take (based on time for steps), and only shows itself if that estimate is beyond minimumDuration.
The default minimumDuration value is 4 seconds, and it could make users think that the GUI is frozen.
This PR sets minimumDuration to zero for all progress dialogs, that affects ones in the WalletControllerActivity class.
QProgressDialog shows itself if the estimated time an operation will
take is beyond the minimumDuration value.
Direct call show() breaks that behavior on macos.
QProgressDialog estimates the time the operation will take (based on
time for steps), and only shows itself if that estimate is beyond
minimumDuration. The default minimumDuration value is 4 seconds, and it
could make users think that the GUI is frozen.
206@@ -207,6 +207,9 @@ void WalletControllerActivity::showProgressDialog(const QString& label_text)
207 m_progress_dialog->setCancelButton(nullptr);
208 m_progress_dialog->setWindowModality(Qt::ApplicationModal);
209 GUIUtil::PolishProgressDialog(m_progress_dialog);
210+ // The setValue call forces QProgressDialog to start the internal duration estimation.
211+ // See details in https://bugreports.qt.io/browse/QTBUG-47042.
In commit “qt: Improve GUI responsiveness” (4935ac583bbdc289dd31a1caae3d711edef742b6)
It could be useful to also add this comment other places we’re relying on this behavior (or maybe dedup the code more generally):
https://github.com/bitcoin/bitcoin/blob/860093401840d7aad7b439aeba0d1598933bc9c6/src/qt/bitcoingui.cpp#L1380 https://github.com/bitcoin/bitcoin/blob/860093401840d7aad7b439aeba0d1598933bc9c6/src/qt/walletview.cpp#L335
Thank you for your review.
I agree with you that there is a possibility to dedup the code more generally. As such a change must be refactoring, I intentionally leaved it for a follow up, if there are no objections from you.
A comment added here because this dialog is not actually a “progress” one, it is a busy indicator without meaningful values. But it requires to call setValue(0) to start internal timer.
In other cases calling setValue() is natural, as dialogs are real “progress” ones, and the value shows the progress.
824 #endif
825+ // QProgressDialog estimates the time the operation will take (based on time
826+ // for steps), and only shows itself if that estimate is beyond minimumDuration.
827+ // The default minimumDuration value is 4 seconds, and it could make users
828+ // think that the GUI is frozen.
829+ dialog->setMinimumDuration(0);
show()?
From QTBUG-47042:
QProgressDialogis designed to show itself automatically, based on an internal estimate for the duration of the operation and theminimumDurationproperty. You never callshow()orexec()on it manually. You’re also not supposed to keep it around when it’s not used. In 5.4, the only way to start the internal duration estimation was to callsetValue(0). But we noticed that many people didn’t callsetValue(0), but just handed external progress information on to the progress dialog, and whether or not that progress information started at 0 or, say, 1, would determine whether the dialog would show itself at all, or not. This was thought to be a bit fragile, so we changed it so that the start time of the duration estimate was the dialog construction time unlesssetValue(0)(or, more correctly,setValue(minimum())was called, leading to the dialog showing itselfminimumDurationafter its construction, even if it was constructed not for immediate use (which the dialog cannot distinguish from a really long-running operation).
ACK 4935ac583bbdc289dd31a1caae3d711edef742b6
Code review ACK and light tested ACK of progress bars. Patch looks good.